geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jens Deppe <jde...@pivotal.io>
Subject Re: [DISCUSS] require reviews before merging a PR
Date Thu, 06 Jun 2019 14:26:08 GMT
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 <udo@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 <udo@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.
> > >>>>
> > >>>>
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message