> On May 30, 2019, at 4:47 PM, Owen Nichols <onich...@pivotal.io> wrote: > > Some folks have found it really helpful to have the PR author schedule a walk-through of the changes to give reviewers more context and explain the thinking behind the changes.
I always thought this is what a good commit message should be: an elevator pitch of the changes you made, providing enough context and/or directing where that context could be found for anyone who wants to look at the full diff. Formal walkthroughs should be unnecessary if we use commit history in a readable way, rather than just as a JIRA cross-reference. And while I agree that a strict requirement of X +1s would be counterproductive in the long term, I would encourage everyone to at least look over PRs that are perhaps outside their immediate wheelhouse. If everyone only stays in their comfort zones, you end up with, say, only Patrick and Robert feeling comfortable improving the build... On Fri, May 31, 2019 at 12:22 PM Nabarun Nag <n...@apache.org> wrote: > Hi, > > I agree with Dan, I would like to follow what he has suggested. > Also, I will not mind anyone following the 3 reviewers for everything if > they chose to do so. Just please don't make it blanket rule. > > Regards > Naba > > PS: I found this filter on github to look up PRs that I have reviewed till > date / awaiting reviews. > > Reviewed : is:pr is:closed reviewed-by:<github-username> > Awaiting review : is:pr is:closed review-requested:<github-username> > > On Fri, May 31, 2019 at 11:57 AM Udo Kohlmeyer <u...@apache.org> wrote: > > > Thank you Dan, > > > > Some guidance around how we can go about reviewing the code. > > > > I want to remind ALL > > committers.. > > > https://www.apache.org/dev/new-committers-guide.html#committer-responsibilities > > > > It states "/Each committer has a responsibility to monitor the changes > > made for potential issues, both coding and legal."/ > > > > I cannot see a reason to have any reviewers on trivial (spelling, typos). > > > > Post that, everything should have at least a view reviewers. 1 does not > > an opinion (or reviewed code) make, and I must agree with the statement > > that Owen made. 3 reviewers. > > > > Yes it is a real pita. As it takes dedication from ppl to review code > > and it takes away from our daily lives and the limited time we have to > > dedicate to it. Yet, I cannot understand how out of 70 committers (I'm > > using 70% fudge factor) we cannot get 3 reviews on correctness, style, > > potential bugs. In addition committers CAN nominate reviewers on PR's. > > In addition to the nomination of reviewers, I would advocate that > > reviewers of code not be more than 66% of a potentially biased team > > members. (given that I know many committers are employed by the same > > company). > > > > @Naba I agree, there is more work now. I now as a committer actually > > have to review the code of a fellow committer. BUT we all know that the > > space and code we work on is complex. Concurrent, distributed systems > > are not easy and being too close to the problem could cause blinders to > > issues which someone more removed could spot. The opposite is also true, > > too removed will only evaluate on basic criteria and might not spot > > issues. Regardless of this, we have many troops that can do work.. and 1 > > code review 1-2 twice a week is not going to kill us. > > > > I would also like to request of everyone, that when submitting a PR, > > keep an eye on it. Track it, ping the @dev channel if no progress is > > made. Oversights can happen and in some cases emails can be filtered out > > with new PR's or comments made on one's own PR. > > > > --Udo > > > > On 5/31/19 11:33, Dan Smith wrote: > > > As others pointed out - I think it's really important to remember that > > > people and community are much more important than process. That is a > key > > > part of "The Apache Way" - it's not a set very specific rules, but more > > of > > > a philosophy of building an open community. I think this page has a > good > > > take on the apache way - http://theapacheway.com/. > > > > > > The page I linked also mentions "Getting good enough consensus is often > > > better than voting." Apache is about consensus building, *not* about > > > voting. Ideally, the only things we should formally vote on are > > > irreversible decisions such as a release or new committer. > > > > > > Regarding code reviews, I think having a really strict process implies > > that > > > we don't trust our committers. Rather than that, maybe have some > > guidelines > > > for committers who aren't sure how much of a review to get. Here's > what I > > > feel like I've been trying to follow: > > > 1. Fixing a typo, broken spotless, etc. - just push it! > > > 2. Straightforward change - get at least one reviewer. Maybe a second > > > author on the commit should count here? > > > 3. Technically challenging, complicated change - get multiple reviewers > > > 4. New Public API, large features - Write up a wiki page and have a > > > discussion on the dev list to get feedback > > > > > > For all of these, if someone rejects your PR/feature, fix the issues or > > > talk with them. A good rule of thumb is if you are frustrated or > annoyed > > by > > > what they said - talk to them one-on-one first instead of answering > > > directly in the PR/thread. If you a really pissed, wait a day and then > > talk > > > to them one-on-one. > > > > > > -Dan > > > > > > On Fri, May 31, 2019 at 11:00 AM Helena Bales <hba...@pivotal.io> > wrote: > > > > > >> Thanks for the filter, Jinmei! > > >> > > >> +1 to Naba and Bruce. > > >> > > >> I will add that I think we should have a formal policy of getting *a* > > >> review (and more for large changes), and that PRs that are merged > > without > > >> one should include (in the PR) a comment providing a justification for > > this > > >> merge, so that committers can review the reasoning later on if needed. > > This > > >> has the benefits of being low impact on our current workflow, but also > > >> increasing transparency, accountability, and thoughtfulness around the > > >> proposed changes and their impact. > > >> > > >> On Fri, May 31, 2019 at 10:42 AM Jinmei Liao <jil...@pivotal.io> > wrote: > > >> > > >>> I was told that screenshot that I sent earlier got filtered out by > the > > >> dev > > >>> list. Basically, the filter puts "notificati...@github.com" in the > > >> "From" > > >>> section, and put "review_reques...@noreply.github.com" in the > "Doesn't > > >>> have" section of the form. > > >>> > > >>> On Fri, May 31, 2019 at 10:36 AM Anthony Baker <aba...@pivotal.io> > > >> wrote: > > >>>> > > >>>>> On May 31, 2019, at 10:01 AM, Owen Nichols <onich...@pivotal.io> > > >>> wrote: > > >>>>> We chose to make Geode an Apache open source project for a reason. > > >> If > > >>>> we no longer wish to embrace The Apache Way < > > >>>> https://www.apache.org/theapacheway/index.html>, perhaps we should > > >>>> reconsider. > > >>>> > > >>>> I strongly disagree with the assertion that we are not following the > > >>>> Apache Way because we aren’t doing RTC. Please take a look around > > >> other > > >>>> ASF communities and compare that to our approach. I think you’ll > see > > a > > >>> lot > > >>>> of similarities in the way we review GitHub pull requests. > > >>>> > > >>>>> If we believe that reviewing each other’s code changes is an > onerous > > >>>> burden of no value, we should question why. The long-term success > of > > >>>> Geode depends on sharing of knowledge, not “cowboy coders”. 3 > reviews > > >>>> means now 3 other people are more familiar with that part of the > code… > > >>>> > > >>>> Yes of course: community >> code. Can you point me to cases of > > >> “cowboy > > >>>> coding” in Geode? I’m not seeing it but happy to be convinced > > >> otherwise. > > >>>>> If apathy is our thing, Apache does allows for “lazy consensus”, > but > > >>> you > > >>>> have to declare that you will be using it, e.g. “This PR fixes > > >>>> GEODE-123456; if no-one objects within three days, I'll assume lazy > > >>>> consensus and merge it.” > > >>>> > > >>>> IMO lazy consensus does not imply apathy. > > >>>> > > >>>> > > >>>> > > >>>> Anthony > > >>>> > > >>>> > > >>> -- > > >>> Cheers > > >>> > > >>> Jinmei > > >>> > > >
