You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Owen Nichols <on...@pivotal.io> on 2019/05/30 22:51:42 UTC
[DISCUSS] require reviews before merging a PR
It seems common for Geode PRs to get merged with only a single green checkmark in GitHub.
According to https://www.apache.org/foundation/voting.html we should not be merging PRs with fewer than 3 green checkmarks.
Consensus is a fundamental value in doing things The Apache Way. A single +1 is not consensus. Since we’re currently discussing what it takes to become a committer and what standards a committer is expected to uphold, it seems like a good time to review this policy.
GitHub can be configured to require N reviews before a commit can be merged. Should we enable this feature?
-Owen
VOTES ON CODE MODIFICATION <https://www.apache.org/foundation/voting.html#votes-on-code-modification>
For code-modification votes, +1 votes are in favour of the proposal, but -1 votes are vetos <https://www.apache.org/foundation/voting.html#Veto> and kill the proposal dead until all vetoers withdraw their -1 votes.
Unless a vote has been declared as using lazy consensus <https://www.apache.org/foundation/voting.html#LazyConsensus> , three +1 votes are required for a code-modification proposal to pass.
Whole numbers are recommended for this type of vote, as the opinion being expressed is Boolean: 'I approve/do not approve of this change.'
CONSENSUS GAUGING THROUGH SILENCE <https://www.apache.org/foundation/voting.html#LazyConsensus>
An alternative to voting that is sometimes used to measure the acceptability of something is the concept of lazy consensus <https://www.apache.org/foundation/glossary.html#LazyConsensus>.
Lazy consensus is simply an announcement of 'silence gives assent.’ When someone wants to determine the sense of the community this way, it might do so with a mail message such as:
"The patch below fixes GEODE-12345; if no-one objects within three days, I'll assume lazy consensus and commit it."
Lazy consensus cannot be used on projects that enforce a review-then-commit <https://www.apache.org/foundation/glossary.html#ReviewThenCommit> policy.
Re: [DISCUSS] require reviews before merging a PR
Posted by Helena Bales <hb...@pivotal.io>.
To Naba's point, there's an even easier way to keep track of all the PRs:
https://github.com/pulls/review-requested
It shows your PRs, your assigned PRs, your requested reviews, and your
mentions (keep an eye on this tab, because people who aren't committers yet
can't request reviews and may instead @you in a comment on the PR). I use
this feature of GitHub almost every day and find it very useful since I
filter my emails.
On Fri, May 31, 2019 at 1:15 PM Anthony Baker <ab...@pivotal.io> wrote:
> I’m glad you raised this question because without it we wouldn’t have
> asked ourselves “What makes a good code review, when is it needed, and who
> should participate?”.
>
> Thank you!
>
> Anthony
>
>
> > On May 31, 2019, at 12:44 PM, Owen Nichols <on...@pivotal.io> wrote:
> >
> > I have learned that other than the required quarterly report to the
> board, just about everything else about being an Apache project is just
> guidelines, not hard requirements. I was confused because we do adhere
> rigorously to every other voting guideline on
> https://www.apache.org/foundation/voting.html; now I understand that is
> by choice and not because Apache “requires” it.
> >
> > Thank you for all the responses on this thread. It seems like the
> consensus is that we’ve struck an appropriate balance already (and in
> particular regard to reviews, that we can trust committers to seek an
> appropriate amount of review based on the nature and scope of a PR).
> >
> > I will not seek a vote on enforcing a requirement of 1 (or more) reviews
> before a PR can be merged, since some valid scenarios were raised where 0
> reviews prior to merge could be appropriate.
> >
> >> On May 31, 2019, at 9:01 AM, Jacob Barrett <jb...@pivotal.io> wrote:
> >>
> >>
> >>> On May 31, 2019, at 8:52 AM, Owen Nichols <on...@pivotal.io> wrote:
> >>>
> >>> Apache requires 3 reviews for code changes. Docs and typos likely
> would not
> >>> fall under that heading.
> >>
> >> Where is this listed as a requirement? The link you sent before
> offered guidance on common policies within the organization.
> >>
> >
>
>
Re: [DISCUSS] require reviews before merging a PR
Posted by Anthony Baker <ab...@pivotal.io>.
I’m glad you raised this question because without it we wouldn’t have asked ourselves “What makes a good code review, when is it needed, and who should participate?”.
Thank you!
Anthony
> On May 31, 2019, at 12:44 PM, Owen Nichols <on...@pivotal.io> wrote:
>
> I have learned that other than the required quarterly report to the board, just about everything else about being an Apache project is just guidelines, not hard requirements. I was confused because we do adhere rigorously to every other voting guideline on https://www.apache.org/foundation/voting.html; now I understand that is by choice and not because Apache “requires” it.
>
> Thank you for all the responses on this thread. It seems like the consensus is that we’ve struck an appropriate balance already (and in particular regard to reviews, that we can trust committers to seek an appropriate amount of review based on the nature and scope of a PR).
>
> I will not seek a vote on enforcing a requirement of 1 (or more) reviews before a PR can be merged, since some valid scenarios were raised where 0 reviews prior to merge could be appropriate.
>
>> On May 31, 2019, at 9:01 AM, Jacob Barrett <jb...@pivotal.io> wrote:
>>
>>
>>> On May 31, 2019, at 8:52 AM, Owen Nichols <on...@pivotal.io> wrote:
>>>
>>> Apache requires 3 reviews for code changes. Docs and typos likely would not
>>> fall under that heading.
>>
>> Where is this listed as a requirement? The link you sent before offered guidance on common policies within the organization.
>>
>
Re: [DISCUSS] require reviews before merging a PR
Posted by Xiaojian Zhou <gz...@pivotal.io>.
I think my recent practice with Eric Shu's PR #3623 could be a good
example.
In this specific bug with a lot of context, it's hard for Eric Shu to find
3 persons to review. Bruce and I are the 2 right persons who know the
history and context.
Eric came to me many times and we had a lot of discussion. I logged the
major design in GEM-2425 as comment in order not to forget (I think it's a
good idea to log the major design into either GEM or GEODE ticket). When he
finally sent me a PR, I challenged him how he implemented the design. He
walked me through the use cases and corner cases. I noticed he used some
trick logics to implement. Then I add a new comment in GEM-2425 to explain
the tricks.
As I feel it's good enough to approve, I purposely wait for Bruce to give
another review. Not surprisingly, Bruce raised the same concerns for the
tricks. Then I append the detail design and implementation into the PR to
see if it can convince Bruce. I saw Bruce asked Eric to add comments in the
code or javadoc for the tricks.
Above steps can be an alternative to review meetings. Since reviewers are
usually busy with their own assignments. If participated into 2 PR review
meetings a day, there's nothing to report for next day's standup.
If a PR requested multiple reviewers explicitly, then all of them should
approve. I think we can add this as a rule.
Regards
Gester
On Fri, May 31, 2019 at 2:10 PM Jacob Barrett <jb...@pivotal.io> wrote:
> I’ll be posting a PR for it later next week so y’all can review.
>
> > On May 31, 2019, at 2:02 PM, Helena Bales <hb...@pivotal.io> wrote:
> >
> > I'm happy to provide feedback on a CONTRIBUTING.md, but I don't want to
> > take the lead on this particular doc right now.
> >
> >> On Fri, May 31, 2019 at 1:48 PM Jacob Barrett <jb...@pivotal.io>
> wrote:
> >>
> >> It is probably worthwhile to codify our “policy” so that it’s not
> confused
> >> later. Simply adding something about lazy consensus model to the
> >> CONTRIBUTING.md (which I realize we are missing, already working on
> that)
> >> might be useful.
> >>
> >> I could take a stab at the wording based on my earlier reply about this
> if
> >> no one else wants to.
> >>
> >> -jake
> >>
> >>
> >>> On May 31, 2019, at 12:44 PM, Owen Nichols <on...@pivotal.io>
> wrote:
> >>>
> >>> I have learned that other than the required quarterly report to the
> >> board, just about everything else about being an Apache project is just
> >> guidelines, not hard requirements. I was confused because we do adhere
> >> rigorously to every other voting guideline on
> >> https://www.apache.org/foundation/voting.html; now I understand that is
> >> by choice and not because Apache “requires” it.
> >>>
> >>> Thank you for all the responses on this thread. It seems like the
> >> consensus is that we’ve struck an appropriate balance already (and in
> >> particular regard to reviews, that we can trust committers to seek an
> >> appropriate amount of review based on the nature and scope of a PR).
> >>>
> >>> I will not seek a vote on enforcing a requirement of 1 (or more)
> reviews
> >> before a PR can be merged, since some valid scenarios were raised where
> 0
> >> reviews prior to merge could be appropriate.
> >>>
> >>>> On May 31, 2019, at 9:01 AM, Jacob Barrett <jb...@pivotal.io>
> wrote:
> >>>>
> >>>>
> >>>>> On May 31, 2019, at 8:52 AM, Owen Nichols <on...@pivotal.io>
> wrote:
> >>>>>
> >>>>> Apache requires 3 reviews for code changes. Docs and typos likely
> >> would not
> >>>>> fall under that heading.
> >>>>
> >>>> Where is this listed as a requirement? The link you sent before
> >> offered guidance on common policies within the organization.
> >>>>
> >>>
> >>
>
Re: [DISCUSS] require reviews before merging a PR
Posted by Jacob Barrett <jb...@pivotal.io>.
I’ll be posting a PR for it later next week so y’all can review.
> On May 31, 2019, at 2:02 PM, Helena Bales <hb...@pivotal.io> wrote:
>
> I'm happy to provide feedback on a CONTRIBUTING.md, but I don't want to
> take the lead on this particular doc right now.
>
>> On Fri, May 31, 2019 at 1:48 PM Jacob Barrett <jb...@pivotal.io> wrote:
>>
>> It is probably worthwhile to codify our “policy” so that it’s not confused
>> later. Simply adding something about lazy consensus model to the
>> CONTRIBUTING.md (which I realize we are missing, already working on that)
>> might be useful.
>>
>> I could take a stab at the wording based on my earlier reply about this if
>> no one else wants to.
>>
>> -jake
>>
>>
>>> On May 31, 2019, at 12:44 PM, Owen Nichols <on...@pivotal.io> wrote:
>>>
>>> I have learned that other than the required quarterly report to the
>> board, just about everything else about being an Apache project is just
>> guidelines, not hard requirements. I was confused because we do adhere
>> rigorously to every other voting guideline on
>> https://www.apache.org/foundation/voting.html; now I understand that is
>> by choice and not because Apache “requires” it.
>>>
>>> Thank you for all the responses on this thread. It seems like the
>> consensus is that we’ve struck an appropriate balance already (and in
>> particular regard to reviews, that we can trust committers to seek an
>> appropriate amount of review based on the nature and scope of a PR).
>>>
>>> I will not seek a vote on enforcing a requirement of 1 (or more) reviews
>> before a PR can be merged, since some valid scenarios were raised where 0
>> reviews prior to merge could be appropriate.
>>>
>>>> On May 31, 2019, at 9:01 AM, Jacob Barrett <jb...@pivotal.io> wrote:
>>>>
>>>>
>>>>> On May 31, 2019, at 8:52 AM, Owen Nichols <on...@pivotal.io> wrote:
>>>>>
>>>>> Apache requires 3 reviews for code changes. Docs and typos likely
>> would not
>>>>> fall under that heading.
>>>>
>>>> Where is this listed as a requirement? The link you sent before
>> offered guidance on common policies within the organization.
>>>>
>>>
>>
Re: [DISCUSS] require reviews before merging a PR
Posted by Helena Bales <hb...@pivotal.io>.
I'm happy to provide feedback on a CONTRIBUTING.md, but I don't want to
take the lead on this particular doc right now.
On Fri, May 31, 2019 at 1:48 PM Jacob Barrett <jb...@pivotal.io> wrote:
> It is probably worthwhile to codify our “policy” so that it’s not confused
> later. Simply adding something about lazy consensus model to the
> CONTRIBUTING.md (which I realize we are missing, already working on that)
> might be useful.
>
> I could take a stab at the wording based on my earlier reply about this if
> no one else wants to.
>
> -jake
>
>
> > On May 31, 2019, at 12:44 PM, Owen Nichols <on...@pivotal.io> wrote:
> >
> > I have learned that other than the required quarterly report to the
> board, just about everything else about being an Apache project is just
> guidelines, not hard requirements. I was confused because we do adhere
> rigorously to every other voting guideline on
> https://www.apache.org/foundation/voting.html; now I understand that is
> by choice and not because Apache “requires” it.
> >
> > Thank you for all the responses on this thread. It seems like the
> consensus is that we’ve struck an appropriate balance already (and in
> particular regard to reviews, that we can trust committers to seek an
> appropriate amount of review based on the nature and scope of a PR).
> >
> > I will not seek a vote on enforcing a requirement of 1 (or more) reviews
> before a PR can be merged, since some valid scenarios were raised where 0
> reviews prior to merge could be appropriate.
> >
> >> On May 31, 2019, at 9:01 AM, Jacob Barrett <jb...@pivotal.io> wrote:
> >>
> >>
> >>> On May 31, 2019, at 8:52 AM, Owen Nichols <on...@pivotal.io> wrote:
> >>>
> >>> Apache requires 3 reviews for code changes. Docs and typos likely
> would not
> >>> fall under that heading.
> >>
> >> Where is this listed as a requirement? The link you sent before
> offered guidance on common policies within the organization.
> >>
> >
>
Re: [DISCUSS] require reviews before merging a PR
Posted by Jacob Barrett <jb...@pivotal.io>.
It is probably worthwhile to codify our “policy” so that it’s not confused later. Simply adding something about lazy consensus model to the CONTRIBUTING.md (which I realize we are missing, already working on that) might be useful.
I could take a stab at the wording based on my earlier reply about this if no one else wants to.
-jake
> On May 31, 2019, at 12:44 PM, Owen Nichols <on...@pivotal.io> wrote:
>
> I have learned that other than the required quarterly report to the board, just about everything else about being an Apache project is just guidelines, not hard requirements. I was confused because we do adhere rigorously to every other voting guideline on https://www.apache.org/foundation/voting.html; now I understand that is by choice and not because Apache “requires” it.
>
> Thank you for all the responses on this thread. It seems like the consensus is that we’ve struck an appropriate balance already (and in particular regard to reviews, that we can trust committers to seek an appropriate amount of review based on the nature and scope of a PR).
>
> I will not seek a vote on enforcing a requirement of 1 (or more) reviews before a PR can be merged, since some valid scenarios were raised where 0 reviews prior to merge could be appropriate.
>
>> On May 31, 2019, at 9:01 AM, Jacob Barrett <jb...@pivotal.io> wrote:
>>
>>
>>> On May 31, 2019, at 8:52 AM, Owen Nichols <on...@pivotal.io> wrote:
>>>
>>> Apache requires 3 reviews for code changes. Docs and typos likely would not
>>> fall under that heading.
>>
>> Where is this listed as a requirement? The link you sent before offered guidance on common policies within the organization.
>>
>
Re: [DISCUSS] require reviews before merging a PR
Posted by Owen Nichols <on...@pivotal.io>.
I have learned that other than the required quarterly report to the board, just about everything else about being an Apache project is just guidelines, not hard requirements. I was confused because we do adhere rigorously to every other voting guideline on https://www.apache.org/foundation/voting.html; now I understand that is by choice and not because Apache “requires” it.
Thank you for all the responses on this thread. It seems like the consensus is that we’ve struck an appropriate balance already (and in particular regard to reviews, that we can trust committers to seek an appropriate amount of review based on the nature and scope of a PR).
I will not seek a vote on enforcing a requirement of 1 (or more) reviews before a PR can be merged, since some valid scenarios were raised where 0 reviews prior to merge could be appropriate.
> On May 31, 2019, at 9:01 AM, Jacob Barrett <jb...@pivotal.io> wrote:
>
>
>> On May 31, 2019, at 8:52 AM, Owen Nichols <on...@pivotal.io> wrote:
>>
>> Apache requires 3 reviews for code changes. Docs and typos likely would not
>> fall under that heading.
>
> Where is this listed as a requirement? The link you sent before offered guidance on common policies within the organization.
>
Re: [DISCUSS] require reviews before merging a PR
Posted by Patrick Rhomberg <pr...@apache.org>.
> On May 30, 2019, at 4:47 PM, Owen Nichols <on...@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 <nn...@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 <ud...@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 <hb...@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 <ji...@pivotal.io>
> wrote:
> > >>
> > >>> I was told that screenshot that I sent earlier got filtered out by
> the
> > >> dev
> > >>> list. Basically, the filter puts "notifications@github.com" in the
> > >> "From"
> > >>> section, and put "review_requested@noreply.github.com" in the
> "Doesn't
> > >>> have" section of the form.
> > >>>
> > >>> On Fri, May 31, 2019 at 10:36 AM Anthony Baker <ab...@pivotal.io>
> > >> wrote:
> > >>>>
> > >>>>> On May 31, 2019, at 10:01 AM, Owen Nichols <on...@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
> > >>>
> >
>
Re: [DISCUSS] require reviews before merging a PR
Posted by Nabarun Nag <nn...@apache.org>.
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 <ud...@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 <hb...@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 <ji...@pivotal.io> wrote:
> >>
> >>> I was told that screenshot that I sent earlier got filtered out by the
> >> dev
> >>> list. Basically, the filter puts "notifications@github.com" in the
> >> "From"
> >>> section, and put "review_requested@noreply.github.com" in the "Doesn't
> >>> have" section of the form.
> >>>
> >>> On Fri, May 31, 2019 at 10:36 AM Anthony Baker <ab...@pivotal.io>
> >> wrote:
> >>>>
> >>>>> On May 31, 2019, at 10:01 AM, Owen Nichols <on...@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
> >>>
>
Re: [DISCUSS] require reviews before merging a PR
Posted by Udo Kohlmeyer <ud...@apache.org>.
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 <hb...@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 <ji...@pivotal.io> wrote:
>>
>>> I was told that screenshot that I sent earlier got filtered out by the
>> dev
>>> list. Basically, the filter puts "notifications@github.com" in the
>> "From"
>>> section, and put "review_requested@noreply.github.com" in the "Doesn't
>>> have" section of the form.
>>>
>>> On Fri, May 31, 2019 at 10:36 AM Anthony Baker <ab...@pivotal.io>
>> wrote:
>>>>
>>>>> On May 31, 2019, at 10:01 AM, Owen Nichols <on...@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
>>>
Re: [DISCUSS] require reviews before merging a PR
Posted by Dan Smith <ds...@pivotal.io>.
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 <hb...@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 <ji...@pivotal.io> wrote:
>
> > I was told that screenshot that I sent earlier got filtered out by the
> dev
> > list. Basically, the filter puts "notifications@github.com" in the
> "From"
> > section, and put "review_requested@noreply.github.com" in the "Doesn't
> > have" section of the form.
> >
> > On Fri, May 31, 2019 at 10:36 AM Anthony Baker <ab...@pivotal.io>
> wrote:
> >
> > >
> > >
> > > > On May 31, 2019, at 10:01 AM, Owen Nichols <on...@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
> >
>
Re: [DISCUSS] require reviews before merging a PR
Posted by Helena Bales <hb...@pivotal.io>.
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 <ji...@pivotal.io> wrote:
> I was told that screenshot that I sent earlier got filtered out by the dev
> list. Basically, the filter puts "notifications@github.com" in the "From"
> section, and put "review_requested@noreply.github.com" in the "Doesn't
> have" section of the form.
>
> On Fri, May 31, 2019 at 10:36 AM Anthony Baker <ab...@pivotal.io> wrote:
>
> >
> >
> > > On May 31, 2019, at 10:01 AM, Owen Nichols <on...@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
>
Re: [DISCUSS] require reviews before merging a PR
Posted by Jinmei Liao <ji...@pivotal.io>.
I was told that screenshot that I sent earlier got filtered out by the dev
list. Basically, the filter puts "notifications@github.com" in the "From"
section, and put "review_requested@noreply.github.com" in the "Doesn't
have" section of the form.
On Fri, May 31, 2019 at 10:36 AM Anthony Baker <ab...@pivotal.io> wrote:
>
>
> > On May 31, 2019, at 10:01 AM, Owen Nichols <on...@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
Re: [DISCUSS] require reviews before merging a PR
Posted by Anthony Baker <ab...@pivotal.io>.
> On May 31, 2019, at 10:01 AM, Owen Nichols <on...@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
Re: [DISCUSS] require reviews before merging a PR
Posted by Nabarun Nag <nn...@apache.org>.
Hi,
I personally feel the same as how Bruce feels.
- This will make life harder / inconvenient.
- One approval from a person who is experienced in that part of code is
more than enough for me.
- The workload on the Geode developers is already too high, it is unfair
to burden then with more tasks which can be avoided.
- The commits are well written with proper description, hence anyone who
wants to get familiar with the code can go and read them.
- Comparing other projects like Kafka, Hadoop, Spark and their closed pull
requests, none of them have 3 approvals.
- Only project I saw was Kubernetes (not ASF) who need minimum 2 approvals
from experts in the module on which the PR is opened and they have over
2000 contributors.
Personally its a -1 for me as the inconveniences outweigh the benefits of
this task.
Regards
Naba
On Fri, May 31, 2019 at 10:10 AM Jinmei Liao <ji...@pivotal.io> wrote:
> One reason for lacking reviews in the PR might be that people are
> filtering out notifications from github. I've had this problem for a long
> time before I finally figured out a way to filter out noses "other than"
> the review requested emails. Here is a snapshot of my filter setup.
> Hopefully it will be useful to you:
>
> [image: Screen Shot 2019-05-31 at 10.07.26 AM.png]
>
> On Fri, May 31, 2019 at 10:02 AM Owen Nichols <on...@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.
>>
>> 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...
>>
>> 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."
>>
>> > On May 31, 2019, at 9:05 AM, Alexander Murmann <am...@pivotal.io>
>> wrote:
>> >
>> > Jake, Owen is quoting the "VOTES ON CODE MODIFICATION" section from
>> > https://www.apache.org/foundation/voting.html . "code modification" ==
>> > "every PR" is a interpretation that I think would bring the project to a
>> > grinding halt.
>> >
>> > On Fri, May 31, 2019 at 9:01 AM Jacob Barrett <jb...@pivotal.io>
>> wrote:
>> >
>> >>
>> >>
>> >>> On May 31, 2019, at 8:52 AM, Owen Nichols <on...@pivotal.io>
>> wrote:
>> >>>
>> >>> Apache requires 3 reviews for code changes. Docs and typos likely
>> would
>> >> not
>> >>> fall under that heading.
>> >>
>> >> Where is this listed as a requirement? The link you sent before
>> offered
>> >> guidance on common policies within the organization.
>> >>
>> >>
>>
>>
>
> --
> Cheers
>
> Jinmei
>
Re: [DISCUSS] require reviews before merging a PR
Posted by Jinmei Liao <ji...@pivotal.io>.
One reason for lacking reviews in the PR might be that people are filtering
out notifications from github. I've had this problem for a long time before
I finally figured out a way to filter out noses "other than" the review
requested emails. Here is a snapshot of my filter setup. Hopefully it will
be useful to you:
[image: Screen Shot 2019-05-31 at 10.07.26 AM.png]
On Fri, May 31, 2019 at 10:02 AM Owen Nichols <on...@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.
>
> 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...
>
> 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."
>
> > On May 31, 2019, at 9:05 AM, Alexander Murmann <am...@pivotal.io>
> wrote:
> >
> > Jake, Owen is quoting the "VOTES ON CODE MODIFICATION" section from
> > https://www.apache.org/foundation/voting.html . "code modification" ==
> > "every PR" is a interpretation that I think would bring the project to a
> > grinding halt.
> >
> > On Fri, May 31, 2019 at 9:01 AM Jacob Barrett <jb...@pivotal.io>
> wrote:
> >
> >>
> >>
> >>> On May 31, 2019, at 8:52 AM, Owen Nichols <on...@pivotal.io> wrote:
> >>>
> >>> Apache requires 3 reviews for code changes. Docs and typos likely would
> >> not
> >>> fall under that heading.
> >>
> >> Where is this listed as a requirement? The link you sent before offered
> >> guidance on common policies within the organization.
> >>
> >>
>
>
--
Cheers
Jinmei
Re: [DISCUSS] require reviews before merging a PR
Posted by Owen Nichols <on...@pivotal.io>.
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.
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...
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."
> On May 31, 2019, at 9:05 AM, Alexander Murmann <am...@pivotal.io> wrote:
>
> Jake, Owen is quoting the "VOTES ON CODE MODIFICATION" section from
> https://www.apache.org/foundation/voting.html . "code modification" ==
> "every PR" is a interpretation that I think would bring the project to a
> grinding halt.
>
> On Fri, May 31, 2019 at 9:01 AM Jacob Barrett <jb...@pivotal.io> wrote:
>
>>
>>
>>> On May 31, 2019, at 8:52 AM, Owen Nichols <on...@pivotal.io> wrote:
>>>
>>> Apache requires 3 reviews for code changes. Docs and typos likely would
>> not
>>> fall under that heading.
>>
>> Where is this listed as a requirement? The link you sent before offered
>> guidance on common policies within the organization.
>>
>>
Re: [DISCUSS] require reviews before merging a PR
Posted by Anthony Baker <ab...@pivotal.io>.
When asking a question like “What is ASF policy and practice on XXX?” I have found that observing other ASF projects can be helpful. In particular, the ASF Incubator (general@incubator.apache.org <ma...@incubator.apache.org>) covers these topics fairly frequently.
Anthony
> On May 31, 2019, at 9:05 AM, Alexander Murmann <am...@pivotal.io> wrote:
>
> Jake, Owen is quoting the "VOTES ON CODE MODIFICATION" section from
> https://www.apache.org/foundation/voting.html . "code modification" ==
> "every PR" is a interpretation that I think would bring the project to a
> grinding halt.
>
> On Fri, May 31, 2019 at 9:01 AM Jacob Barrett <jb...@pivotal.io> wrote:
>
>>
>>
>>> On May 31, 2019, at 8:52 AM, Owen Nichols <on...@pivotal.io> wrote:
>>>
>>> Apache requires 3 reviews for code changes. Docs and typos likely would
>> not
>>> fall under that heading.
>>
>> Where is this listed as a requirement? The link you sent before offered
>> guidance on common policies within the organization.
>>
>>
Re: [DISCUSS] require reviews before merging a PR
Posted by Alexander Murmann <am...@pivotal.io>.
Jake, Owen is quoting the "VOTES ON CODE MODIFICATION" section from
https://www.apache.org/foundation/voting.html . "code modification" ==
"every PR" is a interpretation that I think would bring the project to a
grinding halt.
On Fri, May 31, 2019 at 9:01 AM Jacob Barrett <jb...@pivotal.io> wrote:
>
>
> > On May 31, 2019, at 8:52 AM, Owen Nichols <on...@pivotal.io> wrote:
> >
> > Apache requires 3 reviews for code changes. Docs and typos likely would
> not
> > fall under that heading.
>
> Where is this listed as a requirement? The link you sent before offered
> guidance on common policies within the organization.
>
>
Re: [DISCUSS] require reviews before merging a PR
Posted by Jacob Barrett <jb...@pivotal.io>.
> On May 31, 2019, at 8:52 AM, Owen Nichols <on...@pivotal.io> wrote:
>
> Apache requires 3 reviews for code changes. Docs and typos likely would not
> fall under that heading.
Where is this listed as a requirement? The link you sent before offered guidance on common policies within the organization.
Re: [DISCUSS] require reviews before merging a PR
Posted by Alexander Murmann <am...@pivotal.io>.
+1 to Jake's interpretation of the voting system not having been adjusted
yet for the new world that is GitHub. When I read the Apache voting
guidelines they make sense to me for bug decisions but not for a regular
PR.
The inertia coming with three votes on every PR is way more costly than the
alternative of an occasional rewind.
On Fri, May 31, 2019 at 8:52 AM Owen Nichols <on...@pivotal.io> wrote:
> Apache requires 3 reviews for code changes. Docs and typos likely would not
> fall under that heading.
>
> On Fri, May 31, 2019 at 8:47 AM Dave Barnes <db...@apache.org> wrote:
>
> > As a writer, I'm a big user of Lazy Consensus: If no one objects, I'm
> > merging my change. Requiring multiple reviews discourages minor
> > improvements. In the doc realm, I'm inclined to check in typo fixes and
> > grammar corrections without even bothering with the PR process, but I do
> it
> > for the community-ness of it. But requiring three reviews to correct a
> > spelling error is a big waste of the reviewers' time.
> >
> > On Thu, May 30, 2019 at 10:12 PM Jacob Barrett <jb...@pivotal.io>
> > wrote:
> >
> > >
> > >
> > > > On May 30, 2019, at 4:47 PM, Owen Nichols <on...@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.
> > >
> > > This can’t be policy unless the walkthrough is scheduled with the whole
> > > dev@geode community. You could say in your PR that a walkthrough will
> > > happen at a given time and location (online) so that interested parties
> > > could watch and ask questions. This strikes me as extremely onerous for
> > > most PRs. For large scale refactors, features, etc. maybe it makes
> sense,
> > > though for those a discussion thread should have happened on dev@geode
> > > first.
> > >
> > > -Jake
> > >
> > >
> >
>
Re: [DISCUSS] require reviews before merging a PR
Posted by Anthony Baker <ab...@pivotal.io>.
> On May 31, 2019, at 8:52 AM, Owen Nichols <on...@pivotal.io> wrote:
>
> Apache requires 3 reviews for code changes. Docs and typos likely would not
> fall under that heading.
>
Code change == commit of any kind to the source repo(s).
I agree that a strict RTC approach is as you described. ASF doesn’t mandate that projects follow RTC.
Anthony
Re: [DISCUSS] require reviews before merging a PR
Posted by Owen Nichols <on...@pivotal.io>.
Apache requires 3 reviews for code changes. Docs and typos likely would not
fall under that heading.
On Fri, May 31, 2019 at 8:47 AM Dave Barnes <db...@apache.org> wrote:
> As a writer, I'm a big user of Lazy Consensus: If no one objects, I'm
> merging my change. Requiring multiple reviews discourages minor
> improvements. In the doc realm, I'm inclined to check in typo fixes and
> grammar corrections without even bothering with the PR process, but I do it
> for the community-ness of it. But requiring three reviews to correct a
> spelling error is a big waste of the reviewers' time.
>
> On Thu, May 30, 2019 at 10:12 PM Jacob Barrett <jb...@pivotal.io>
> wrote:
>
> >
> >
> > > On May 30, 2019, at 4:47 PM, Owen Nichols <on...@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.
> >
> > This can’t be policy unless the walkthrough is scheduled with the whole
> > dev@geode community. You could say in your PR that a walkthrough will
> > happen at a given time and location (online) so that interested parties
> > could watch and ask questions. This strikes me as extremely onerous for
> > most PRs. For large scale refactors, features, etc. maybe it makes sense,
> > though for those a discussion thread should have happened on dev@geode
> > first.
> >
> > -Jake
> >
> >
>
Re: [DISCUSS] require reviews before merging a PR
Posted by Dave Barnes <db...@apache.org>.
As a writer, I'm a big user of Lazy Consensus: If no one objects, I'm
merging my change. Requiring multiple reviews discourages minor
improvements. In the doc realm, I'm inclined to check in typo fixes and
grammar corrections without even bothering with the PR process, but I do it
for the community-ness of it. But requiring three reviews to correct a
spelling error is a big waste of the reviewers' time.
On Thu, May 30, 2019 at 10:12 PM Jacob Barrett <jb...@pivotal.io> wrote:
>
>
> > On May 30, 2019, at 4:47 PM, Owen Nichols <on...@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.
>
> This can’t be policy unless the walkthrough is scheduled with the whole
> dev@geode community. You could say in your PR that a walkthrough will
> happen at a given time and location (online) so that interested parties
> could watch and ask questions. This strikes me as extremely onerous for
> most PRs. For large scale refactors, features, etc. maybe it makes sense,
> though for those a discussion thread should have happened on dev@geode
> first.
>
> -Jake
>
>
Re: [DISCUSS] require reviews before merging a PR
Posted by Jacob Barrett <jb...@pivotal.io>.
> On May 30, 2019, at 4:47 PM, Owen Nichols <on...@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.
This can’t be policy unless the walkthrough is scheduled with the whole dev@geode community. You could say in your PR that a walkthrough will happen at a given time and location (online) so that interested parties could watch and ask questions. This strikes me as extremely onerous for most PRs. For large scale refactors, features, etc. maybe it makes sense, though for those a discussion thread should have happened on dev@geode first.
-Jake
Re: [DISCUSS] require reviews before merging a PR
Posted by Jacob Barrett <jb...@pivotal.io>.
> On May 30, 2019, at 5:00 PM, Anthony Baker <ab...@pivotal.io> wrote:
>
> Checkout [1] for some helpful context from the early days.
>
> Anthony
>
> [1] https://lists.apache.org/thread.html/108602a14b422abe9c94d46b2c5d02c11a9cbb8b224db08b706c6263@1430991799@%3Cdev.geode.apache.org%3E <https://lists.apache.org/thread.html/108602a14b422abe9c94d46b2c5d02c11a9cbb8b224db08b706c6263@1430991799@%3Cdev.geode.apache.org%3E>
So it looks like at some point early on we decided on RTC but in practice were are somewhere in between RTC and CTR. This policy was chosen when we were using Apache git and Review Board. These policies have not really been updated for the GitHub PR workflow since Apache moved all git repositories to GitHub. Perhaps we should rectify that.
RTC has proven to be less than efficient as it is difficult to get a single reviewer let alone 3.
CTR would imply that we merged a PR and then get some lazy consensus to keep or revert.
We have been practicing a hybrid in that we post a PR, wait from some lazy consensus and then commit. We could call it LRTC (lazy review then commit).
I don’t think Apache really mandates any one policy, just that you have policy and stick to it so that everyone is on even ground.
We can’t expect that for each PR posted that we also babysit and wrangle up 3 members on the community to give a review. Sure this makes sense for large scale architectural changes, new features, and some nasty refactoring, but if we define a default of LRTC and the the contributor can request a RTC policy on a per PR basis.
-Jake
Re: [DISCUSS] require reviews before merging a PR
Posted by Anthony Baker <ab...@pivotal.io>.
Checkout [1] for some helpful context from the early days.
Anthony
[1] https://lists.apache.org/thread.html/108602a14b422abe9c94d46b2c5d02c11a9cbb8b224db08b706c6263@1430991799@%3Cdev.geode.apache.org%3E <https://lists.apache.org/thread.html/108602a14b422abe9c94d46b2c5d02c11a9cbb8b224db08b706c6263@1430991799@%3Cdev.geode.apache.org%3E>
> On May 30, 2019, at 4:47 PM, Owen Nichols <on...@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.
>
> Yes, it’s more work to get reviews. Apache says (really, mandates) that this must be part of the process. I started this discussion to come to consensus on what is expected of a committer. Being clear on expectations will help us both to evaluate potential new committers, and also to hold each other accountable.
>
> -Owen
>
>> On May 30, 2019, at 4:25 PM, Bruce Schuchardt <bs...@pivotal.io> wrote:
>>
>> Maybe your experience is different but I find it hard enough to get even one person to review my pull requests. I've resorted to merging minor changes without a review a few times due to lack of response.
>>
>>
>> On 5/30/19 3:51 PM, Owen Nichols wrote:
>>> It seems common for Geode PRs to get merged with only a single green checkmark in GitHub.
>>>
>>> According to https://www.apache.org/foundation/voting.html we should not be merging PRs with fewer than 3 green checkmarks.
>>>
>>> Consensus is a fundamental value in doing things The Apache Way. A single +1 is not consensus. Since we’re currently discussing what it takes to become a committer and what standards a committer is expected to uphold, it seems like a good time to review this policy.
>>>
>>> GitHub can be configured to require N reviews before a commit can be merged. Should we enable this feature?
>>>
>>> -Owen
>>> VOTES ON CODE MODIFICATION <https://www.apache.org/foundation/voting.html#votes-on-code-modification>
>>> For code-modification votes, +1 votes are in favour of the proposal, but -1 votes are vetos <https://www.apache.org/foundation/voting.html#Veto> and kill the proposal dead until all vetoers withdraw their -1 votes.
>>>
>>> Unless a vote has been declared as using lazy consensus <https://www.apache.org/foundation/voting.html#LazyConsensus> , three +1 votes are required for a code-modification proposal to pass.
>>>
>>> Whole numbers are recommended for this type of vote, as the opinion being expressed is Boolean: 'I approve/do not approve of this change.'
>>>
>>>
>>> CONSENSUS GAUGING THROUGH SILENCE <https://www.apache.org/foundation/voting.html#LazyConsensus>
>>> An alternative to voting that is sometimes used to measure the acceptability of something is the concept of lazy consensus <https://www.apache.org/foundation/glossary.html#LazyConsensus>.
>>>
>>> Lazy consensus is simply an announcement of 'silence gives assent.’ When someone wants to determine the sense of the community this way, it might do so with a mail message such as:
>>> "The patch below fixes GEODE-12345; if no-one objects within three days, I'll assume lazy consensus and commit it."
>>>
>>> Lazy consensus cannot be used on projects that enforce a review-then-commit <https://www.apache.org/foundation/glossary.html#ReviewThenCommit> policy.
>>>
>>>
>>>
>
Re: [DISCUSS] require reviews before merging a PR
Posted by Owen Nichols <on...@pivotal.io>.
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.
Yes, it’s more work to get reviews. Apache says (really, mandates) that this must be part of the process. I started this discussion to come to consensus on what is expected of a committer. Being clear on expectations will help us both to evaluate potential new committers, and also to hold each other accountable.
-Owen
> On May 30, 2019, at 4:25 PM, Bruce Schuchardt <bs...@pivotal.io> wrote:
>
> Maybe your experience is different but I find it hard enough to get even one person to review my pull requests. I've resorted to merging minor changes without a review a few times due to lack of response.
>
>
> On 5/30/19 3:51 PM, Owen Nichols wrote:
>> It seems common for Geode PRs to get merged with only a single green checkmark in GitHub.
>>
>> According to https://www.apache.org/foundation/voting.html we should not be merging PRs with fewer than 3 green checkmarks.
>>
>> Consensus is a fundamental value in doing things The Apache Way. A single +1 is not consensus. Since we’re currently discussing what it takes to become a committer and what standards a committer is expected to uphold, it seems like a good time to review this policy.
>>
>> GitHub can be configured to require N reviews before a commit can be merged. Should we enable this feature?
>>
>> -Owen
>> VOTES ON CODE MODIFICATION <https://www.apache.org/foundation/voting.html#votes-on-code-modification>
>> For code-modification votes, +1 votes are in favour of the proposal, but -1 votes are vetos <https://www.apache.org/foundation/voting.html#Veto> and kill the proposal dead until all vetoers withdraw their -1 votes.
>>
>> Unless a vote has been declared as using lazy consensus <https://www.apache.org/foundation/voting.html#LazyConsensus> , three +1 votes are required for a code-modification proposal to pass.
>>
>> Whole numbers are recommended for this type of vote, as the opinion being expressed is Boolean: 'I approve/do not approve of this change.'
>>
>>
>> CONSENSUS GAUGING THROUGH SILENCE <https://www.apache.org/foundation/voting.html#LazyConsensus>
>> An alternative to voting that is sometimes used to measure the acceptability of something is the concept of lazy consensus <https://www.apache.org/foundation/glossary.html#LazyConsensus>.
>>
>> Lazy consensus is simply an announcement of 'silence gives assent.’ When someone wants to determine the sense of the community this way, it might do so with a mail message such as:
>> "The patch below fixes GEODE-12345; if no-one objects within three days, I'll assume lazy consensus and commit it."
>>
>> Lazy consensus cannot be used on projects that enforce a review-then-commit <https://www.apache.org/foundation/glossary.html#ReviewThenCommit> policy.
>>
>>
>>
Re: [DISCUSS] require reviews before merging a PR
Posted by Bruce Schuchardt <bs...@pivotal.io>.
Maybe your experience is different but I find it hard enough to get even
one person to review my pull requests. I've resorted to merging minor
changes without a review a few times due to lack of response.
On 5/30/19 3:51 PM, Owen Nichols wrote:
> It seems common for Geode PRs to get merged with only a single green checkmark in GitHub.
>
> According to https://www.apache.org/foundation/voting.html we should not be merging PRs with fewer than 3 green checkmarks.
>
> Consensus is a fundamental value in doing things The Apache Way. A single +1 is not consensus. Since we’re currently discussing what it takes to become a committer and what standards a committer is expected to uphold, it seems like a good time to review this policy.
>
> GitHub can be configured to require N reviews before a commit can be merged. Should we enable this feature?
>
> -Owen
> VOTES ON CODE MODIFICATION <https://www.apache.org/foundation/voting.html#votes-on-code-modification>
> For code-modification votes, +1 votes are in favour of the proposal, but -1 votes are vetos <https://www.apache.org/foundation/voting.html#Veto> and kill the proposal dead until all vetoers withdraw their -1 votes.
>
> Unless a vote has been declared as using lazy consensus <https://www.apache.org/foundation/voting.html#LazyConsensus> , three +1 votes are required for a code-modification proposal to pass.
>
> Whole numbers are recommended for this type of vote, as the opinion being expressed is Boolean: 'I approve/do not approve of this change.'
>
>
> CONSENSUS GAUGING THROUGH SILENCE <https://www.apache.org/foundation/voting.html#LazyConsensus>
> An alternative to voting that is sometimes used to measure the acceptability of something is the concept of lazy consensus <https://www.apache.org/foundation/glossary.html#LazyConsensus>.
>
> Lazy consensus is simply an announcement of 'silence gives assent.’ When someone wants to determine the sense of the community this way, it might do so with a mail message such as:
> "The patch below fixes GEODE-12345; if no-one objects within three days, I'll assume lazy consensus and commit it."
>
> Lazy consensus cannot be used on projects that enforce a review-then-commit <https://www.apache.org/foundation/glossary.html#ReviewThenCommit> policy.
>
>
>
Re: [DISCUSS] require reviews before merging a PR
Posted by Joris Melchior <jm...@pivotal.io>.
Thanks Patrick. That's what we're doing currently so we'll continue to do
so :).
On Thu, Jun 6, 2019 at 12:40 PM Patrick Rhomberg <pr...@apache.org>
wrote:
> >
> > I'm not sure it's possible to do this with the "reviewers" field - if
> > someone can figure out how to do this with the github IU, we can at least
> > try filing an ticket with apache infrastructure.
> >
>
> According to their docs [1], "collaborator with write access" is the GitHub
> required criteria to add reviewers. So, yeah, you have to be a committer.
>
> If you're not a committer, you could always request in your PR comments for
> a committer to add some specific people, should you have any in mind.
>
> Imagination is Change.
> ~Patrick
>
> [1] https://help.github.com/en/articles/requesting-a-pull-request-review
>
> On Thu, Jun 6, 2019 at 9:26 AM Dan Smith <ds...@pivotal.io> wrote:
>
> > >
> > > Would it be possible to allow people who do not have committer status
> to
> > > request reviewers on a pull request.
> >
> >
> > I'm not sure it's possible to do this with the "reviewers" field - if
> > someone can figure out how to do this with the github IU, we can at least
> > try filing an ticket with apache infrastructure.
> >
> > In the meantime, you can also just add a comment mentioning the people
> > you'd like to review the request. That works for requesting reviews from
> > non-committers as well.
> >
> > -Dan
> >
> > On Thu, Jun 6, 2019 at 9:05 AM Joris Melchior <jm...@pivotal.io>
> > wrote:
> >
> > > Would it be possible to allow people who do not have committer status
> to
> > > request reviewers on a pull request. In some cases we may know who
> should
> > > take a look at it and in that case making it official by adding these
> > > people to the pull request would be good IMO.
> > >
> > > On Thu, Jun 6, 2019 at 10:26 AM Jens Deppe <jd...@pivotal.io> wrote:
> > >
> > > > As reviewers we should also feel empowered to request additional
> > > reviewers
> > > > on a PR (perhaps beyond whomever the original submitter may already
> > have
> > > > requested).
> > > >
> > > > I think that, sometimes the complexity of a change prevents someone
> > from
> > > > commenting on just a portion of the change if they do not feel
> > > comfortable
> > > > understanding the scope of the whole change.
> > > >
> > > > Having said that though, once you have 'touched' a PR you should also
> > be
> > > > tracking the PR for additional commits or feedback until it is
> merged.
> > > >
> > > > --Jens
> > > >
> > > > On Wed, Jun 5, 2019 at 11:37 AM Alexander Murmann <
> amurmann@pivotal.io
> > >
> > > > wrote:
> > > >
> > > > > >
> > > > > > If we trust committers, why review at all? Just commit... and we
> > > might
> > > > > > catch a problem, we might not.
> > > > >
> > > > > Honestly that to me would be the ideal state. However, our test
> > > coverage
> > > > > and code quality is nowhere near to allow for that.
> > > > >
> > > > > What I was referring to is different though. I didn't say "trust
> them
> > > to
> > > > > write perfect code", but trust " to decide how much review they
> > require
> > > > to
> > > > > feel comfortable". In some cases this might mean one review and in
> > > > others
> > > > > maybe two, three or even more and maybe even by very specific
> people.
> > > > >
> > > > > On Wed, Jun 5, 2019 at 11:31 AM Udo Kohlmeyer <ud...@apache.org>
> > wrote:
> > > > >
> > > > > > Alexander, thank you for your response. And yes, change is
> > > > uncomfortable
> > > > > > and in some cases more reviewers would not have caught issues.
> BUT,
> > > > more
> > > > > > people would have seen the code, maybe become more familiar with
> > it,
> > > > > etc...
> > > > > >
> > > > > > I don't say don't trust committers, as I am one. But I also know
> > > that I
> > > > > > mistakes are made regardless of intent. If we trust committers,
> why
> > > > > > review at all? Just commit... and we might catch a problem, we
> > might
> > > > not.
> > > > > >
> > > > > > --Udo
> > > > > >
> > > > > > On 6/5/19 11:20, Alexander Murmann wrote:
> > > > > > > Udo, I agree with many of the pains you are addressing, but am
> > > > > > pessimistic
> > > > > > > that having more reviewers will solve them.
> > > > > > >
> > > > > > > You are absolutely correct in calling out that the code is ugly
> > > > complex
> > > > > > and
> > > > > > > missing coverage. The best way to address this is to clean up
> the
> > > > code
> > > > > > and
> > > > > > > improve coverage. You say yourself "In the past single small
> > > changes
> > > > > have
> > > > > > > caused failures the were completely unforeseen by anyone". I
> > don't
> > > > > think
> > > > > > > more eyeballs will go a long way in making someone see complex
> > bugs
> > > > > > > introduced by seemingly safe changes.
> > > > > > >
> > > > > > > I also am concerned that introducing a hurdle like this will
> make
> > > > > > > committers more excited to review PRs with care, but rather
> might
> > > > lead
> > > > > to
> > > > > > > less care. It would be great of our committers were more
> > > passionate
> > > > > > about
> > > > > > > PR reviews and do them more often, but forcing it rarely
> > > accomplishes
> > > > > > that
> > > > > > > goal.
> > > > > > >
> > > > > > > I'd rather see us trust our committers to decide how much
> review
> > > they
> > > > > > > require to feel comfortable about their work and use the time
> > saved
> > > > to
> > > > > > > address the root of the problem (accidental complexity & lack
> of
> > > test
> > > > > > > coverage)
> > > > > > >
> > > > > > > On Wed, Jun 5, 2019 at 11:03 AM Udo Kohlmeyer <ud...@apache.org>
> > > > wrote:
> > > > > > >
> > > > > > >> @Kirk, I totally understand the pain that you speak of. I too
> > > agree
> > > > > that
> > > > > > >> every line of changed code should have a test confirming that
> > > > behavior
> > > > > > >> was not changed.
> > > > > > >>
> > > > > > >> I don't believe that we need to go as far as revoking
> committer
> > > > rights
> > > > > > >> and reviewing each committer again, BUT it would be AMAZING
> that
> > > out
> > > > > of
> > > > > > >> our 100 committers, 80% of them would be more active in PR
> > > reviews,
> > > > > > >> mailing lists and in the end active on the project outside of
> > > their
> > > > > > >> focus area.
> > > > > > >>
> > > > > > >> I do want to remind all Geode committers, it IS your
> > > responsibility
> > > > to
> > > > > > >> be part of the PR review cycle. I will hold myself just as
> > > > accountable
> > > > > > >> to this than what I hold every committer to, as I've been just
> > as
> > > > lazy
> > > > > > >> as the rest of them.
> > > > > > >>
> > > > > > >> BUT
> > > > > > >>
> > > > > > >> The reality is:
> > > > > > >>
> > > > > > >> 1. Geode code is HUGELY complex and NOT a test complete as
> > we'd
> > > > like
> > > > > > >> 2. In the past single small changes have caused failures the
> > > were
> > > > > > >> completely unforeseen by anyone
> > > > > > >> 3. In the past commits with single reviewers, have caused
> > > backward
> > > > > > >> compatibility issues which were only caught by chance in
> > > > > unrelated
> > > > > > >> testing.
> > > > > > >> 4. There are 100 committers on Geode, and we keep on arguing
> > > that
> > > > it
> > > > > > is
> > > > > > >> hard to get PR's reviewed and that is why it is ok to
> have
> > > > only 1
> > > > > > >> reviewer per PR.
> > > > > > >> 5. There seems to be majority (unstated) opinion of: "why
> > > change,
> > > > it
> > > > > > >> has been working for us so far." (I call is unstated,
> > because
> > > > > being
> > > > > > >> silent means you agree with the status quo)
> > > > > > >> 6. With requiring only 1 reviewer on code submissions, we
> are
> > > > > possibly
> > > > > > >> creating areas of the code only understood by a few.
> > > > > > >>
> > > > > > >> IF, we as a project, have decided that all code shall enter
> only
> > > > > through
> > > > > > >> the flow of PR, then why not extend the QA cycle a little by
> > > > requiring
> > > > > > >> more eyes. Lazy consensus, is as stated, lazy and would only
> > work
> > > > in a
> > > > > > >> project where the levels of complexity and size are not as
> high
> > as
> > > > > > >> Geode's. In addition, with PR submissions, we have admitted
> that
> > > we
> > > > > are
> > > > > > >> human and could make mistakes and in an already complex
> > > environment
> > > > > and
> > > > > > >> to the best of our ability get it wrong.
> > > > > > >>
> > > > > > >> Now, there are commits that really do not require 3 pairs of
> > eyes,
> > > > > > >> because spelling mistakes and typos don't need consensus. But
> > any
> > > > time
> > > > > > >> code logic was amended, this needs to be reviewed.
> > > > > > >>
> > > > > > >> I have seen different approach to code submissions:
> > > > > > >>
> > > > > > >> * The submitter of the PR is NOT the committer of the PR.
> > This
> > > > task
> > > > > > is
> > > > > > >> the responsibility of another committer(s) to review,
> > approve
> > > > and
> > > > > > >> finally merge in.
> > > > > > >> * Smaller amount of committers with higher numbers of
> > > > contributors.
> > > > > > >> Yes, this does create a bottleneck, but it promotes a
> sense
> > > of
> > > > > > pride
> > > > > > >> and responsibility that individual feels. Possibly a
> > greater
> > > > > > >> understanding of the target module is promoted through
> this
> > > > > > approach
> > > > > > >> as well.
> > > > > > >>
> > > > > > >> Now, I don't say we as a project should follow these strict or
> > > > > > >> restricting approaches, but from my perspective, if we as a
> > > project
> > > > > > >> argue that we struggle to find 3 reviewers out of 100, then
> > there
> > > > are
> > > > > > >> bigger problems in the project than we anticipated. It is not
> a
> > > lack
> > > > > of
> > > > > > >> trust in our committers, to me it is a sense of pride that I
> > want
> > > > > other
> > > > > > >> committers to confirm that I've delivered code to the high
> > > standard
> > > > > that
> > > > > > >> we want to be known for. Whilst it is painful to go through
> the
> > > > > process,
> > > > > > >> but if done correctly it is beneficial to all involved, as
> > > differing
> > > > > > >> opinions and approaches can be shared and all can learn from.
> > > > > > >>
> > > > > > >> In addition, I have personally stumbled upon a few PR's, which
> > > upon
> > > > > > >> review found to be lacking in the areas of best practices of
> > code
> > > > > and/or
> > > > > > >> design.
> > > > > > >>
> > > > > > >> I fully support the notion of 3 reviewers per PR. I'm also
> going
> > > to
> > > > > take
> > > > > > >> it one step further, in the list of reviewers, there is at
> > least 1
> > > > > > >> reviewer that is not part of a team, as this might drive a
> > > unbiased
> > > > > view
> > > > > > >> of the code and approach. I would also like to encourage ALL
> > > > > committers
> > > > > > >> to review code outside of the focus area. This will only
> > promote a
> > > > > > >> broader understanding of the project codebase. I also support
> > the
> > > > > notion
> > > > > > >> of a pair/mobbing reviews, if a reviewer does not understand
> the
> > > > > problem
> > > > > > >> area enough to effectively review, OR where the solution is
> not
> > > > > > apparent.
> > > > > > >>
> > > > > > >> --Udo
> > > > > > >>
> > > > > > >> On 6/4/19 16:49, Kirk Lund wrote:
> > > > > > >>> I'm -1 for requiring N reviews before merging a commit.
> > > > > > >>>
> > > > > > >>> Overall, I support Lazy Consensus. If I post a PR that fixes
> > the
> > > > > > >> flakiness
> > > > > > >>> in a test, the precheckin jobs prove it, and it sits there
> for
> > 2
> > > > > weeks
> > > > > > >>> without reviews, then I favor merging it in at that point
> > without
> > > > any
> > > > > > >>> reviews. I'm not going to chase people around or spam the dev
> > > list
> > > > > over
> > > > > > >> and
> > > > > > >>> over asking for reviews. Nothing in the Apache Way says you
> > have
> > > to
> > > > > do
> > > > > > >>> reviews before committing -- some projects prefer "commit
> then
> > > > > review"
> > > > > > >>> instead of "review then commit". You can always look at the
> > code
> > > > > > someone
> > > > > > >>> changed and you can always change it further or revert it.
> > > > > > >>>
> > > > > > >>> I think if we don't trust our committers then we have a
> bigger
> > > > > systemic
> > > > > > >>> problem that becoming more strict about PR reviews isn not
> > going
> > > to
> > > > > > fix.
> > > > > > >>>
> > > > > > >>> Overall, I also favor pairing/mobbing over reviews. Without
> > being
> > > > > there
> > > > > > >>> during the work, a reviewer lacks the context to understand
> why
> > > it
> > > > > was
> > > > > > >> done
> > > > > > >>> the way it was done.
> > > > > > >>>
> > > > > > >>> If we cannot establish or maintain trust in committers, then
> I
> > > > think
> > > > > we
> > > > > > >>> should remove committer status from everyone and start over
> as
> > a
> > > > > > project,
> > > > > > >>> proposing and accepting one committer at a time.
> > > > > > >>>
> > > > > > >>> Instead of constraints on reviews, I would prefer to
> establish
> > > new
> > > > > > >> criteria
> > > > > > >>> for coding such as:
> > > > > > >>> 1) all classes touched in a PR must have a unit test created
> if
> > > > none
> > > > > > >> exists
> > > > > > >>> 2) all code touched in a PR must have unit test coverage (and
> > > > > possibly
> > > > > > >>> integration test coverage) specific to the changes
> > > > > > >>> 3) all new classes must have full unit test coverage
> > > > > > >>> 4) all code touched in a PR must follow clean code principles
> > > > (which
> > > > > > >> would
> > > > > > >>> obviously need defining on the wiki)
> > > > > > >>>
> > > > > > >>> Then it becomes the responsibility of the author(s) and
> > > > committer(s)
> > > > > of
> > > > > > >>> that PR to ensure that the code and the PR follows the
> > project's
> > > > > > criteria
> > > > > > >>> for code quality and test coverage. It also becomes easier to
> > > > measure
> > > > > > the
> > > > > > >>> PRs of a non-committer to determine if we think they would
> > make a
> > > > > good
> > > > > > >>> committer (for example, do they adhere to clean code quality
> > and
> > > > unit
> > > > > > >>> testing with mocks? -- along with any other criteria).
> > > > > > >>>
> > > > > > >>> On Thu, May 30, 2019 at 3:51 PM Owen Nichols <
> > > onichols@pivotal.io>
> > > > > > >> wrote:
> > > > > > >>>> It seems common for Geode PRs to get merged with only a
> single
> > > > green
> > > > > > >>>> checkmark in GitHub.
> > > > > > >>>>
> > > > > > >>>> According to https://www.apache.org/foundation/voting.html
> we
> > > > > should
> > > > > > >> not
> > > > > > >>>> be merging PRs with fewer than 3 green checkmarks.
> > > > > > >>>>
> > > > > > >>>> Consensus is a fundamental value in doing things The Apache
> > Way.
> > > > A
> > > > > > >> single
> > > > > > >>>> +1 is not consensus. Since we’re currently discussing what
> it
> > > > takes
> > > > > > to
> > > > > > >>>> become a committer and what standards a committer is
> expected
> > to
> > > > > > >> uphold, it
> > > > > > >>>> seems like a good time to review this policy.
> > > > > > >>>>
> > > > > > >>>> GitHub can be configured to require N reviews before a
> commit
> > > can
> > > > be
> > > > > > >>>> merged. Should we enable this feature?
> > > > > > >>>>
> > > > > > >>>> -Owen
> > > > > > >>>> VOTES ON CODE MODIFICATION <
> > > > > > >>>>
> > > > > > >>
> > > > > >
> > > >
> > https://www.apache.org/foundation/voting.html#votes-on-code-modification
> > > > > >
> > > > > > >>>> For code-modification votes, +1 votes are in favour of the
> > > > proposal,
> > > > > > but
> > > > > > >>>> -1 votes are vetos <
> > > > > > https://www.apache.org/foundation/voting.html#Veto>
> > > > > > >>>> and kill the proposal dead until all vetoers withdraw their
> -1
> > > > > votes.
> > > > > > >>>>
> > > > > > >>>> Unless a vote has been declared as using lazy consensus <
> > > > > > >>>> https://www.apache.org/foundation/voting.html#LazyConsensus
> >
> > ,
> > > > > three
> > > > > > +1
> > > > > > >>>> votes are required for a code-modification proposal to pass.
> > > > > > >>>>
> > > > > > >>>> Whole numbers are recommended for this type of vote, as the
> > > > opinion
> > > > > > >> being
> > > > > > >>>> expressed is Boolean: 'I approve/do not approve of this
> > change.'
> > > > > > >>>>
> > > > > > >>>>
> > > > > > >>>> CONSENSUS GAUGING THROUGH SILENCE <
> > > > > > >>>> https://www.apache.org/foundation/voting.html#LazyConsensus
> >
> > > > > > >>>> An alternative to voting that is sometimes used to measure
> the
> > > > > > >>>> acceptability of something is the concept of lazy consensus
> <
> > > > > > >>>>
> https://www.apache.org/foundation/glossary.html#LazyConsensus
> > >.
> > > > > > >>>>
> > > > > > >>>> Lazy consensus is simply an announcement of 'silence gives
> > > > assent.’
> > > > > > When
> > > > > > >>>> someone wants to determine the sense of the community this
> > way,
> > > it
> > > > > > >> might do
> > > > > > >>>> so with a mail message such as:
> > > > > > >>>> "The patch below fixes GEODE-12345; if no-one objects within
> > > three
> > > > > > days,
> > > > > > >>>> I'll assume lazy consensus and commit it."
> > > > > > >>>>
> > > > > > >>>> Lazy consensus cannot be used on projects that enforce a
> > > > > > >>>> review-then-commit <
> > > > > > >>>>
> > > https://www.apache.org/foundation/glossary.html#ReviewThenCommit>
> > > > > > >> policy.
> > > > > > >>>>
> > > > > > >>>>
> > > > > >
> > > > >
> > > >
> > >
> > >
> > > --
> > > *Joris Melchior *
> > > CF Engineering
> > > Pivotal Toronto
> > > 416 877 5427
> > >
> > > “Programs must be written for people to read, and only incidentally for
> > > machines to execute.” – *Hal Abelson*
> > > <https://en.wikipedia.org/wiki/Hal_Abelson>
> > >
> >
>
--
*Joris Melchior *
CF Engineering
Pivotal Toronto
416 877 5427
“Programs must be written for people to read, and only incidentally for
machines to execute.” – *Hal Abelson*
<https://en.wikipedia.org/wiki/Hal_Abelson>
Re: [DISCUSS] require reviews before merging a PR
Posted by Patrick Rhomberg <pr...@apache.org>.
>
> I'm not sure it's possible to do this with the "reviewers" field - if
> someone can figure out how to do this with the github IU, we can at least
> try filing an ticket with apache infrastructure.
>
According to their docs [1], "collaborator with write access" is the GitHub
required criteria to add reviewers. So, yeah, you have to be a committer.
If you're not a committer, you could always request in your PR comments for
a committer to add some specific people, should you have any in mind.
Imagination is Change.
~Patrick
[1] https://help.github.com/en/articles/requesting-a-pull-request-review
On Thu, Jun 6, 2019 at 9:26 AM Dan Smith <ds...@pivotal.io> wrote:
> >
> > Would it be possible to allow people who do not have committer status to
> > request reviewers on a pull request.
>
>
> I'm not sure it's possible to do this with the "reviewers" field - if
> someone can figure out how to do this with the github IU, we can at least
> try filing an ticket with apache infrastructure.
>
> In the meantime, you can also just add a comment mentioning the people
> you'd like to review the request. That works for requesting reviews from
> non-committers as well.
>
> -Dan
>
> On Thu, Jun 6, 2019 at 9:05 AM Joris Melchior <jm...@pivotal.io>
> wrote:
>
> > Would it be possible to allow people who do not have committer status to
> > request reviewers on a pull request. In some cases we may know who should
> > take a look at it and in that case making it official by adding these
> > people to the pull request would be good IMO.
> >
> > On Thu, Jun 6, 2019 at 10:26 AM Jens Deppe <jd...@pivotal.io> wrote:
> >
> > > As reviewers we should also feel empowered to request additional
> > reviewers
> > > on a PR (perhaps beyond whomever the original submitter may already
> have
> > > requested).
> > >
> > > I think that, sometimes the complexity of a change prevents someone
> from
> > > commenting on just a portion of the change if they do not feel
> > comfortable
> > > understanding the scope of the whole change.
> > >
> > > Having said that though, once you have 'touched' a PR you should also
> be
> > > tracking the PR for additional commits or feedback until it is merged.
> > >
> > > --Jens
> > >
> > > On Wed, Jun 5, 2019 at 11:37 AM Alexander Murmann <amurmann@pivotal.io
> >
> > > wrote:
> > >
> > > > >
> > > > > If we trust committers, why review at all? Just commit... and we
> > might
> > > > > catch a problem, we might not.
> > > >
> > > > Honestly that to me would be the ideal state. However, our test
> > coverage
> > > > and code quality is nowhere near to allow for that.
> > > >
> > > > What I was referring to is different though. I didn't say "trust them
> > to
> > > > write perfect code", but trust " to decide how much review they
> require
> > > to
> > > > feel comfortable". In some cases this might mean one review and in
> > > others
> > > > maybe two, three or even more and maybe even by very specific people.
> > > >
> > > > On Wed, Jun 5, 2019 at 11:31 AM Udo Kohlmeyer <ud...@apache.org>
> wrote:
> > > >
> > > > > Alexander, thank you for your response. And yes, change is
> > > uncomfortable
> > > > > and in some cases more reviewers would not have caught issues. BUT,
> > > more
> > > > > people would have seen the code, maybe become more familiar with
> it,
> > > > etc...
> > > > >
> > > > > I don't say don't trust committers, as I am one. But I also know
> > that I
> > > > > mistakes are made regardless of intent. If we trust committers, why
> > > > > review at all? Just commit... and we might catch a problem, we
> might
> > > not.
> > > > >
> > > > > --Udo
> > > > >
> > > > > On 6/5/19 11:20, Alexander Murmann wrote:
> > > > > > Udo, I agree with many of the pains you are addressing, but am
> > > > > pessimistic
> > > > > > that having more reviewers will solve them.
> > > > > >
> > > > > > You are absolutely correct in calling out that the code is ugly
> > > complex
> > > > > and
> > > > > > missing coverage. The best way to address this is to clean up the
> > > code
> > > > > and
> > > > > > improve coverage. You say yourself "In the past single small
> > changes
> > > > have
> > > > > > caused failures the were completely unforeseen by anyone". I
> don't
> > > > think
> > > > > > more eyeballs will go a long way in making someone see complex
> bugs
> > > > > > introduced by seemingly safe changes.
> > > > > >
> > > > > > I also am concerned that introducing a hurdle like this will make
> > > > > > committers more excited to review PRs with care, but rather might
> > > lead
> > > > to
> > > > > > less care. It would be great of our committers were more
> > passionate
> > > > > about
> > > > > > PR reviews and do them more often, but forcing it rarely
> > accomplishes
> > > > > that
> > > > > > goal.
> > > > > >
> > > > > > I'd rather see us trust our committers to decide how much review
> > they
> > > > > > require to feel comfortable about their work and use the time
> saved
> > > to
> > > > > > address the root of the problem (accidental complexity & lack of
> > test
> > > > > > coverage)
> > > > > >
> > > > > > On Wed, Jun 5, 2019 at 11:03 AM Udo Kohlmeyer <ud...@apache.org>
> > > wrote:
> > > > > >
> > > > > >> @Kirk, I totally understand the pain that you speak of. I too
> > agree
> > > > that
> > > > > >> every line of changed code should have a test confirming that
> > > behavior
> > > > > >> was not changed.
> > > > > >>
> > > > > >> I don't believe that we need to go as far as revoking committer
> > > rights
> > > > > >> and reviewing each committer again, BUT it would be AMAZING that
> > out
> > > > of
> > > > > >> our 100 committers, 80% of them would be more active in PR
> > reviews,
> > > > > >> mailing lists and in the end active on the project outside of
> > their
> > > > > >> focus area.
> > > > > >>
> > > > > >> I do want to remind all Geode committers, it IS your
> > responsibility
> > > to
> > > > > >> be part of the PR review cycle. I will hold myself just as
> > > accountable
> > > > > >> to this than what I hold every committer to, as I've been just
> as
> > > lazy
> > > > > >> as the rest of them.
> > > > > >>
> > > > > >> BUT
> > > > > >>
> > > > > >> The reality is:
> > > > > >>
> > > > > >> 1. Geode code is HUGELY complex and NOT a test complete as
> we'd
> > > like
> > > > > >> 2. In the past single small changes have caused failures the
> > were
> > > > > >> completely unforeseen by anyone
> > > > > >> 3. In the past commits with single reviewers, have caused
> > backward
> > > > > >> compatibility issues which were only caught by chance in
> > > > unrelated
> > > > > >> testing.
> > > > > >> 4. There are 100 committers on Geode, and we keep on arguing
> > that
> > > it
> > > > > is
> > > > > >> hard to get PR's reviewed and that is why it is ok to have
> > > only 1
> > > > > >> reviewer per PR.
> > > > > >> 5. There seems to be majority (unstated) opinion of: "why
> > change,
> > > it
> > > > > >> has been working for us so far." (I call is unstated,
> because
> > > > being
> > > > > >> silent means you agree with the status quo)
> > > > > >> 6. With requiring only 1 reviewer on code submissions, we are
> > > > possibly
> > > > > >> creating areas of the code only understood by a few.
> > > > > >>
> > > > > >> IF, we as a project, have decided that all code shall enter only
> > > > through
> > > > > >> the flow of PR, then why not extend the QA cycle a little by
> > > requiring
> > > > > >> more eyes. Lazy consensus, is as stated, lazy and would only
> work
> > > in a
> > > > > >> project where the levels of complexity and size are not as high
> as
> > > > > >> Geode's. In addition, with PR submissions, we have admitted that
> > we
> > > > are
> > > > > >> human and could make mistakes and in an already complex
> > environment
> > > > and
> > > > > >> to the best of our ability get it wrong.
> > > > > >>
> > > > > >> Now, there are commits that really do not require 3 pairs of
> eyes,
> > > > > >> because spelling mistakes and typos don't need consensus. But
> any
> > > time
> > > > > >> code logic was amended, this needs to be reviewed.
> > > > > >>
> > > > > >> I have seen different approach to code submissions:
> > > > > >>
> > > > > >> * The submitter of the PR is NOT the committer of the PR.
> This
> > > task
> > > > > is
> > > > > >> the responsibility of another committer(s) to review,
> approve
> > > and
> > > > > >> finally merge in.
> > > > > >> * Smaller amount of committers with higher numbers of
> > > contributors.
> > > > > >> Yes, this does create a bottleneck, but it promotes a sense
> > of
> > > > > pride
> > > > > >> and responsibility that individual feels. Possibly a
> greater
> > > > > >> understanding of the target module is promoted through this
> > > > > approach
> > > > > >> as well.
> > > > > >>
> > > > > >> Now, I don't say we as a project should follow these strict or
> > > > > >> restricting approaches, but from my perspective, if we as a
> > project
> > > > > >> argue that we struggle to find 3 reviewers out of 100, then
> there
> > > are
> > > > > >> bigger problems in the project than we anticipated. It is not a
> > lack
> > > > of
> > > > > >> trust in our committers, to me it is a sense of pride that I
> want
> > > > other
> > > > > >> committers to confirm that I've delivered code to the high
> > standard
> > > > that
> > > > > >> we want to be known for. Whilst it is painful to go through the
> > > > process,
> > > > > >> but if done correctly it is beneficial to all involved, as
> > differing
> > > > > >> opinions and approaches can be shared and all can learn from.
> > > > > >>
> > > > > >> In addition, I have personally stumbled upon a few PR's, which
> > upon
> > > > > >> review found to be lacking in the areas of best practices of
> code
> > > > and/or
> > > > > >> design.
> > > > > >>
> > > > > >> I fully support the notion of 3 reviewers per PR. I'm also going
> > to
> > > > take
> > > > > >> it one step further, in the list of reviewers, there is at
> least 1
> > > > > >> reviewer that is not part of a team, as this might drive a
> > unbiased
> > > > view
> > > > > >> of the code and approach. I would also like to encourage ALL
> > > > committers
> > > > > >> to review code outside of the focus area. This will only
> promote a
> > > > > >> broader understanding of the project codebase. I also support
> the
> > > > notion
> > > > > >> of a pair/mobbing reviews, if a reviewer does not understand the
> > > > problem
> > > > > >> area enough to effectively review, OR where the solution is not
> > > > > apparent.
> > > > > >>
> > > > > >> --Udo
> > > > > >>
> > > > > >> On 6/4/19 16:49, Kirk Lund wrote:
> > > > > >>> I'm -1 for requiring N reviews before merging a commit.
> > > > > >>>
> > > > > >>> Overall, I support Lazy Consensus. If I post a PR that fixes
> the
> > > > > >> flakiness
> > > > > >>> in a test, the precheckin jobs prove it, and it sits there for
> 2
> > > > weeks
> > > > > >>> without reviews, then I favor merging it in at that point
> without
> > > any
> > > > > >>> reviews. I'm not going to chase people around or spam the dev
> > list
> > > > over
> > > > > >> and
> > > > > >>> over asking for reviews. Nothing in the Apache Way says you
> have
> > to
> > > > do
> > > > > >>> reviews before committing -- some projects prefer "commit then
> > > > review"
> > > > > >>> instead of "review then commit". You can always look at the
> code
> > > > > someone
> > > > > >>> changed and you can always change it further or revert it.
> > > > > >>>
> > > > > >>> I think if we don't trust our committers then we have a bigger
> > > > systemic
> > > > > >>> problem that becoming more strict about PR reviews isn not
> going
> > to
> > > > > fix.
> > > > > >>>
> > > > > >>> Overall, I also favor pairing/mobbing over reviews. Without
> being
> > > > there
> > > > > >>> during the work, a reviewer lacks the context to understand why
> > it
> > > > was
> > > > > >> done
> > > > > >>> the way it was done.
> > > > > >>>
> > > > > >>> If we cannot establish or maintain trust in committers, then I
> > > think
> > > > we
> > > > > >>> should remove committer status from everyone and start over as
> a
> > > > > project,
> > > > > >>> proposing and accepting one committer at a time.
> > > > > >>>
> > > > > >>> Instead of constraints on reviews, I would prefer to establish
> > new
> > > > > >> criteria
> > > > > >>> for coding such as:
> > > > > >>> 1) all classes touched in a PR must have a unit test created if
> > > none
> > > > > >> exists
> > > > > >>> 2) all code touched in a PR must have unit test coverage (and
> > > > possibly
> > > > > >>> integration test coverage) specific to the changes
> > > > > >>> 3) all new classes must have full unit test coverage
> > > > > >>> 4) all code touched in a PR must follow clean code principles
> > > (which
> > > > > >> would
> > > > > >>> obviously need defining on the wiki)
> > > > > >>>
> > > > > >>> Then it becomes the responsibility of the author(s) and
> > > committer(s)
> > > > of
> > > > > >>> that PR to ensure that the code and the PR follows the
> project's
> > > > > criteria
> > > > > >>> for code quality and test coverage. It also becomes easier to
> > > measure
> > > > > the
> > > > > >>> PRs of a non-committer to determine if we think they would
> make a
> > > > good
> > > > > >>> committer (for example, do they adhere to clean code quality
> and
> > > unit
> > > > > >>> testing with mocks? -- along with any other criteria).
> > > > > >>>
> > > > > >>> On Thu, May 30, 2019 at 3:51 PM Owen Nichols <
> > onichols@pivotal.io>
> > > > > >> wrote:
> > > > > >>>> It seems common for Geode PRs to get merged with only a single
> > > green
> > > > > >>>> checkmark in GitHub.
> > > > > >>>>
> > > > > >>>> According to https://www.apache.org/foundation/voting.html we
> > > > should
> > > > > >> not
> > > > > >>>> be merging PRs with fewer than 3 green checkmarks.
> > > > > >>>>
> > > > > >>>> Consensus is a fundamental value in doing things The Apache
> Way.
> > > A
> > > > > >> single
> > > > > >>>> +1 is not consensus. Since we’re currently discussing what it
> > > takes
> > > > > to
> > > > > >>>> become a committer and what standards a committer is expected
> to
> > > > > >> uphold, it
> > > > > >>>> seems like a good time to review this policy.
> > > > > >>>>
> > > > > >>>> GitHub can be configured to require N reviews before a commit
> > can
> > > be
> > > > > >>>> merged. Should we enable this feature?
> > > > > >>>>
> > > > > >>>> -Owen
> > > > > >>>> VOTES ON CODE MODIFICATION <
> > > > > >>>>
> > > > > >>
> > > > >
> > >
> https://www.apache.org/foundation/voting.html#votes-on-code-modification
> > > > >
> > > > > >>>> For code-modification votes, +1 votes are in favour of the
> > > proposal,
> > > > > but
> > > > > >>>> -1 votes are vetos <
> > > > > https://www.apache.org/foundation/voting.html#Veto>
> > > > > >>>> and kill the proposal dead until all vetoers withdraw their -1
> > > > votes.
> > > > > >>>>
> > > > > >>>> Unless a vote has been declared as using lazy consensus <
> > > > > >>>> https://www.apache.org/foundation/voting.html#LazyConsensus>
> ,
> > > > three
> > > > > +1
> > > > > >>>> votes are required for a code-modification proposal to pass.
> > > > > >>>>
> > > > > >>>> Whole numbers are recommended for this type of vote, as the
> > > opinion
> > > > > >> being
> > > > > >>>> expressed is Boolean: 'I approve/do not approve of this
> change.'
> > > > > >>>>
> > > > > >>>>
> > > > > >>>> CONSENSUS GAUGING THROUGH SILENCE <
> > > > > >>>> https://www.apache.org/foundation/voting.html#LazyConsensus>
> > > > > >>>> An alternative to voting that is sometimes used to measure the
> > > > > >>>> acceptability of something is the concept of lazy consensus <
> > > > > >>>> https://www.apache.org/foundation/glossary.html#LazyConsensus
> >.
> > > > > >>>>
> > > > > >>>> Lazy consensus is simply an announcement of 'silence gives
> > > assent.’
> > > > > When
> > > > > >>>> someone wants to determine the sense of the community this
> way,
> > it
> > > > > >> might do
> > > > > >>>> so with a mail message such as:
> > > > > >>>> "The patch below fixes GEODE-12345; if no-one objects within
> > three
> > > > > days,
> > > > > >>>> I'll assume lazy consensus and commit it."
> > > > > >>>>
> > > > > >>>> Lazy consensus cannot be used on projects that enforce a
> > > > > >>>> review-then-commit <
> > > > > >>>>
> > https://www.apache.org/foundation/glossary.html#ReviewThenCommit>
> > > > > >> policy.
> > > > > >>>>
> > > > > >>>>
> > > > >
> > > >
> > >
> >
> >
> > --
> > *Joris Melchior *
> > CF Engineering
> > Pivotal Toronto
> > 416 877 5427
> >
> > “Programs must be written for people to read, and only incidentally for
> > machines to execute.” – *Hal Abelson*
> > <https://en.wikipedia.org/wiki/Hal_Abelson>
> >
>
Re: [DISCUSS] require reviews before merging a PR
Posted by Dan Smith <ds...@pivotal.io>.
>
> Would it be possible to allow people who do not have committer status to
> request reviewers on a pull request.
I'm not sure it's possible to do this with the "reviewers" field - if
someone can figure out how to do this with the github IU, we can at least
try filing an ticket with apache infrastructure.
In the meantime, you can also just add a comment mentioning the people
you'd like to review the request. That works for requesting reviews from
non-committers as well.
-Dan
On Thu, Jun 6, 2019 at 9:05 AM Joris Melchior <jm...@pivotal.io> wrote:
> Would it be possible to allow people who do not have committer status to
> request reviewers on a pull request. In some cases we may know who should
> take a look at it and in that case making it official by adding these
> people to the pull request would be good IMO.
>
> On Thu, Jun 6, 2019 at 10:26 AM Jens Deppe <jd...@pivotal.io> wrote:
>
> > As reviewers we should also feel empowered to request additional
> reviewers
> > on a PR (perhaps beyond whomever the original submitter may already have
> > requested).
> >
> > I think that, sometimes the complexity of a change prevents someone from
> > commenting on just a portion of the change if they do not feel
> comfortable
> > understanding the scope of the whole change.
> >
> > Having said that though, once you have 'touched' a PR you should also be
> > tracking the PR for additional commits or feedback until it is merged.
> >
> > --Jens
> >
> > On Wed, Jun 5, 2019 at 11:37 AM Alexander Murmann <am...@pivotal.io>
> > wrote:
> >
> > > >
> > > > If we trust committers, why review at all? Just commit... and we
> might
> > > > catch a problem, we might not.
> > >
> > > Honestly that to me would be the ideal state. However, our test
> coverage
> > > and code quality is nowhere near to allow for that.
> > >
> > > What I was referring to is different though. I didn't say "trust them
> to
> > > write perfect code", but trust " to decide how much review they require
> > to
> > > feel comfortable". In some cases this might mean one review and in
> > others
> > > maybe two, three or even more and maybe even by very specific people.
> > >
> > > On Wed, Jun 5, 2019 at 11:31 AM Udo Kohlmeyer <ud...@apache.org> wrote:
> > >
> > > > Alexander, thank you for your response. And yes, change is
> > uncomfortable
> > > > and in some cases more reviewers would not have caught issues. BUT,
> > more
> > > > people would have seen the code, maybe become more familiar with it,
> > > etc...
> > > >
> > > > I don't say don't trust committers, as I am one. But I also know
> that I
> > > > mistakes are made regardless of intent. If we trust committers, why
> > > > review at all? Just commit... and we might catch a problem, we might
> > not.
> > > >
> > > > --Udo
> > > >
> > > > On 6/5/19 11:20, Alexander Murmann wrote:
> > > > > Udo, I agree with many of the pains you are addressing, but am
> > > > pessimistic
> > > > > that having more reviewers will solve them.
> > > > >
> > > > > You are absolutely correct in calling out that the code is ugly
> > complex
> > > > and
> > > > > missing coverage. The best way to address this is to clean up the
> > code
> > > > and
> > > > > improve coverage. You say yourself "In the past single small
> changes
> > > have
> > > > > caused failures the were completely unforeseen by anyone". I don't
> > > think
> > > > > more eyeballs will go a long way in making someone see complex bugs
> > > > > introduced by seemingly safe changes.
> > > > >
> > > > > I also am concerned that introducing a hurdle like this will make
> > > > > committers more excited to review PRs with care, but rather might
> > lead
> > > to
> > > > > less care. It would be great of our committers were more
> passionate
> > > > about
> > > > > PR reviews and do them more often, but forcing it rarely
> accomplishes
> > > > that
> > > > > goal.
> > > > >
> > > > > I'd rather see us trust our committers to decide how much review
> they
> > > > > require to feel comfortable about their work and use the time saved
> > to
> > > > > address the root of the problem (accidental complexity & lack of
> test
> > > > > coverage)
> > > > >
> > > > > On Wed, Jun 5, 2019 at 11:03 AM Udo Kohlmeyer <ud...@apache.org>
> > wrote:
> > > > >
> > > > >> @Kirk, I totally understand the pain that you speak of. I too
> agree
> > > that
> > > > >> every line of changed code should have a test confirming that
> > behavior
> > > > >> was not changed.
> > > > >>
> > > > >> I don't believe that we need to go as far as revoking committer
> > rights
> > > > >> and reviewing each committer again, BUT it would be AMAZING that
> out
> > > of
> > > > >> our 100 committers, 80% of them would be more active in PR
> reviews,
> > > > >> mailing lists and in the end active on the project outside of
> their
> > > > >> focus area.
> > > > >>
> > > > >> I do want to remind all Geode committers, it IS your
> responsibility
> > to
> > > > >> be part of the PR review cycle. I will hold myself just as
> > accountable
> > > > >> to this than what I hold every committer to, as I've been just as
> > lazy
> > > > >> as the rest of them.
> > > > >>
> > > > >> BUT
> > > > >>
> > > > >> The reality is:
> > > > >>
> > > > >> 1. Geode code is HUGELY complex and NOT a test complete as we'd
> > like
> > > > >> 2. In the past single small changes have caused failures the
> were
> > > > >> completely unforeseen by anyone
> > > > >> 3. In the past commits with single reviewers, have caused
> backward
> > > > >> compatibility issues which were only caught by chance in
> > > unrelated
> > > > >> testing.
> > > > >> 4. There are 100 committers on Geode, and we keep on arguing
> that
> > it
> > > > is
> > > > >> hard to get PR's reviewed and that is why it is ok to have
> > only 1
> > > > >> reviewer per PR.
> > > > >> 5. There seems to be majority (unstated) opinion of: "why
> change,
> > it
> > > > >> has been working for us so far." (I call is unstated, because
> > > being
> > > > >> silent means you agree with the status quo)
> > > > >> 6. With requiring only 1 reviewer on code submissions, we are
> > > possibly
> > > > >> creating areas of the code only understood by a few.
> > > > >>
> > > > >> IF, we as a project, have decided that all code shall enter only
> > > through
> > > > >> the flow of PR, then why not extend the QA cycle a little by
> > requiring
> > > > >> more eyes. Lazy consensus, is as stated, lazy and would only work
> > in a
> > > > >> project where the levels of complexity and size are not as high as
> > > > >> Geode's. In addition, with PR submissions, we have admitted that
> we
> > > are
> > > > >> human and could make mistakes and in an already complex
> environment
> > > and
> > > > >> to the best of our ability get it wrong.
> > > > >>
> > > > >> Now, there are commits that really do not require 3 pairs of eyes,
> > > > >> because spelling mistakes and typos don't need consensus. But any
> > time
> > > > >> code logic was amended, this needs to be reviewed.
> > > > >>
> > > > >> I have seen different approach to code submissions:
> > > > >>
> > > > >> * The submitter of the PR is NOT the committer of the PR. This
> > task
> > > > is
> > > > >> the responsibility of another committer(s) to review, approve
> > and
> > > > >> finally merge in.
> > > > >> * Smaller amount of committers with higher numbers of
> > contributors.
> > > > >> Yes, this does create a bottleneck, but it promotes a sense
> of
> > > > pride
> > > > >> and responsibility that individual feels. Possibly a greater
> > > > >> understanding of the target module is promoted through this
> > > > approach
> > > > >> as well.
> > > > >>
> > > > >> Now, I don't say we as a project should follow these strict or
> > > > >> restricting approaches, but from my perspective, if we as a
> project
> > > > >> argue that we struggle to find 3 reviewers out of 100, then there
> > are
> > > > >> bigger problems in the project than we anticipated. It is not a
> lack
> > > of
> > > > >> trust in our committers, to me it is a sense of pride that I want
> > > other
> > > > >> committers to confirm that I've delivered code to the high
> standard
> > > that
> > > > >> we want to be known for. Whilst it is painful to go through the
> > > process,
> > > > >> but if done correctly it is beneficial to all involved, as
> differing
> > > > >> opinions and approaches can be shared and all can learn from.
> > > > >>
> > > > >> In addition, I have personally stumbled upon a few PR's, which
> upon
> > > > >> review found to be lacking in the areas of best practices of code
> > > and/or
> > > > >> design.
> > > > >>
> > > > >> I fully support the notion of 3 reviewers per PR. I'm also going
> to
> > > take
> > > > >> it one step further, in the list of reviewers, there is at least 1
> > > > >> reviewer that is not part of a team, as this might drive a
> unbiased
> > > view
> > > > >> of the code and approach. I would also like to encourage ALL
> > > committers
> > > > >> to review code outside of the focus area. This will only promote a
> > > > >> broader understanding of the project codebase. I also support the
> > > notion
> > > > >> of a pair/mobbing reviews, if a reviewer does not understand the
> > > problem
> > > > >> area enough to effectively review, OR where the solution is not
> > > > apparent.
> > > > >>
> > > > >> --Udo
> > > > >>
> > > > >> On 6/4/19 16:49, Kirk Lund wrote:
> > > > >>> I'm -1 for requiring N reviews before merging a commit.
> > > > >>>
> > > > >>> Overall, I support Lazy Consensus. If I post a PR that fixes the
> > > > >> flakiness
> > > > >>> in a test, the precheckin jobs prove it, and it sits there for 2
> > > weeks
> > > > >>> without reviews, then I favor merging it in at that point without
> > any
> > > > >>> reviews. I'm not going to chase people around or spam the dev
> list
> > > over
> > > > >> and
> > > > >>> over asking for reviews. Nothing in the Apache Way says you have
> to
> > > do
> > > > >>> reviews before committing -- some projects prefer "commit then
> > > review"
> > > > >>> instead of "review then commit". You can always look at the code
> > > > someone
> > > > >>> changed and you can always change it further or revert it.
> > > > >>>
> > > > >>> I think if we don't trust our committers then we have a bigger
> > > systemic
> > > > >>> problem that becoming more strict about PR reviews isn not going
> to
> > > > fix.
> > > > >>>
> > > > >>> Overall, I also favor pairing/mobbing over reviews. Without being
> > > there
> > > > >>> during the work, a reviewer lacks the context to understand why
> it
> > > was
> > > > >> done
> > > > >>> the way it was done.
> > > > >>>
> > > > >>> If we cannot establish or maintain trust in committers, then I
> > think
> > > we
> > > > >>> should remove committer status from everyone and start over as a
> > > > project,
> > > > >>> proposing and accepting one committer at a time.
> > > > >>>
> > > > >>> Instead of constraints on reviews, I would prefer to establish
> new
> > > > >> criteria
> > > > >>> for coding such as:
> > > > >>> 1) all classes touched in a PR must have a unit test created if
> > none
> > > > >> exists
> > > > >>> 2) all code touched in a PR must have unit test coverage (and
> > > possibly
> > > > >>> integration test coverage) specific to the changes
> > > > >>> 3) all new classes must have full unit test coverage
> > > > >>> 4) all code touched in a PR must follow clean code principles
> > (which
> > > > >> would
> > > > >>> obviously need defining on the wiki)
> > > > >>>
> > > > >>> Then it becomes the responsibility of the author(s) and
> > committer(s)
> > > of
> > > > >>> that PR to ensure that the code and the PR follows the project's
> > > > criteria
> > > > >>> for code quality and test coverage. It also becomes easier to
> > measure
> > > > the
> > > > >>> PRs of a non-committer to determine if we think they would make a
> > > good
> > > > >>> committer (for example, do they adhere to clean code quality and
> > unit
> > > > >>> testing with mocks? -- along with any other criteria).
> > > > >>>
> > > > >>> On Thu, May 30, 2019 at 3:51 PM Owen Nichols <
> onichols@pivotal.io>
> > > > >> wrote:
> > > > >>>> It seems common for Geode PRs to get merged with only a single
> > green
> > > > >>>> checkmark in GitHub.
> > > > >>>>
> > > > >>>> According to https://www.apache.org/foundation/voting.html we
> > > should
> > > > >> not
> > > > >>>> be merging PRs with fewer than 3 green checkmarks.
> > > > >>>>
> > > > >>>> Consensus is a fundamental value in doing things The Apache Way.
> > A
> > > > >> single
> > > > >>>> +1 is not consensus. Since we’re currently discussing what it
> > takes
> > > > to
> > > > >>>> become a committer and what standards a committer is expected to
> > > > >> uphold, it
> > > > >>>> seems like a good time to review this policy.
> > > > >>>>
> > > > >>>> GitHub can be configured to require N reviews before a commit
> can
> > be
> > > > >>>> merged. Should we enable this feature?
> > > > >>>>
> > > > >>>> -Owen
> > > > >>>> VOTES ON CODE MODIFICATION <
> > > > >>>>
> > > > >>
> > > >
> > https://www.apache.org/foundation/voting.html#votes-on-code-modification
> > > >
> > > > >>>> For code-modification votes, +1 votes are in favour of the
> > proposal,
> > > > but
> > > > >>>> -1 votes are vetos <
> > > > https://www.apache.org/foundation/voting.html#Veto>
> > > > >>>> and kill the proposal dead until all vetoers withdraw their -1
> > > votes.
> > > > >>>>
> > > > >>>> Unless a vote has been declared as using lazy consensus <
> > > > >>>> https://www.apache.org/foundation/voting.html#LazyConsensus> ,
> > > three
> > > > +1
> > > > >>>> votes are required for a code-modification proposal to pass.
> > > > >>>>
> > > > >>>> Whole numbers are recommended for this type of vote, as the
> > opinion
> > > > >> being
> > > > >>>> expressed is Boolean: 'I approve/do not approve of this change.'
> > > > >>>>
> > > > >>>>
> > > > >>>> CONSENSUS GAUGING THROUGH SILENCE <
> > > > >>>> https://www.apache.org/foundation/voting.html#LazyConsensus>
> > > > >>>> An alternative to voting that is sometimes used to measure the
> > > > >>>> acceptability of something is the concept of lazy consensus <
> > > > >>>> https://www.apache.org/foundation/glossary.html#LazyConsensus>.
> > > > >>>>
> > > > >>>> Lazy consensus is simply an announcement of 'silence gives
> > assent.’
> > > > When
> > > > >>>> someone wants to determine the sense of the community this way,
> it
> > > > >> might do
> > > > >>>> so with a mail message such as:
> > > > >>>> "The patch below fixes GEODE-12345; if no-one objects within
> three
> > > > days,
> > > > >>>> I'll assume lazy consensus and commit it."
> > > > >>>>
> > > > >>>> Lazy consensus cannot be used on projects that enforce a
> > > > >>>> review-then-commit <
> > > > >>>>
> https://www.apache.org/foundation/glossary.html#ReviewThenCommit>
> > > > >> policy.
> > > > >>>>
> > > > >>>>
> > > >
> > >
> >
>
>
> --
> *Joris Melchior *
> CF Engineering
> Pivotal Toronto
> 416 877 5427
>
> “Programs must be written for people to read, and only incidentally for
> machines to execute.” – *Hal Abelson*
> <https://en.wikipedia.org/wiki/Hal_Abelson>
>
Re: [DISCUSS] require reviews before merging a PR
Posted by Joris Melchior <jm...@pivotal.io>.
Would it be possible to allow people who do not have committer status to
request reviewers on a pull request. In some cases we may know who should
take a look at it and in that case making it official by adding these
people to the pull request would be good IMO.
On Thu, Jun 6, 2019 at 10:26 AM Jens Deppe <jd...@pivotal.io> wrote:
> As reviewers we should also feel empowered to request additional reviewers
> on a PR (perhaps beyond whomever the original submitter may already have
> requested).
>
> I think that, sometimes the complexity of a change prevents someone from
> commenting on just a portion of the change if they do not feel comfortable
> understanding the scope of the whole change.
>
> Having said that though, once you have 'touched' a PR you should also be
> tracking the PR for additional commits or feedback until it is merged.
>
> --Jens
>
> On Wed, Jun 5, 2019 at 11:37 AM Alexander Murmann <am...@pivotal.io>
> wrote:
>
> > >
> > > If we trust committers, why review at all? Just commit... and we might
> > > catch a problem, we might not.
> >
> > Honestly that to me would be the ideal state. However, our test coverage
> > and code quality is nowhere near to allow for that.
> >
> > What I was referring to is different though. I didn't say "trust them to
> > write perfect code", but trust " to decide how much review they require
> to
> > feel comfortable". In some cases this might mean one review and in
> others
> > maybe two, three or even more and maybe even by very specific people.
> >
> > On Wed, Jun 5, 2019 at 11:31 AM Udo Kohlmeyer <ud...@apache.org> wrote:
> >
> > > Alexander, thank you for your response. And yes, change is
> uncomfortable
> > > and in some cases more reviewers would not have caught issues. BUT,
> more
> > > people would have seen the code, maybe become more familiar with it,
> > etc...
> > >
> > > I don't say don't trust committers, as I am one. But I also know that I
> > > mistakes are made regardless of intent. If we trust committers, why
> > > review at all? Just commit... and we might catch a problem, we might
> not.
> > >
> > > --Udo
> > >
> > > On 6/5/19 11:20, Alexander Murmann wrote:
> > > > Udo, I agree with many of the pains you are addressing, but am
> > > pessimistic
> > > > that having more reviewers will solve them.
> > > >
> > > > You are absolutely correct in calling out that the code is ugly
> complex
> > > and
> > > > missing coverage. The best way to address this is to clean up the
> code
> > > and
> > > > improve coverage. You say yourself "In the past single small changes
> > have
> > > > caused failures the were completely unforeseen by anyone". I don't
> > think
> > > > more eyeballs will go a long way in making someone see complex bugs
> > > > introduced by seemingly safe changes.
> > > >
> > > > I also am concerned that introducing a hurdle like this will make
> > > > committers more excited to review PRs with care, but rather might
> lead
> > to
> > > > less care. It would be great of our committers were more passionate
> > > about
> > > > PR reviews and do them more often, but forcing it rarely accomplishes
> > > that
> > > > goal.
> > > >
> > > > I'd rather see us trust our committers to decide how much review they
> > > > require to feel comfortable about their work and use the time saved
> to
> > > > address the root of the problem (accidental complexity & lack of test
> > > > coverage)
> > > >
> > > > On Wed, Jun 5, 2019 at 11:03 AM Udo Kohlmeyer <ud...@apache.org>
> wrote:
> > > >
> > > >> @Kirk, I totally understand the pain that you speak of. I too agree
> > that
> > > >> every line of changed code should have a test confirming that
> behavior
> > > >> was not changed.
> > > >>
> > > >> I don't believe that we need to go as far as revoking committer
> rights
> > > >> and reviewing each committer again, BUT it would be AMAZING that out
> > of
> > > >> our 100 committers, 80% of them would be more active in PR reviews,
> > > >> mailing lists and in the end active on the project outside of their
> > > >> focus area.
> > > >>
> > > >> I do want to remind all Geode committers, it IS your responsibility
> to
> > > >> be part of the PR review cycle. I will hold myself just as
> accountable
> > > >> to this than what I hold every committer to, as I've been just as
> lazy
> > > >> as the rest of them.
> > > >>
> > > >> BUT
> > > >>
> > > >> The reality is:
> > > >>
> > > >> 1. Geode code is HUGELY complex and NOT a test complete as we'd
> like
> > > >> 2. In the past single small changes have caused failures the were
> > > >> completely unforeseen by anyone
> > > >> 3. In the past commits with single reviewers, have caused backward
> > > >> compatibility issues which were only caught by chance in
> > unrelated
> > > >> testing.
> > > >> 4. There are 100 committers on Geode, and we keep on arguing that
> it
> > > is
> > > >> hard to get PR's reviewed and that is why it is ok to have
> only 1
> > > >> reviewer per PR.
> > > >> 5. There seems to be majority (unstated) opinion of: "why change,
> it
> > > >> has been working for us so far." (I call is unstated, because
> > being
> > > >> silent means you agree with the status quo)
> > > >> 6. With requiring only 1 reviewer on code submissions, we are
> > possibly
> > > >> creating areas of the code only understood by a few.
> > > >>
> > > >> IF, we as a project, have decided that all code shall enter only
> > through
> > > >> the flow of PR, then why not extend the QA cycle a little by
> requiring
> > > >> more eyes. Lazy consensus, is as stated, lazy and would only work
> in a
> > > >> project where the levels of complexity and size are not as high as
> > > >> Geode's. In addition, with PR submissions, we have admitted that we
> > are
> > > >> human and could make mistakes and in an already complex environment
> > and
> > > >> to the best of our ability get it wrong.
> > > >>
> > > >> Now, there are commits that really do not require 3 pairs of eyes,
> > > >> because spelling mistakes and typos don't need consensus. But any
> time
> > > >> code logic was amended, this needs to be reviewed.
> > > >>
> > > >> I have seen different approach to code submissions:
> > > >>
> > > >> * The submitter of the PR is NOT the committer of the PR. This
> task
> > > is
> > > >> the responsibility of another committer(s) to review, approve
> and
> > > >> finally merge in.
> > > >> * Smaller amount of committers with higher numbers of
> contributors.
> > > >> Yes, this does create a bottleneck, but it promotes a sense of
> > > pride
> > > >> and responsibility that individual feels. Possibly a greater
> > > >> understanding of the target module is promoted through this
> > > approach
> > > >> as well.
> > > >>
> > > >> Now, I don't say we as a project should follow these strict or
> > > >> restricting approaches, but from my perspective, if we as a project
> > > >> argue that we struggle to find 3 reviewers out of 100, then there
> are
> > > >> bigger problems in the project than we anticipated. It is not a lack
> > of
> > > >> trust in our committers, to me it is a sense of pride that I want
> > other
> > > >> committers to confirm that I've delivered code to the high standard
> > that
> > > >> we want to be known for. Whilst it is painful to go through the
> > process,
> > > >> but if done correctly it is beneficial to all involved, as differing
> > > >> opinions and approaches can be shared and all can learn from.
> > > >>
> > > >> In addition, I have personally stumbled upon a few PR's, which upon
> > > >> review found to be lacking in the areas of best practices of code
> > and/or
> > > >> design.
> > > >>
> > > >> I fully support the notion of 3 reviewers per PR. I'm also going to
> > take
> > > >> it one step further, in the list of reviewers, there is at least 1
> > > >> reviewer that is not part of a team, as this might drive a unbiased
> > view
> > > >> of the code and approach. I would also like to encourage ALL
> > committers
> > > >> to review code outside of the focus area. This will only promote a
> > > >> broader understanding of the project codebase. I also support the
> > notion
> > > >> of a pair/mobbing reviews, if a reviewer does not understand the
> > problem
> > > >> area enough to effectively review, OR where the solution is not
> > > apparent.
> > > >>
> > > >> --Udo
> > > >>
> > > >> On 6/4/19 16:49, Kirk Lund wrote:
> > > >>> I'm -1 for requiring N reviews before merging a commit.
> > > >>>
> > > >>> Overall, I support Lazy Consensus. If I post a PR that fixes the
> > > >> flakiness
> > > >>> in a test, the precheckin jobs prove it, and it sits there for 2
> > weeks
> > > >>> without reviews, then I favor merging it in at that point without
> any
> > > >>> reviews. I'm not going to chase people around or spam the dev list
> > over
> > > >> and
> > > >>> over asking for reviews. Nothing in the Apache Way says you have to
> > do
> > > >>> reviews before committing -- some projects prefer "commit then
> > review"
> > > >>> instead of "review then commit". You can always look at the code
> > > someone
> > > >>> changed and you can always change it further or revert it.
> > > >>>
> > > >>> I think if we don't trust our committers then we have a bigger
> > systemic
> > > >>> problem that becoming more strict about PR reviews isn not going to
> > > fix.
> > > >>>
> > > >>> Overall, I also favor pairing/mobbing over reviews. Without being
> > there
> > > >>> during the work, a reviewer lacks the context to understand why it
> > was
> > > >> done
> > > >>> the way it was done.
> > > >>>
> > > >>> If we cannot establish or maintain trust in committers, then I
> think
> > we
> > > >>> should remove committer status from everyone and start over as a
> > > project,
> > > >>> proposing and accepting one committer at a time.
> > > >>>
> > > >>> Instead of constraints on reviews, I would prefer to establish new
> > > >> criteria
> > > >>> for coding such as:
> > > >>> 1) all classes touched in a PR must have a unit test created if
> none
> > > >> exists
> > > >>> 2) all code touched in a PR must have unit test coverage (and
> > possibly
> > > >>> integration test coverage) specific to the changes
> > > >>> 3) all new classes must have full unit test coverage
> > > >>> 4) all code touched in a PR must follow clean code principles
> (which
> > > >> would
> > > >>> obviously need defining on the wiki)
> > > >>>
> > > >>> Then it becomes the responsibility of the author(s) and
> committer(s)
> > of
> > > >>> that PR to ensure that the code and the PR follows the project's
> > > criteria
> > > >>> for code quality and test coverage. It also becomes easier to
> measure
> > > the
> > > >>> PRs of a non-committer to determine if we think they would make a
> > good
> > > >>> committer (for example, do they adhere to clean code quality and
> unit
> > > >>> testing with mocks? -- along with any other criteria).
> > > >>>
> > > >>> On Thu, May 30, 2019 at 3:51 PM Owen Nichols <on...@pivotal.io>
> > > >> wrote:
> > > >>>> It seems common for Geode PRs to get merged with only a single
> green
> > > >>>> checkmark in GitHub.
> > > >>>>
> > > >>>> According to https://www.apache.org/foundation/voting.html we
> > should
> > > >> not
> > > >>>> be merging PRs with fewer than 3 green checkmarks.
> > > >>>>
> > > >>>> Consensus is a fundamental value in doing things The Apache Way.
> A
> > > >> single
> > > >>>> +1 is not consensus. Since we’re currently discussing what it
> takes
> > > to
> > > >>>> become a committer and what standards a committer is expected to
> > > >> uphold, it
> > > >>>> seems like a good time to review this policy.
> > > >>>>
> > > >>>> GitHub can be configured to require N reviews before a commit can
> be
> > > >>>> merged. Should we enable this feature?
> > > >>>>
> > > >>>> -Owen
> > > >>>> VOTES ON CODE MODIFICATION <
> > > >>>>
> > > >>
> > >
> https://www.apache.org/foundation/voting.html#votes-on-code-modification
> > >
> > > >>>> For code-modification votes, +1 votes are in favour of the
> proposal,
> > > but
> > > >>>> -1 votes are vetos <
> > > https://www.apache.org/foundation/voting.html#Veto>
> > > >>>> and kill the proposal dead until all vetoers withdraw their -1
> > votes.
> > > >>>>
> > > >>>> Unless a vote has been declared as using lazy consensus <
> > > >>>> https://www.apache.org/foundation/voting.html#LazyConsensus> ,
> > three
> > > +1
> > > >>>> votes are required for a code-modification proposal to pass.
> > > >>>>
> > > >>>> Whole numbers are recommended for this type of vote, as the
> opinion
> > > >> being
> > > >>>> expressed is Boolean: 'I approve/do not approve of this change.'
> > > >>>>
> > > >>>>
> > > >>>> CONSENSUS GAUGING THROUGH SILENCE <
> > > >>>> https://www.apache.org/foundation/voting.html#LazyConsensus>
> > > >>>> An alternative to voting that is sometimes used to measure the
> > > >>>> acceptability of something is the concept of lazy consensus <
> > > >>>> https://www.apache.org/foundation/glossary.html#LazyConsensus>.
> > > >>>>
> > > >>>> Lazy consensus is simply an announcement of 'silence gives
> assent.’
> > > When
> > > >>>> someone wants to determine the sense of the community this way, it
> > > >> might do
> > > >>>> so with a mail message such as:
> > > >>>> "The patch below fixes GEODE-12345; if no-one objects within three
> > > days,
> > > >>>> I'll assume lazy consensus and commit it."
> > > >>>>
> > > >>>> Lazy consensus cannot be used on projects that enforce a
> > > >>>> review-then-commit <
> > > >>>> https://www.apache.org/foundation/glossary.html#ReviewThenCommit>
> > > >> policy.
> > > >>>>
> > > >>>>
> > >
> >
>
--
*Joris Melchior *
CF Engineering
Pivotal Toronto
416 877 5427
“Programs must be written for people to read, and only incidentally for
machines to execute.” – *Hal Abelson*
<https://en.wikipedia.org/wiki/Hal_Abelson>
Re: [DISCUSS] require reviews before merging a PR
Posted by Jens Deppe <jd...@pivotal.io>.
As reviewers we should also feel empowered to request additional reviewers
on a PR (perhaps beyond whomever the original submitter may already have
requested).
I think that, sometimes the complexity of a change prevents someone from
commenting on just a portion of the change if they do not feel comfortable
understanding the scope of the whole change.
Having said that though, once you have 'touched' a PR you should also be
tracking the PR for additional commits or feedback until it is merged.
--Jens
On Wed, Jun 5, 2019 at 11:37 AM Alexander Murmann <am...@pivotal.io>
wrote:
> >
> > If we trust committers, why review at all? Just commit... and we might
> > catch a problem, we might not.
>
> Honestly that to me would be the ideal state. However, our test coverage
> and code quality is nowhere near to allow for that.
>
> What I was referring to is different though. I didn't say "trust them to
> write perfect code", but trust " to decide how much review they require to
> feel comfortable". In some cases this might mean one review and in others
> maybe two, three or even more and maybe even by very specific people.
>
> On Wed, Jun 5, 2019 at 11:31 AM Udo Kohlmeyer <ud...@apache.org> wrote:
>
> > Alexander, thank you for your response. And yes, change is uncomfortable
> > and in some cases more reviewers would not have caught issues. BUT, more
> > people would have seen the code, maybe become more familiar with it,
> etc...
> >
> > I don't say don't trust committers, as I am one. But I also know that I
> > mistakes are made regardless of intent. If we trust committers, why
> > review at all? Just commit... and we might catch a problem, we might not.
> >
> > --Udo
> >
> > On 6/5/19 11:20, Alexander Murmann wrote:
> > > Udo, I agree with many of the pains you are addressing, but am
> > pessimistic
> > > that having more reviewers will solve them.
> > >
> > > You are absolutely correct in calling out that the code is ugly complex
> > and
> > > missing coverage. The best way to address this is to clean up the code
> > and
> > > improve coverage. You say yourself "In the past single small changes
> have
> > > caused failures the were completely unforeseen by anyone". I don't
> think
> > > more eyeballs will go a long way in making someone see complex bugs
> > > introduced by seemingly safe changes.
> > >
> > > I also am concerned that introducing a hurdle like this will make
> > > committers more excited to review PRs with care, but rather might lead
> to
> > > less care. It would be great of our committers were more passionate
> > about
> > > PR reviews and do them more often, but forcing it rarely accomplishes
> > that
> > > goal.
> > >
> > > I'd rather see us trust our committers to decide how much review they
> > > require to feel comfortable about their work and use the time saved to
> > > address the root of the problem (accidental complexity & lack of test
> > > coverage)
> > >
> > > On Wed, Jun 5, 2019 at 11:03 AM Udo Kohlmeyer <ud...@apache.org> wrote:
> > >
> > >> @Kirk, I totally understand the pain that you speak of. I too agree
> that
> > >> every line of changed code should have a test confirming that behavior
> > >> was not changed.
> > >>
> > >> I don't believe that we need to go as far as revoking committer rights
> > >> and reviewing each committer again, BUT it would be AMAZING that out
> of
> > >> our 100 committers, 80% of them would be more active in PR reviews,
> > >> mailing lists and in the end active on the project outside of their
> > >> focus area.
> > >>
> > >> I do want to remind all Geode committers, it IS your responsibility to
> > >> be part of the PR review cycle. I will hold myself just as accountable
> > >> to this than what I hold every committer to, as I've been just as lazy
> > >> as the rest of them.
> > >>
> > >> BUT
> > >>
> > >> The reality is:
> > >>
> > >> 1. Geode code is HUGELY complex and NOT a test complete as we'd like
> > >> 2. In the past single small changes have caused failures the were
> > >> completely unforeseen by anyone
> > >> 3. In the past commits with single reviewers, have caused backward
> > >> compatibility issues which were only caught by chance in
> unrelated
> > >> testing.
> > >> 4. There are 100 committers on Geode, and we keep on arguing that it
> > is
> > >> hard to get PR's reviewed and that is why it is ok to have only 1
> > >> reviewer per PR.
> > >> 5. There seems to be majority (unstated) opinion of: "why change, it
> > >> has been working for us so far." (I call is unstated, because
> being
> > >> silent means you agree with the status quo)
> > >> 6. With requiring only 1 reviewer on code submissions, we are
> possibly
> > >> creating areas of the code only understood by a few.
> > >>
> > >> IF, we as a project, have decided that all code shall enter only
> through
> > >> the flow of PR, then why not extend the QA cycle a little by requiring
> > >> more eyes. Lazy consensus, is as stated, lazy and would only work in a
> > >> project where the levels of complexity and size are not as high as
> > >> Geode's. In addition, with PR submissions, we have admitted that we
> are
> > >> human and could make mistakes and in an already complex environment
> and
> > >> to the best of our ability get it wrong.
> > >>
> > >> Now, there are commits that really do not require 3 pairs of eyes,
> > >> because spelling mistakes and typos don't need consensus. But any time
> > >> code logic was amended, this needs to be reviewed.
> > >>
> > >> I have seen different approach to code submissions:
> > >>
> > >> * The submitter of the PR is NOT the committer of the PR. This task
> > is
> > >> the responsibility of another committer(s) to review, approve and
> > >> finally merge in.
> > >> * Smaller amount of committers with higher numbers of contributors.
> > >> Yes, this does create a bottleneck, but it promotes a sense of
> > pride
> > >> and responsibility that individual feels. Possibly a greater
> > >> understanding of the target module is promoted through this
> > approach
> > >> as well.
> > >>
> > >> Now, I don't say we as a project should follow these strict or
> > >> restricting approaches, but from my perspective, if we as a project
> > >> argue that we struggle to find 3 reviewers out of 100, then there are
> > >> bigger problems in the project than we anticipated. It is not a lack
> of
> > >> trust in our committers, to me it is a sense of pride that I want
> other
> > >> committers to confirm that I've delivered code to the high standard
> that
> > >> we want to be known for. Whilst it is painful to go through the
> process,
> > >> but if done correctly it is beneficial to all involved, as differing
> > >> opinions and approaches can be shared and all can learn from.
> > >>
> > >> In addition, I have personally stumbled upon a few PR's, which upon
> > >> review found to be lacking in the areas of best practices of code
> and/or
> > >> design.
> > >>
> > >> I fully support the notion of 3 reviewers per PR. I'm also going to
> take
> > >> it one step further, in the list of reviewers, there is at least 1
> > >> reviewer that is not part of a team, as this might drive a unbiased
> view
> > >> of the code and approach. I would also like to encourage ALL
> committers
> > >> to review code outside of the focus area. This will only promote a
> > >> broader understanding of the project codebase. I also support the
> notion
> > >> of a pair/mobbing reviews, if a reviewer does not understand the
> problem
> > >> area enough to effectively review, OR where the solution is not
> > apparent.
> > >>
> > >> --Udo
> > >>
> > >> On 6/4/19 16:49, Kirk Lund wrote:
> > >>> I'm -1 for requiring N reviews before merging a commit.
> > >>>
> > >>> Overall, I support Lazy Consensus. If I post a PR that fixes the
> > >> flakiness
> > >>> in a test, the precheckin jobs prove it, and it sits there for 2
> weeks
> > >>> without reviews, then I favor merging it in at that point without any
> > >>> reviews. I'm not going to chase people around or spam the dev list
> over
> > >> and
> > >>> over asking for reviews. Nothing in the Apache Way says you have to
> do
> > >>> reviews before committing -- some projects prefer "commit then
> review"
> > >>> instead of "review then commit". You can always look at the code
> > someone
> > >>> changed and you can always change it further or revert it.
> > >>>
> > >>> I think if we don't trust our committers then we have a bigger
> systemic
> > >>> problem that becoming more strict about PR reviews isn not going to
> > fix.
> > >>>
> > >>> Overall, I also favor pairing/mobbing over reviews. Without being
> there
> > >>> during the work, a reviewer lacks the context to understand why it
> was
> > >> done
> > >>> the way it was done.
> > >>>
> > >>> If we cannot establish or maintain trust in committers, then I think
> we
> > >>> should remove committer status from everyone and start over as a
> > project,
> > >>> proposing and accepting one committer at a time.
> > >>>
> > >>> Instead of constraints on reviews, I would prefer to establish new
> > >> criteria
> > >>> for coding such as:
> > >>> 1) all classes touched in a PR must have a unit test created if none
> > >> exists
> > >>> 2) all code touched in a PR must have unit test coverage (and
> possibly
> > >>> integration test coverage) specific to the changes
> > >>> 3) all new classes must have full unit test coverage
> > >>> 4) all code touched in a PR must follow clean code principles (which
> > >> would
> > >>> obviously need defining on the wiki)
> > >>>
> > >>> Then it becomes the responsibility of the author(s) and committer(s)
> of
> > >>> that PR to ensure that the code and the PR follows the project's
> > criteria
> > >>> for code quality and test coverage. It also becomes easier to measure
> > the
> > >>> PRs of a non-committer to determine if we think they would make a
> good
> > >>> committer (for example, do they adhere to clean code quality and unit
> > >>> testing with mocks? -- along with any other criteria).
> > >>>
> > >>> On Thu, May 30, 2019 at 3:51 PM Owen Nichols <on...@pivotal.io>
> > >> wrote:
> > >>>> It seems common for Geode PRs to get merged with only a single green
> > >>>> checkmark in GitHub.
> > >>>>
> > >>>> According to https://www.apache.org/foundation/voting.html we
> should
> > >> not
> > >>>> be merging PRs with fewer than 3 green checkmarks.
> > >>>>
> > >>>> Consensus is a fundamental value in doing things The Apache Way. A
> > >> single
> > >>>> +1 is not consensus. Since we’re currently discussing what it takes
> > to
> > >>>> become a committer and what standards a committer is expected to
> > >> uphold, it
> > >>>> seems like a good time to review this policy.
> > >>>>
> > >>>> GitHub can be configured to require N reviews before a commit can be
> > >>>> merged. Should we enable this feature?
> > >>>>
> > >>>> -Owen
> > >>>> VOTES ON CODE MODIFICATION <
> > >>>>
> > >>
> > https://www.apache.org/foundation/voting.html#votes-on-code-modification
> >
> > >>>> For code-modification votes, +1 votes are in favour of the proposal,
> > but
> > >>>> -1 votes are vetos <
> > https://www.apache.org/foundation/voting.html#Veto>
> > >>>> and kill the proposal dead until all vetoers withdraw their -1
> votes.
> > >>>>
> > >>>> Unless a vote has been declared as using lazy consensus <
> > >>>> https://www.apache.org/foundation/voting.html#LazyConsensus> ,
> three
> > +1
> > >>>> votes are required for a code-modification proposal to pass.
> > >>>>
> > >>>> Whole numbers are recommended for this type of vote, as the opinion
> > >> being
> > >>>> expressed is Boolean: 'I approve/do not approve of this change.'
> > >>>>
> > >>>>
> > >>>> CONSENSUS GAUGING THROUGH SILENCE <
> > >>>> https://www.apache.org/foundation/voting.html#LazyConsensus>
> > >>>> An alternative to voting that is sometimes used to measure the
> > >>>> acceptability of something is the concept of lazy consensus <
> > >>>> https://www.apache.org/foundation/glossary.html#LazyConsensus>.
> > >>>>
> > >>>> Lazy consensus is simply an announcement of 'silence gives assent.’
> > When
> > >>>> someone wants to determine the sense of the community this way, it
> > >> might do
> > >>>> so with a mail message such as:
> > >>>> "The patch below fixes GEODE-12345; if no-one objects within three
> > days,
> > >>>> I'll assume lazy consensus and commit it."
> > >>>>
> > >>>> Lazy consensus cannot be used on projects that enforce a
> > >>>> review-then-commit <
> > >>>> https://www.apache.org/foundation/glossary.html#ReviewThenCommit>
> > >> policy.
> > >>>>
> > >>>>
> >
>
Re: [DISCUSS] require reviews before merging a PR
Posted by Alexander Murmann <am...@pivotal.io>.
>
> If we trust committers, why review at all? Just commit... and we might
> catch a problem, we might not.
Honestly that to me would be the ideal state. However, our test coverage
and code quality is nowhere near to allow for that.
What I was referring to is different though. I didn't say "trust them to
write perfect code", but trust " to decide how much review they require to
feel comfortable". In some cases this might mean one review and in others
maybe two, three or even more and maybe even by very specific people.
On Wed, Jun 5, 2019 at 11:31 AM Udo Kohlmeyer <ud...@apache.org> wrote:
> Alexander, thank you for your response. And yes, change is uncomfortable
> and in some cases more reviewers would not have caught issues. BUT, more
> people would have seen the code, maybe become more familiar with it, etc...
>
> I don't say don't trust committers, as I am one. But I also know that I
> mistakes are made regardless of intent. If we trust committers, why
> review at all? Just commit... and we might catch a problem, we might not.
>
> --Udo
>
> On 6/5/19 11:20, Alexander Murmann wrote:
> > Udo, I agree with many of the pains you are addressing, but am
> pessimistic
> > that having more reviewers will solve them.
> >
> > You are absolutely correct in calling out that the code is ugly complex
> and
> > missing coverage. The best way to address this is to clean up the code
> and
> > improve coverage. You say yourself "In the past single small changes have
> > caused failures the were completely unforeseen by anyone". I don't think
> > more eyeballs will go a long way in making someone see complex bugs
> > introduced by seemingly safe changes.
> >
> > I also am concerned that introducing a hurdle like this will make
> > committers more excited to review PRs with care, but rather might lead to
> > less care. It would be great of our committers were more passionate
> about
> > PR reviews and do them more often, but forcing it rarely accomplishes
> that
> > goal.
> >
> > I'd rather see us trust our committers to decide how much review they
> > require to feel comfortable about their work and use the time saved to
> > address the root of the problem (accidental complexity & lack of test
> > coverage)
> >
> > On Wed, Jun 5, 2019 at 11:03 AM Udo Kohlmeyer <ud...@apache.org> wrote:
> >
> >> @Kirk, I totally understand the pain that you speak of. I too agree that
> >> every line of changed code should have a test confirming that behavior
> >> was not changed.
> >>
> >> I don't believe that we need to go as far as revoking committer rights
> >> and reviewing each committer again, BUT it would be AMAZING that out of
> >> our 100 committers, 80% of them would be more active in PR reviews,
> >> mailing lists and in the end active on the project outside of their
> >> focus area.
> >>
> >> I do want to remind all Geode committers, it IS your responsibility to
> >> be part of the PR review cycle. I will hold myself just as accountable
> >> to this than what I hold every committer to, as I've been just as lazy
> >> as the rest of them.
> >>
> >> BUT
> >>
> >> The reality is:
> >>
> >> 1. Geode code is HUGELY complex and NOT a test complete as we'd like
> >> 2. In the past single small changes have caused failures the were
> >> completely unforeseen by anyone
> >> 3. In the past commits with single reviewers, have caused backward
> >> compatibility issues which were only caught by chance in unrelated
> >> testing.
> >> 4. There are 100 committers on Geode, and we keep on arguing that it
> is
> >> hard to get PR's reviewed and that is why it is ok to have only 1
> >> reviewer per PR.
> >> 5. There seems to be majority (unstated) opinion of: "why change, it
> >> has been working for us so far." (I call is unstated, because being
> >> silent means you agree with the status quo)
> >> 6. With requiring only 1 reviewer on code submissions, we are possibly
> >> creating areas of the code only understood by a few.
> >>
> >> IF, we as a project, have decided that all code shall enter only through
> >> the flow of PR, then why not extend the QA cycle a little by requiring
> >> more eyes. Lazy consensus, is as stated, lazy and would only work in a
> >> project where the levels of complexity and size are not as high as
> >> Geode's. In addition, with PR submissions, we have admitted that we are
> >> human and could make mistakes and in an already complex environment and
> >> to the best of our ability get it wrong.
> >>
> >> Now, there are commits that really do not require 3 pairs of eyes,
> >> because spelling mistakes and typos don't need consensus. But any time
> >> code logic was amended, this needs to be reviewed.
> >>
> >> I have seen different approach to code submissions:
> >>
> >> * The submitter of the PR is NOT the committer of the PR. This task
> is
> >> the responsibility of another committer(s) to review, approve and
> >> finally merge in.
> >> * Smaller amount of committers with higher numbers of contributors.
> >> Yes, this does create a bottleneck, but it promotes a sense of
> pride
> >> and responsibility that individual feels. Possibly a greater
> >> understanding of the target module is promoted through this
> approach
> >> as well.
> >>
> >> Now, I don't say we as a project should follow these strict or
> >> restricting approaches, but from my perspective, if we as a project
> >> argue that we struggle to find 3 reviewers out of 100, then there are
> >> bigger problems in the project than we anticipated. It is not a lack of
> >> trust in our committers, to me it is a sense of pride that I want other
> >> committers to confirm that I've delivered code to the high standard that
> >> we want to be known for. Whilst it is painful to go through the process,
> >> but if done correctly it is beneficial to all involved, as differing
> >> opinions and approaches can be shared and all can learn from.
> >>
> >> In addition, I have personally stumbled upon a few PR's, which upon
> >> review found to be lacking in the areas of best practices of code and/or
> >> design.
> >>
> >> I fully support the notion of 3 reviewers per PR. I'm also going to take
> >> it one step further, in the list of reviewers, there is at least 1
> >> reviewer that is not part of a team, as this might drive a unbiased view
> >> of the code and approach. I would also like to encourage ALL committers
> >> to review code outside of the focus area. This will only promote a
> >> broader understanding of the project codebase. I also support the notion
> >> of a pair/mobbing reviews, if a reviewer does not understand the problem
> >> area enough to effectively review, OR where the solution is not
> apparent.
> >>
> >> --Udo
> >>
> >> On 6/4/19 16:49, Kirk Lund wrote:
> >>> I'm -1 for requiring N reviews before merging a commit.
> >>>
> >>> Overall, I support Lazy Consensus. If I post a PR that fixes the
> >> flakiness
> >>> in a test, the precheckin jobs prove it, and it sits there for 2 weeks
> >>> without reviews, then I favor merging it in at that point without any
> >>> reviews. I'm not going to chase people around or spam the dev list over
> >> and
> >>> over asking for reviews. Nothing in the Apache Way says you have to do
> >>> reviews before committing -- some projects prefer "commit then review"
> >>> instead of "review then commit". You can always look at the code
> someone
> >>> changed and you can always change it further or revert it.
> >>>
> >>> I think if we don't trust our committers then we have a bigger systemic
> >>> problem that becoming more strict about PR reviews isn not going to
> fix.
> >>>
> >>> Overall, I also favor pairing/mobbing over reviews. Without being there
> >>> during the work, a reviewer lacks the context to understand why it was
> >> done
> >>> the way it was done.
> >>>
> >>> If we cannot establish or maintain trust in committers, then I think we
> >>> should remove committer status from everyone and start over as a
> project,
> >>> proposing and accepting one committer at a time.
> >>>
> >>> Instead of constraints on reviews, I would prefer to establish new
> >> criteria
> >>> for coding such as:
> >>> 1) all classes touched in a PR must have a unit test created if none
> >> exists
> >>> 2) all code touched in a PR must have unit test coverage (and possibly
> >>> integration test coverage) specific to the changes
> >>> 3) all new classes must have full unit test coverage
> >>> 4) all code touched in a PR must follow clean code principles (which
> >> would
> >>> obviously need defining on the wiki)
> >>>
> >>> Then it becomes the responsibility of the author(s) and committer(s) of
> >>> that PR to ensure that the code and the PR follows the project's
> criteria
> >>> for code quality and test coverage. It also becomes easier to measure
> the
> >>> PRs of a non-committer to determine if we think they would make a good
> >>> committer (for example, do they adhere to clean code quality and unit
> >>> testing with mocks? -- along with any other criteria).
> >>>
> >>> On Thu, May 30, 2019 at 3:51 PM Owen Nichols <on...@pivotal.io>
> >> wrote:
> >>>> It seems common for Geode PRs to get merged with only a single green
> >>>> checkmark in GitHub.
> >>>>
> >>>> According to https://www.apache.org/foundation/voting.html we should
> >> not
> >>>> be merging PRs with fewer than 3 green checkmarks.
> >>>>
> >>>> Consensus is a fundamental value in doing things The Apache Way. A
> >> single
> >>>> +1 is not consensus. Since we’re currently discussing what it takes
> to
> >>>> become a committer and what standards a committer is expected to
> >> uphold, it
> >>>> seems like a good time to review this policy.
> >>>>
> >>>> GitHub can be configured to require N reviews before a commit can be
> >>>> merged. Should we enable this feature?
> >>>>
> >>>> -Owen
> >>>> VOTES ON CODE MODIFICATION <
> >>>>
> >>
> https://www.apache.org/foundation/voting.html#votes-on-code-modification>
> >>>> For code-modification votes, +1 votes are in favour of the proposal,
> but
> >>>> -1 votes are vetos <
> https://www.apache.org/foundation/voting.html#Veto>
> >>>> and kill the proposal dead until all vetoers withdraw their -1 votes.
> >>>>
> >>>> Unless a vote has been declared as using lazy consensus <
> >>>> https://www.apache.org/foundation/voting.html#LazyConsensus> , three
> +1
> >>>> votes are required for a code-modification proposal to pass.
> >>>>
> >>>> Whole numbers are recommended for this type of vote, as the opinion
> >> being
> >>>> expressed is Boolean: 'I approve/do not approve of this change.'
> >>>>
> >>>>
> >>>> CONSENSUS GAUGING THROUGH SILENCE <
> >>>> https://www.apache.org/foundation/voting.html#LazyConsensus>
> >>>> An alternative to voting that is sometimes used to measure the
> >>>> acceptability of something is the concept of lazy consensus <
> >>>> https://www.apache.org/foundation/glossary.html#LazyConsensus>.
> >>>>
> >>>> Lazy consensus is simply an announcement of 'silence gives assent.’
> When
> >>>> someone wants to determine the sense of the community this way, it
> >> might do
> >>>> so with a mail message such as:
> >>>> "The patch below fixes GEODE-12345; if no-one objects within three
> days,
> >>>> I'll assume lazy consensus and commit it."
> >>>>
> >>>> Lazy consensus cannot be used on projects that enforce a
> >>>> review-then-commit <
> >>>> https://www.apache.org/foundation/glossary.html#ReviewThenCommit>
> >> policy.
> >>>>
> >>>>
>
Re: [DISCUSS] require reviews before merging a PR
Posted by Udo Kohlmeyer <ud...@apache.org>.
Alexander, thank you for your response. And yes, change is uncomfortable
and in some cases more reviewers would not have caught issues. BUT, more
people would have seen the code, maybe become more familiar with it, etc...
I don't say don't trust committers, as I am one. But I also know that I
mistakes are made regardless of intent. If we trust committers, why
review at all? Just commit... and we might catch a problem, we might not.
--Udo
On 6/5/19 11:20, Alexander Murmann wrote:
> Udo, I agree with many of the pains you are addressing, but am pessimistic
> that having more reviewers will solve them.
>
> You are absolutely correct in calling out that the code is ugly complex and
> missing coverage. The best way to address this is to clean up the code and
> improve coverage. You say yourself "In the past single small changes have
> caused failures the were completely unforeseen by anyone". I don't think
> more eyeballs will go a long way in making someone see complex bugs
> introduced by seemingly safe changes.
>
> I also am concerned that introducing a hurdle like this will make
> committers more excited to review PRs with care, but rather might lead to
> less care. It would be great of our committers were more passionate about
> PR reviews and do them more often, but forcing it rarely accomplishes that
> goal.
>
> I'd rather see us trust our committers to decide how much review they
> require to feel comfortable about their work and use the time saved to
> address the root of the problem (accidental complexity & lack of test
> coverage)
>
> On Wed, Jun 5, 2019 at 11:03 AM Udo Kohlmeyer <ud...@apache.org> wrote:
>
>> @Kirk, I totally understand the pain that you speak of. I too agree that
>> every line of changed code should have a test confirming that behavior
>> was not changed.
>>
>> I don't believe that we need to go as far as revoking committer rights
>> and reviewing each committer again, BUT it would be AMAZING that out of
>> our 100 committers, 80% of them would be more active in PR reviews,
>> mailing lists and in the end active on the project outside of their
>> focus area.
>>
>> I do want to remind all Geode committers, it IS your responsibility to
>> be part of the PR review cycle. I will hold myself just as accountable
>> to this than what I hold every committer to, as I've been just as lazy
>> as the rest of them.
>>
>> BUT
>>
>> The reality is:
>>
>> 1. Geode code is HUGELY complex and NOT a test complete as we'd like
>> 2. In the past single small changes have caused failures the were
>> completely unforeseen by anyone
>> 3. In the past commits with single reviewers, have caused backward
>> compatibility issues which were only caught by chance in unrelated
>> testing.
>> 4. There are 100 committers on Geode, and we keep on arguing that it is
>> hard to get PR's reviewed and that is why it is ok to have only 1
>> reviewer per PR.
>> 5. There seems to be majority (unstated) opinion of: "why change, it
>> has been working for us so far." (I call is unstated, because being
>> silent means you agree with the status quo)
>> 6. With requiring only 1 reviewer on code submissions, we are possibly
>> creating areas of the code only understood by a few.
>>
>> IF, we as a project, have decided that all code shall enter only through
>> the flow of PR, then why not extend the QA cycle a little by requiring
>> more eyes. Lazy consensus, is as stated, lazy and would only work in a
>> project where the levels of complexity and size are not as high as
>> Geode's. In addition, with PR submissions, we have admitted that we are
>> human and could make mistakes and in an already complex environment and
>> to the best of our ability get it wrong.
>>
>> Now, there are commits that really do not require 3 pairs of eyes,
>> because spelling mistakes and typos don't need consensus. But any time
>> code logic was amended, this needs to be reviewed.
>>
>> I have seen different approach to code submissions:
>>
>> * The submitter of the PR is NOT the committer of the PR. This task is
>> the responsibility of another committer(s) to review, approve and
>> finally merge in.
>> * Smaller amount of committers with higher numbers of contributors.
>> Yes, this does create a bottleneck, but it promotes a sense of pride
>> and responsibility that individual feels. Possibly a greater
>> understanding of the target module is promoted through this approach
>> as well.
>>
>> Now, I don't say we as a project should follow these strict or
>> restricting approaches, but from my perspective, if we as a project
>> argue that we struggle to find 3 reviewers out of 100, then there are
>> bigger problems in the project than we anticipated. It is not a lack of
>> trust in our committers, to me it is a sense of pride that I want other
>> committers to confirm that I've delivered code to the high standard that
>> we want to be known for. Whilst it is painful to go through the process,
>> but if done correctly it is beneficial to all involved, as differing
>> opinions and approaches can be shared and all can learn from.
>>
>> In addition, I have personally stumbled upon a few PR's, which upon
>> review found to be lacking in the areas of best practices of code and/or
>> design.
>>
>> I fully support the notion of 3 reviewers per PR. I'm also going to take
>> it one step further, in the list of reviewers, there is at least 1
>> reviewer that is not part of a team, as this might drive a unbiased view
>> of the code and approach. I would also like to encourage ALL committers
>> to review code outside of the focus area. This will only promote a
>> broader understanding of the project codebase. I also support the notion
>> of a pair/mobbing reviews, if a reviewer does not understand the problem
>> area enough to effectively review, OR where the solution is not apparent.
>>
>> --Udo
>>
>> On 6/4/19 16:49, Kirk Lund wrote:
>>> I'm -1 for requiring N reviews before merging a commit.
>>>
>>> Overall, I support Lazy Consensus. If I post a PR that fixes the
>> flakiness
>>> in a test, the precheckin jobs prove it, and it sits there for 2 weeks
>>> without reviews, then I favor merging it in at that point without any
>>> reviews. I'm not going to chase people around or spam the dev list over
>> and
>>> over asking for reviews. Nothing in the Apache Way says you have to do
>>> reviews before committing -- some projects prefer "commit then review"
>>> instead of "review then commit". You can always look at the code someone
>>> changed and you can always change it further or revert it.
>>>
>>> I think if we don't trust our committers then we have a bigger systemic
>>> problem that becoming more strict about PR reviews isn not going to fix.
>>>
>>> Overall, I also favor pairing/mobbing over reviews. Without being there
>>> during the work, a reviewer lacks the context to understand why it was
>> done
>>> the way it was done.
>>>
>>> If we cannot establish or maintain trust in committers, then I think we
>>> should remove committer status from everyone and start over as a project,
>>> proposing and accepting one committer at a time.
>>>
>>> Instead of constraints on reviews, I would prefer to establish new
>> criteria
>>> for coding such as:
>>> 1) all classes touched in a PR must have a unit test created if none
>> exists
>>> 2) all code touched in a PR must have unit test coverage (and possibly
>>> integration test coverage) specific to the changes
>>> 3) all new classes must have full unit test coverage
>>> 4) all code touched in a PR must follow clean code principles (which
>> would
>>> obviously need defining on the wiki)
>>>
>>> Then it becomes the responsibility of the author(s) and committer(s) of
>>> that PR to ensure that the code and the PR follows the project's criteria
>>> for code quality and test coverage. It also becomes easier to measure the
>>> PRs of a non-committer to determine if we think they would make a good
>>> committer (for example, do they adhere to clean code quality and unit
>>> testing with mocks? -- along with any other criteria).
>>>
>>> On Thu, May 30, 2019 at 3:51 PM Owen Nichols <on...@pivotal.io>
>> wrote:
>>>> It seems common for Geode PRs to get merged with only a single green
>>>> checkmark in GitHub.
>>>>
>>>> According to https://www.apache.org/foundation/voting.html we should
>> not
>>>> be merging PRs with fewer than 3 green checkmarks.
>>>>
>>>> Consensus is a fundamental value in doing things The Apache Way. A
>> single
>>>> +1 is not consensus. Since we’re currently discussing what it takes to
>>>> become a committer and what standards a committer is expected to
>> uphold, it
>>>> seems like a good time to review this policy.
>>>>
>>>> GitHub can be configured to require N reviews before a commit can be
>>>> merged. Should we enable this feature?
>>>>
>>>> -Owen
>>>> VOTES ON CODE MODIFICATION <
>>>>
>> https://www.apache.org/foundation/voting.html#votes-on-code-modification>
>>>> For code-modification votes, +1 votes are in favour of the proposal, but
>>>> -1 votes are vetos <https://www.apache.org/foundation/voting.html#Veto>
>>>> and kill the proposal dead until all vetoers withdraw their -1 votes.
>>>>
>>>> Unless a vote has been declared as using lazy consensus <
>>>> https://www.apache.org/foundation/voting.html#LazyConsensus> , three +1
>>>> votes are required for a code-modification proposal to pass.
>>>>
>>>> Whole numbers are recommended for this type of vote, as the opinion
>> being
>>>> expressed is Boolean: 'I approve/do not approve of this change.'
>>>>
>>>>
>>>> CONSENSUS GAUGING THROUGH SILENCE <
>>>> https://www.apache.org/foundation/voting.html#LazyConsensus>
>>>> An alternative to voting that is sometimes used to measure the
>>>> acceptability of something is the concept of lazy consensus <
>>>> https://www.apache.org/foundation/glossary.html#LazyConsensus>.
>>>>
>>>> Lazy consensus is simply an announcement of 'silence gives assent.’ When
>>>> someone wants to determine the sense of the community this way, it
>> might do
>>>> so with a mail message such as:
>>>> "The patch below fixes GEODE-12345; if no-one objects within three days,
>>>> I'll assume lazy consensus and commit it."
>>>>
>>>> Lazy consensus cannot be used on projects that enforce a
>>>> review-then-commit <
>>>> https://www.apache.org/foundation/glossary.html#ReviewThenCommit>
>> policy.
>>>>
>>>>
Re: [DISCUSS] require reviews before merging a PR
Posted by Alexander Murmann <am...@pivotal.io>.
Udo, I agree with many of the pains you are addressing, but am pessimistic
that having more reviewers will solve them.
You are absolutely correct in calling out that the code is ugly complex and
missing coverage. The best way to address this is to clean up the code and
improve coverage. You say yourself "In the past single small changes have
caused failures the were completely unforeseen by anyone". I don't think
more eyeballs will go a long way in making someone see complex bugs
introduced by seemingly safe changes.
I also am concerned that introducing a hurdle like this will make
committers more excited to review PRs with care, but rather might lead to
less care. It would be great of our committers were more passionate about
PR reviews and do them more often, but forcing it rarely accomplishes that
goal.
I'd rather see us trust our committers to decide how much review they
require to feel comfortable about their work and use the time saved to
address the root of the problem (accidental complexity & lack of test
coverage)
On Wed, Jun 5, 2019 at 11:03 AM Udo Kohlmeyer <ud...@apache.org> wrote:
> @Kirk, I totally understand the pain that you speak of. I too agree that
> every line of changed code should have a test confirming that behavior
> was not changed.
>
> I don't believe that we need to go as far as revoking committer rights
> and reviewing each committer again, BUT it would be AMAZING that out of
> our 100 committers, 80% of them would be more active in PR reviews,
> mailing lists and in the end active on the project outside of their
> focus area.
>
> I do want to remind all Geode committers, it IS your responsibility to
> be part of the PR review cycle. I will hold myself just as accountable
> to this than what I hold every committer to, as I've been just as lazy
> as the rest of them.
>
> BUT
>
> The reality is:
>
> 1. Geode code is HUGELY complex and NOT a test complete as we'd like
> 2. In the past single small changes have caused failures the were
> completely unforeseen by anyone
> 3. In the past commits with single reviewers, have caused backward
> compatibility issues which were only caught by chance in unrelated
> testing.
> 4. There are 100 committers on Geode, and we keep on arguing that it is
> hard to get PR's reviewed and that is why it is ok to have only 1
> reviewer per PR.
> 5. There seems to be majority (unstated) opinion of: "why change, it
> has been working for us so far." (I call is unstated, because being
> silent means you agree with the status quo)
> 6. With requiring only 1 reviewer on code submissions, we are possibly
> creating areas of the code only understood by a few.
>
> IF, we as a project, have decided that all code shall enter only through
> the flow of PR, then why not extend the QA cycle a little by requiring
> more eyes. Lazy consensus, is as stated, lazy and would only work in a
> project where the levels of complexity and size are not as high as
> Geode's. In addition, with PR submissions, we have admitted that we are
> human and could make mistakes and in an already complex environment and
> to the best of our ability get it wrong.
>
> Now, there are commits that really do not require 3 pairs of eyes,
> because spelling mistakes and typos don't need consensus. But any time
> code logic was amended, this needs to be reviewed.
>
> I have seen different approach to code submissions:
>
> * The submitter of the PR is NOT the committer of the PR. This task is
> the responsibility of another committer(s) to review, approve and
> finally merge in.
> * Smaller amount of committers with higher numbers of contributors.
> Yes, this does create a bottleneck, but it promotes a sense of pride
> and responsibility that individual feels. Possibly a greater
> understanding of the target module is promoted through this approach
> as well.
>
> Now, I don't say we as a project should follow these strict or
> restricting approaches, but from my perspective, if we as a project
> argue that we struggle to find 3 reviewers out of 100, then there are
> bigger problems in the project than we anticipated. It is not a lack of
> trust in our committers, to me it is a sense of pride that I want other
> committers to confirm that I've delivered code to the high standard that
> we want to be known for. Whilst it is painful to go through the process,
> but if done correctly it is beneficial to all involved, as differing
> opinions and approaches can be shared and all can learn from.
>
> In addition, I have personally stumbled upon a few PR's, which upon
> review found to be lacking in the areas of best practices of code and/or
> design.
>
> I fully support the notion of 3 reviewers per PR. I'm also going to take
> it one step further, in the list of reviewers, there is at least 1
> reviewer that is not part of a team, as this might drive a unbiased view
> of the code and approach. I would also like to encourage ALL committers
> to review code outside of the focus area. This will only promote a
> broader understanding of the project codebase. I also support the notion
> of a pair/mobbing reviews, if a reviewer does not understand the problem
> area enough to effectively review, OR where the solution is not apparent.
>
> --Udo
>
> On 6/4/19 16:49, Kirk Lund wrote:
> > I'm -1 for requiring N reviews before merging a commit.
> >
> > Overall, I support Lazy Consensus. If I post a PR that fixes the
> flakiness
> > in a test, the precheckin jobs prove it, and it sits there for 2 weeks
> > without reviews, then I favor merging it in at that point without any
> > reviews. I'm not going to chase people around or spam the dev list over
> and
> > over asking for reviews. Nothing in the Apache Way says you have to do
> > reviews before committing -- some projects prefer "commit then review"
> > instead of "review then commit". You can always look at the code someone
> > changed and you can always change it further or revert it.
> >
> > I think if we don't trust our committers then we have a bigger systemic
> > problem that becoming more strict about PR reviews isn not going to fix.
> >
> > Overall, I also favor pairing/mobbing over reviews. Without being there
> > during the work, a reviewer lacks the context to understand why it was
> done
> > the way it was done.
> >
> > If we cannot establish or maintain trust in committers, then I think we
> > should remove committer status from everyone and start over as a project,
> > proposing and accepting one committer at a time.
> >
> > Instead of constraints on reviews, I would prefer to establish new
> criteria
> > for coding such as:
> > 1) all classes touched in a PR must have a unit test created if none
> exists
> > 2) all code touched in a PR must have unit test coverage (and possibly
> > integration test coverage) specific to the changes
> > 3) all new classes must have full unit test coverage
> > 4) all code touched in a PR must follow clean code principles (which
> would
> > obviously need defining on the wiki)
> >
> > Then it becomes the responsibility of the author(s) and committer(s) of
> > that PR to ensure that the code and the PR follows the project's criteria
> > for code quality and test coverage. It also becomes easier to measure the
> > PRs of a non-committer to determine if we think they would make a good
> > committer (for example, do they adhere to clean code quality and unit
> > testing with mocks? -- along with any other criteria).
> >
> > On Thu, May 30, 2019 at 3:51 PM Owen Nichols <on...@pivotal.io>
> wrote:
> >
> >> It seems common for Geode PRs to get merged with only a single green
> >> checkmark in GitHub.
> >>
> >> According to https://www.apache.org/foundation/voting.html we should
> not
> >> be merging PRs with fewer than 3 green checkmarks.
> >>
> >> Consensus is a fundamental value in doing things The Apache Way. A
> single
> >> +1 is not consensus. Since we’re currently discussing what it takes to
> >> become a committer and what standards a committer is expected to
> uphold, it
> >> seems like a good time to review this policy.
> >>
> >> GitHub can be configured to require N reviews before a commit can be
> >> merged. Should we enable this feature?
> >>
> >> -Owen
> >> VOTES ON CODE MODIFICATION <
> >>
> https://www.apache.org/foundation/voting.html#votes-on-code-modification>
> >> For code-modification votes, +1 votes are in favour of the proposal, but
> >> -1 votes are vetos <https://www.apache.org/foundation/voting.html#Veto>
> >> and kill the proposal dead until all vetoers withdraw their -1 votes.
> >>
> >> Unless a vote has been declared as using lazy consensus <
> >> https://www.apache.org/foundation/voting.html#LazyConsensus> , three +1
> >> votes are required for a code-modification proposal to pass.
> >>
> >> Whole numbers are recommended for this type of vote, as the opinion
> being
> >> expressed is Boolean: 'I approve/do not approve of this change.'
> >>
> >>
> >> CONSENSUS GAUGING THROUGH SILENCE <
> >> https://www.apache.org/foundation/voting.html#LazyConsensus>
> >> An alternative to voting that is sometimes used to measure the
> >> acceptability of something is the concept of lazy consensus <
> >> https://www.apache.org/foundation/glossary.html#LazyConsensus>.
> >>
> >> Lazy consensus is simply an announcement of 'silence gives assent.’ When
> >> someone wants to determine the sense of the community this way, it
> might do
> >> so with a mail message such as:
> >> "The patch below fixes GEODE-12345; if no-one objects within three days,
> >> I'll assume lazy consensus and commit it."
> >>
> >> Lazy consensus cannot be used on projects that enforce a
> >> review-then-commit <
> >> https://www.apache.org/foundation/glossary.html#ReviewThenCommit>
> policy.
> >>
> >>
> >>
>
Re: [DISCUSS] require reviews before merging a PR
Posted by Udo Kohlmeyer <ud...@apache.org>.
@Kirk, I totally understand the pain that you speak of. I too agree that
every line of changed code should have a test confirming that behavior
was not changed.
I don't believe that we need to go as far as revoking committer rights
and reviewing each committer again, BUT it would be AMAZING that out of
our 100 committers, 80% of them would be more active in PR reviews,
mailing lists and in the end active on the project outside of their
focus area.
I do want to remind all Geode committers, it IS your responsibility to
be part of the PR review cycle. I will hold myself just as accountable
to this than what I hold every committer to, as I've been just as lazy
as the rest of them.
BUT
The reality is:
1. Geode code is HUGELY complex and NOT a test complete as we'd like
2. In the past single small changes have caused failures the were
completely unforeseen by anyone
3. In the past commits with single reviewers, have caused backward
compatibility issues which were only caught by chance in unrelated
testing.
4. There are 100 committers on Geode, and we keep on arguing that it is
hard to get PR's reviewed and that is why it is ok to have only 1
reviewer per PR.
5. There seems to be majority (unstated) opinion of: "why change, it
has been working for us so far." (I call is unstated, because being
silent means you agree with the status quo)
6. With requiring only 1 reviewer on code submissions, we are possibly
creating areas of the code only understood by a few.
IF, we as a project, have decided that all code shall enter only through
the flow of PR, then why not extend the QA cycle a little by requiring
more eyes. Lazy consensus, is as stated, lazy and would only work in a
project where the levels of complexity and size are not as high as
Geode's. In addition, with PR submissions, we have admitted that we are
human and could make mistakes and in an already complex environment and
to the best of our ability get it wrong.
Now, there are commits that really do not require 3 pairs of eyes,
because spelling mistakes and typos don't need consensus. But any time
code logic was amended, this needs to be reviewed.
I have seen different approach to code submissions:
* The submitter of the PR is NOT the committer of the PR. This task is
the responsibility of another committer(s) to review, approve and
finally merge in.
* Smaller amount of committers with higher numbers of contributors.
Yes, this does create a bottleneck, but it promotes a sense of pride
and responsibility that individual feels. Possibly a greater
understanding of the target module is promoted through this approach
as well.
Now, I don't say we as a project should follow these strict or
restricting approaches, but from my perspective, if we as a project
argue that we struggle to find 3 reviewers out of 100, then there are
bigger problems in the project than we anticipated. It is not a lack of
trust in our committers, to me it is a sense of pride that I want other
committers to confirm that I've delivered code to the high standard that
we want to be known for. Whilst it is painful to go through the process,
but if done correctly it is beneficial to all involved, as differing
opinions and approaches can be shared and all can learn from.
In addition, I have personally stumbled upon a few PR's, which upon
review found to be lacking in the areas of best practices of code and/or
design.
I fully support the notion of 3 reviewers per PR. I'm also going to take
it one step further, in the list of reviewers, there is at least 1
reviewer that is not part of a team, as this might drive a unbiased view
of the code and approach. I would also like to encourage ALL committers
to review code outside of the focus area. This will only promote a
broader understanding of the project codebase. I also support the notion
of a pair/mobbing reviews, if a reviewer does not understand the problem
area enough to effectively review, OR where the solution is not apparent.
--Udo
On 6/4/19 16:49, Kirk Lund wrote:
> I'm -1 for requiring N reviews before merging a commit.
>
> Overall, I support Lazy Consensus. If I post a PR that fixes the flakiness
> in a test, the precheckin jobs prove it, and it sits there for 2 weeks
> without reviews, then I favor merging it in at that point without any
> reviews. I'm not going to chase people around or spam the dev list over and
> over asking for reviews. Nothing in the Apache Way says you have to do
> reviews before committing -- some projects prefer "commit then review"
> instead of "review then commit". You can always look at the code someone
> changed and you can always change it further or revert it.
>
> I think if we don't trust our committers then we have a bigger systemic
> problem that becoming more strict about PR reviews isn not going to fix.
>
> Overall, I also favor pairing/mobbing over reviews. Without being there
> during the work, a reviewer lacks the context to understand why it was done
> the way it was done.
>
> If we cannot establish or maintain trust in committers, then I think we
> should remove committer status from everyone and start over as a project,
> proposing and accepting one committer at a time.
>
> Instead of constraints on reviews, I would prefer to establish new criteria
> for coding such as:
> 1) all classes touched in a PR must have a unit test created if none exists
> 2) all code touched in a PR must have unit test coverage (and possibly
> integration test coverage) specific to the changes
> 3) all new classes must have full unit test coverage
> 4) all code touched in a PR must follow clean code principles (which would
> obviously need defining on the wiki)
>
> Then it becomes the responsibility of the author(s) and committer(s) of
> that PR to ensure that the code and the PR follows the project's criteria
> for code quality and test coverage. It also becomes easier to measure the
> PRs of a non-committer to determine if we think they would make a good
> committer (for example, do they adhere to clean code quality and unit
> testing with mocks? -- along with any other criteria).
>
> On Thu, May 30, 2019 at 3:51 PM Owen Nichols <on...@pivotal.io> wrote:
>
>> It seems common for Geode PRs to get merged with only a single green
>> checkmark in GitHub.
>>
>> According to https://www.apache.org/foundation/voting.html we should not
>> be merging PRs with fewer than 3 green checkmarks.
>>
>> Consensus is a fundamental value in doing things The Apache Way. A single
>> +1 is not consensus. Since we’re currently discussing what it takes to
>> become a committer and what standards a committer is expected to uphold, it
>> seems like a good time to review this policy.
>>
>> GitHub can be configured to require N reviews before a commit can be
>> merged. Should we enable this feature?
>>
>> -Owen
>> VOTES ON CODE MODIFICATION <
>> https://www.apache.org/foundation/voting.html#votes-on-code-modification>
>> For code-modification votes, +1 votes are in favour of the proposal, but
>> -1 votes are vetos <https://www.apache.org/foundation/voting.html#Veto>
>> and kill the proposal dead until all vetoers withdraw their -1 votes.
>>
>> Unless a vote has been declared as using lazy consensus <
>> https://www.apache.org/foundation/voting.html#LazyConsensus> , three +1
>> votes are required for a code-modification proposal to pass.
>>
>> Whole numbers are recommended for this type of vote, as the opinion being
>> expressed is Boolean: 'I approve/do not approve of this change.'
>>
>>
>> CONSENSUS GAUGING THROUGH SILENCE <
>> https://www.apache.org/foundation/voting.html#LazyConsensus>
>> An alternative to voting that is sometimes used to measure the
>> acceptability of something is the concept of lazy consensus <
>> https://www.apache.org/foundation/glossary.html#LazyConsensus>.
>>
>> Lazy consensus is simply an announcement of 'silence gives assent.’ When
>> someone wants to determine the sense of the community this way, it might do
>> so with a mail message such as:
>> "The patch below fixes GEODE-12345; if no-one objects within three days,
>> I'll assume lazy consensus and commit it."
>>
>> Lazy consensus cannot be used on projects that enforce a
>> review-then-commit <
>> https://www.apache.org/foundation/glossary.html#ReviewThenCommit> policy.
>>
>>
>>
Re: [DISCUSS] require reviews before merging a PR
Posted by Alberto Bustamante Reyes <al...@est.tech>.
my two cents (although Im a newcomer here):
I agree with Kirk in the point that if a PR is sent by a committer, if after "grace period" (one/two weeks?) no one reviewed it, the author could merge it. But of course he/she is free to wait for a review.
I was thinking about PRs made by contributors, as it is my case. A possible approach could be that if the author is a contributor, then at least one review is needed. Once that review is done, the reviewer should be free to decide between merging the change or, if the PR is still under the "grace period", keep it open waiting for other reviews. If the review is done after the "grace period", then the reviewer should merge the change.
And if a contributor PR has at least a review and the "grace period" ends, the author is allowed to ask someone for merge it, as it already has the green light from a committer.
Finally, I also agree on the point of increasing test coverage and follow clean code principles.
________________________________
De: Kirk Lund <kl...@apache.org>
Enviado: miércoles, 5 de junio de 2019 1:49:06
Para: geode
Asunto: Re: [DISCUSS] require reviews before merging a PR
I'm -1 for requiring N reviews before merging a commit.
Overall, I support Lazy Consensus. If I post a PR that fixes the flakiness
in a test, the precheckin jobs prove it, and it sits there for 2 weeks
without reviews, then I favor merging it in at that point without any
reviews. I'm not going to chase people around or spam the dev list over and
over asking for reviews. Nothing in the Apache Way says you have to do
reviews before committing -- some projects prefer "commit then review"
instead of "review then commit". You can always look at the code someone
changed and you can always change it further or revert it.
I think if we don't trust our committers then we have a bigger systemic
problem that becoming more strict about PR reviews isn not going to fix.
Overall, I also favor pairing/mobbing over reviews. Without being there
during the work, a reviewer lacks the context to understand why it was done
the way it was done.
If we cannot establish or maintain trust in committers, then I think we
should remove committer status from everyone and start over as a project,
proposing and accepting one committer at a time.
Instead of constraints on reviews, I would prefer to establish new criteria
for coding such as:
1) all classes touched in a PR must have a unit test created if none exists
2) all code touched in a PR must have unit test coverage (and possibly
integration test coverage) specific to the changes
3) all new classes must have full unit test coverage
4) all code touched in a PR must follow clean code principles (which would
obviously need defining on the wiki)
Then it becomes the responsibility of the author(s) and committer(s) of
that PR to ensure that the code and the PR follows the project's criteria
for code quality and test coverage. It also becomes easier to measure the
PRs of a non-committer to determine if we think they would make a good
committer (for example, do they adhere to clean code quality and unit
testing with mocks? -- along with any other criteria).
On Thu, May 30, 2019 at 3:51 PM Owen Nichols <on...@pivotal.io> wrote:
> It seems common for Geode PRs to get merged with only a single green
> checkmark in GitHub.
>
> According to https://www.apache.org/foundation/voting.html we should not
> be merging PRs with fewer than 3 green checkmarks.
>
> Consensus is a fundamental value in doing things The Apache Way. A single
> +1 is not consensus. Since we’re currently discussing what it takes to
> become a committer and what standards a committer is expected to uphold, it
> seems like a good time to review this policy.
>
> GitHub can be configured to require N reviews before a commit can be
> merged. Should we enable this feature?
>
> -Owen
> VOTES ON CODE MODIFICATION <
> https://www.apache.org/foundation/voting.html#votes-on-code-modification>
> For code-modification votes, +1 votes are in favour of the proposal, but
> -1 votes are vetos <https://www.apache.org/foundation/voting.html#Veto>
> and kill the proposal dead until all vetoers withdraw their -1 votes.
>
> Unless a vote has been declared as using lazy consensus <
> https://www.apache.org/foundation/voting.html#LazyConsensus> , three +1
> votes are required for a code-modification proposal to pass.
>
> Whole numbers are recommended for this type of vote, as the opinion being
> expressed is Boolean: 'I approve/do not approve of this change.'
>
>
> CONSENSUS GAUGING THROUGH SILENCE <
> https://www.apache.org/foundation/voting.html#LazyConsensus>
> An alternative to voting that is sometimes used to measure the
> acceptability of something is the concept of lazy consensus <
> https://www.apache.org/foundation/glossary.html#LazyConsensus>.
>
> Lazy consensus is simply an announcement of 'silence gives assent.’ When
> someone wants to determine the sense of the community this way, it might do
> so with a mail message such as:
> "The patch below fixes GEODE-12345; if no-one objects within three days,
> I'll assume lazy consensus and commit it."
>
> Lazy consensus cannot be used on projects that enforce a
> review-then-commit <
> https://www.apache.org/foundation/glossary.html#ReviewThenCommit> policy.
>
>
>
Re: [DISCUSS] require reviews before merging a PR
Posted by Kirk Lund <kl...@apache.org>.
I'm -1 for requiring N reviews before merging a commit.
Overall, I support Lazy Consensus. If I post a PR that fixes the flakiness
in a test, the precheckin jobs prove it, and it sits there for 2 weeks
without reviews, then I favor merging it in at that point without any
reviews. I'm not going to chase people around or spam the dev list over and
over asking for reviews. Nothing in the Apache Way says you have to do
reviews before committing -- some projects prefer "commit then review"
instead of "review then commit". You can always look at the code someone
changed and you can always change it further or revert it.
I think if we don't trust our committers then we have a bigger systemic
problem that becoming more strict about PR reviews isn not going to fix.
Overall, I also favor pairing/mobbing over reviews. Without being there
during the work, a reviewer lacks the context to understand why it was done
the way it was done.
If we cannot establish or maintain trust in committers, then I think we
should remove committer status from everyone and start over as a project,
proposing and accepting one committer at a time.
Instead of constraints on reviews, I would prefer to establish new criteria
for coding such as:
1) all classes touched in a PR must have a unit test created if none exists
2) all code touched in a PR must have unit test coverage (and possibly
integration test coverage) specific to the changes
3) all new classes must have full unit test coverage
4) all code touched in a PR must follow clean code principles (which would
obviously need defining on the wiki)
Then it becomes the responsibility of the author(s) and committer(s) of
that PR to ensure that the code and the PR follows the project's criteria
for code quality and test coverage. It also becomes easier to measure the
PRs of a non-committer to determine if we think they would make a good
committer (for example, do they adhere to clean code quality and unit
testing with mocks? -- along with any other criteria).
On Thu, May 30, 2019 at 3:51 PM Owen Nichols <on...@pivotal.io> wrote:
> It seems common for Geode PRs to get merged with only a single green
> checkmark in GitHub.
>
> According to https://www.apache.org/foundation/voting.html we should not
> be merging PRs with fewer than 3 green checkmarks.
>
> Consensus is a fundamental value in doing things The Apache Way. A single
> +1 is not consensus. Since we’re currently discussing what it takes to
> become a committer and what standards a committer is expected to uphold, it
> seems like a good time to review this policy.
>
> GitHub can be configured to require N reviews before a commit can be
> merged. Should we enable this feature?
>
> -Owen
> VOTES ON CODE MODIFICATION <
> https://www.apache.org/foundation/voting.html#votes-on-code-modification>
> For code-modification votes, +1 votes are in favour of the proposal, but
> -1 votes are vetos <https://www.apache.org/foundation/voting.html#Veto>
> and kill the proposal dead until all vetoers withdraw their -1 votes.
>
> Unless a vote has been declared as using lazy consensus <
> https://www.apache.org/foundation/voting.html#LazyConsensus> , three +1
> votes are required for a code-modification proposal to pass.
>
> Whole numbers are recommended for this type of vote, as the opinion being
> expressed is Boolean: 'I approve/do not approve of this change.'
>
>
> CONSENSUS GAUGING THROUGH SILENCE <
> https://www.apache.org/foundation/voting.html#LazyConsensus>
> An alternative to voting that is sometimes used to measure the
> acceptability of something is the concept of lazy consensus <
> https://www.apache.org/foundation/glossary.html#LazyConsensus>.
>
> Lazy consensus is simply an announcement of 'silence gives assent.’ When
> someone wants to determine the sense of the community this way, it might do
> so with a mail message such as:
> "The patch below fixes GEODE-12345; if no-one objects within three days,
> I'll assume lazy consensus and commit it."
>
> Lazy consensus cannot be used on projects that enforce a
> review-then-commit <
> https://www.apache.org/foundation/glossary.html#ReviewThenCommit> policy.
>
>
>