Re: [DISCUSS] require reviews before merging a PR

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