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 > >>> >
