RTC clarification

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

RTC clarification

trkurc
Administrator
All, I was having a discussion with Mike Hogue - a recent contributor - off
list, and he had some questions about the review process. And largely the
root of the question was, if a non-committer reviews a patch or PR (which
Mike has spent some time doing), is that considered a "review"? I didn't
have the answers, so I went on a hunt for documentation. I started with the
Contributor Guide [1]. The guide describes reviewing, and calls out a
Reviewer role, but doesn't specifically point out that Reviewer is a
committer, just that a committer "can actively promote contributions into
the repository", and goes on to imply the non-committers can review.

Given this, I was unable to answer this question:
If a committer "X" submits a patch or PR, it is reviewed by a non-committer
"Y", does that review satisfy the RTC requirement, and "X" may merge in the
patch?

I found a related discussion on the email list in March [2], which I think
implies at least some of the community assumed the canonical review must be
by a committer. I also went back a bit to early days [3], where Benson
implied a much less "formal" review process.

What I'm hoping for is hopefully come to a consensus of what our project
expectations are and document in the Contributor Guide. My expectation was
that non-committers could review, similar to what Sean discussed on this
thread for Apache Gossip (incubating) [4]. However, I don't believe this is
the current consensus.

Thoughts?

Tony

1.
https://cwiki.apache.org/confluence/display/NIFI/Contributor+Guide#ContributorGuide-CodeReviewProcess
2.
https://mail-archives.apache.org/mod_mbox/nifi-dev/201703.mbox/%3CCALJK9a7onOKo%3DXtAEmL7PSBUEBEqOOhcT9WSz-RZaNxqV6q%3Dmg%40mail.gmail.com%3E
3.
https://mail-archives.apache.org/mod_mbox/nifi-dev/201501.mbox/%3CCALhtWkf67UpOe9W9HQ3RSn00xb_=C6ZMxN+_SEfWthpQrU5fsg@...%3E
4.
http://mail-archives.apache.org/mod_mbox/incubator-gossip-dev/201606.mbox/%3CCAN5cbe6P8aEEOLMA%2BPrfpQg9c_AWeSfvvmom8jAp%3Dk7wUpoVgQ%40mail.gmail.com%3E
Reply | Threaded
Open this post in threaded view
|

Re: RTC clarification

Joe Witt
It is undefined at this point and I agree we should reach consensus
and document it.

I am in favor making non-committer reviews binding.

Why do we do RTC:
- To help bring along new committers/grow the community
- To help promote quality by having peer reviews

Enabling non-committer reviews to be binding still allows both of
those to be true.

On Thu, Jul 6, 2017 at 10:10 PM, Tony Kurc <[hidden email]> wrote:

> All, I was having a discussion with Mike Hogue - a recent contributor - off
> list, and he had some questions about the review process. And largely the
> root of the question was, if a non-committer reviews a patch or PR (which
> Mike has spent some time doing), is that considered a "review"? I didn't
> have the answers, so I went on a hunt for documentation. I started with the
> Contributor Guide [1]. The guide describes reviewing, and calls out a
> Reviewer role, but doesn't specifically point out that Reviewer is a
> committer, just that a committer "can actively promote contributions into
> the repository", and goes on to imply the non-committers can review.
>
> Given this, I was unable to answer this question:
> If a committer "X" submits a patch or PR, it is reviewed by a non-committer
> "Y", does that review satisfy the RTC requirement, and "X" may merge in the
> patch?
>
> I found a related discussion on the email list in March [2], which I think
> implies at least some of the community assumed the canonical review must be
> by a committer. I also went back a bit to early days [3], where Benson
> implied a much less "formal" review process.
>
> What I'm hoping for is hopefully come to a consensus of what our project
> expectations are and document in the Contributor Guide. My expectation was
> that non-committers could review, similar to what Sean discussed on this
> thread for Apache Gossip (incubating) [4]. However, I don't believe this is
> the current consensus.
>
> Thoughts?
>
> Tony
>
> 1.
> https://cwiki.apache.org/confluence/display/NIFI/Contributor+Guide#ContributorGuide-CodeReviewProcess
> 2.
> https://mail-archives.apache.org/mod_mbox/nifi-dev/201703.mbox/%3CCALJK9a7onOKo%3DXtAEmL7PSBUEBEqOOhcT9WSz-RZaNxqV6q%3Dmg%40mail.gmail.com%3E
> 3.
> https://mail-archives.apache.org/mod_mbox/nifi-dev/201501.mbox/%3CCALhtWkf67UpOe9W9HQ3RSn00xb_=C6ZMxN+_SEfWthpQrU5fsg@...%3E
> 4.
> http://mail-archives.apache.org/mod_mbox/incubator-gossip-dev/201606.mbox/%3CCAN5cbe6P8aEEOLMA%2BPrfpQg9c_AWeSfvvmom8jAp%3Dk7wUpoVgQ%40mail.gmail.com%3E
Reply | Threaded
Open this post in threaded view
|

Re: RTC clarification

James Wing
We should definitely encourage review feedback from non-committers.
Getting additional perspectives, interest, and enthusiasm from users is
critical for any project, doubly so for an integrating application where
committers cannot be experts in all the systems we are integrating with.  I
believe NiFi could use more review bandwidth.  Are missing out on potential
reviewers because of the current policy?

I do not have any experience with non-committer "binding reviews" as
described in the Apache Gossip thread.  How would that work?  Wouldn't a
committer have to review the review and decide to commit?  If we knew the
reviewer well enough to accept their judgement, why not make them a
committer?

My expectation is that many non-committer reviews are helpful and
constructive, but not necessarily 100% comprehensive.  Reviewers might
comment on the JIRA ticket without working with the PR, or try the proposed
change without reviewing the code, tests, etc.  All great stuff, but
backstopped by committers.

Thanks,

James

On Thu, Jul 6, 2017 at 7:30 PM, Joe Witt <[hidden email]> wrote:

> It is undefined at this point and I agree we should reach consensus
> and document it.
>
> I am in favor making non-committer reviews binding.
>
> Why do we do RTC:
> - To help bring along new committers/grow the community
> - To help promote quality by having peer reviews
>
> Enabling non-committer reviews to be binding still allows both of
> those to be true.
>
> On Thu, Jul 6, 2017 at 10:10 PM, Tony Kurc <[hidden email]> wrote:
> > All, I was having a discussion with Mike Hogue - a recent contributor -
> off
> > list, and he had some questions about the review process. And largely the
> > root of the question was, if a non-committer reviews a patch or PR (which
> > Mike has spent some time doing), is that considered a "review"? I didn't
> > have the answers, so I went on a hunt for documentation. I started with
> the
> > Contributor Guide [1]. The guide describes reviewing, and calls out a
> > Reviewer role, but doesn't specifically point out that Reviewer is a
> > committer, just that a committer "can actively promote contributions into
> > the repository", and goes on to imply the non-committers can review.
> >
> > Given this, I was unable to answer this question:
> > If a committer "X" submits a patch or PR, it is reviewed by a
> non-committer
> > "Y", does that review satisfy the RTC requirement, and "X" may merge in
> the
> > patch?
> >
> > I found a related discussion on the email list in March [2], which I
> think
> > implies at least some of the community assumed the canonical review must
> be
> > by a committer. I also went back a bit to early days [3], where Benson
> > implied a much less "formal" review process.
> >
> > What I'm hoping for is hopefully come to a consensus of what our project
> > expectations are and document in the Contributor Guide. My expectation
> was
> > that non-committers could review, similar to what Sean discussed on this
> > thread for Apache Gossip (incubating) [4]. However, I don't believe this
> is
> > the current consensus.
> >
> > Thoughts?
> >
> > Tony
> >
> > 1.
> > https://cwiki.apache.org/confluence/display/NIFI/Contributor
> +Guide#ContributorGuide-CodeReviewProcess
> > 2.
> > https://mail-archives.apache.org/mod_mbox/nifi-dev/201703.mb
> ox/%3CCALJK9a7onOKo%3DXtAEmL7PSBUEBEqOOhcT9WSz-RZaNxqV6q%
> 3Dmg%40mail.gmail.com%3E
> > 3.
> > https://mail-archives.apache.org/mod_mbox/nifi-dev/201501.mb
> ox/%3CCALhtWkf67UpOe9W9HQ3RSn00xb_=C6ZMxN+_SEfWthpQrU5fsg@
> mail.gmail.com%3E
> > 4.
> > http://mail-archives.apache.org/mod_mbox/incubator-gossip-de
> v/201606.mbox/%3CCAN5cbe6P8aEEOLMA%2BPrfpQg9c_AWeSfvvmom8jAp%3Dk7wUpoVgQ%
> 40mail.gmail.com%3E
>
Reply | Threaded
Open this post in threaded view
|

Re: RTC clarification

Michael Hogue
Thanks for fielding the question, Tony.

Joe and James' statements both make sense. I suppose a case by case
analysis could be carried out, too. For example, since I'm mostly
unfamiliar with the code base but am looking to gain familiarity, I'm
reviewing pretty straightforward or trivial PRs. My plan was to continue
doing that until I felt comfortable reviewing something with a larger
impact, such as the new TCPListenRecord processor implementation [1].
However, as Tony explained, my original question was whether that sort of
review would be binding or whether I should be doing it at all. I think
both of those questions were answered here in that ultimately committer
sign off is needed, but reviews may be binding regardless of source.

Thanks for the feedback!

Mike


[1] https://github.com/apache/nifi/pull/1987
On Fri, Jul 7, 2017 at 01:14 James Wing <[hidden email]> wrote:

> We should definitely encourage review feedback from non-committers.
> Getting additional perspectives, interest, and enthusiasm from users is
> critical for any project, doubly so for an integrating application where
> committers cannot be experts in all the systems we are integrating with.  I
> believe NiFi could use more review bandwidth.  Are missing out on potential
> reviewers because of the current policy?
>
> I do not have any experience with non-committer "binding reviews" as
> described in the Apache Gossip thread.  How would that work?  Wouldn't a
> committer have to review the review and decide to commit?  If we knew the
> reviewer well enough to accept their judgement, why not make them a
> committer?
>
> My expectation is that many non-committer reviews are helpful and
> constructive, but not necessarily 100% comprehensive.  Reviewers might
> comment on the JIRA ticket without working with the PR, or try the proposed
> change without reviewing the code, tests, etc.  All great stuff, but
> backstopped by committers.
>
> Thanks,
>
> James
>
> On Thu, Jul 6, 2017 at 7:30 PM, Joe Witt <[hidden email]> wrote:
>
> > It is undefined at this point and I agree we should reach consensus
> > and document it.
> >
> > I am in favor making non-committer reviews binding.
> >
> > Why do we do RTC:
> > - To help bring along new committers/grow the community
> > - To help promote quality by having peer reviews
> >
> > Enabling non-committer reviews to be binding still allows both of
> > those to be true.
> >
> > On Thu, Jul 6, 2017 at 10:10 PM, Tony Kurc <[hidden email]> wrote:
> > > All, I was having a discussion with Mike Hogue - a recent contributor -
> > off
> > > list, and he had some questions about the review process. And largely
> the
> > > root of the question was, if a non-committer reviews a patch or PR
> (which
> > > Mike has spent some time doing), is that considered a "review"? I
> didn't
> > > have the answers, so I went on a hunt for documentation. I started with
> > the
> > > Contributor Guide [1]. The guide describes reviewing, and calls out a
> > > Reviewer role, but doesn't specifically point out that Reviewer is a
> > > committer, just that a committer "can actively promote contributions
> into
> > > the repository", and goes on to imply the non-committers can review.
> > >
> > > Given this, I was unable to answer this question:
> > > If a committer "X" submits a patch or PR, it is reviewed by a
> > non-committer
> > > "Y", does that review satisfy the RTC requirement, and "X" may merge in
> > the
> > > patch?
> > >
> > > I found a related discussion on the email list in March [2], which I
> > think
> > > implies at least some of the community assumed the canonical review
> must
> > be
> > > by a committer. I also went back a bit to early days [3], where Benson
> > > implied a much less "formal" review process.
> > >
> > > What I'm hoping for is hopefully come to a consensus of what our
> project
> > > expectations are and document in the Contributor Guide. My expectation
> > was
> > > that non-committers could review, similar to what Sean discussed on
> this
> > > thread for Apache Gossip (incubating) [4]. However, I don't believe
> this
> > is
> > > the current consensus.
> > >
> > > Thoughts?
> > >
> > > Tony
> > >
> > > 1.
> > > https://cwiki.apache.org/confluence/display/NIFI/Contributor
> > +Guide#ContributorGuide-CodeReviewProcess
> > > 2.
> > > https://mail-archives.apache.org/mod_mbox/nifi-dev/201703.mb
> > ox/%3CCALJK9a7onOKo%3DXtAEmL7PSBUEBEqOOhcT9WSz-RZaNxqV6q%
> > 3Dmg%40mail.gmail.com%3E
> > > 3.
> > > https://mail-archives.apache.org/mod_mbox/nifi-dev/201501.mb
> > ox/%3CCALhtWkf67UpOe9W9HQ3RSn00xb_=C6ZMxN+_SEfWthpQrU5fsg@
> > mail.gmail.com%3E
> > > 4.
> > > http://mail-archives.apache.org/mod_mbox/incubator-gossip-de
> > v/201606.mbox/%3CCAN5cbe6P8aEEOLMA%2BPrfpQg9c_AWeSfvvmom8jAp%3Dk7wUpoVgQ%
> > 40mail.gmail.com%3E
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: RTC clarification

Bryan Bende
I agree with encouraging reviews from everyone, but I lean towards
"binding" reviews coming from committers.

If we allow any review to be binding, there could be completely
different levels of review that occur...

There could be someone who isn't a committer yet, but has been
contributing already and will probably do a very thorough review of
someone's contribution, and there could be someone else who we never
interacted with us before and writes "+1 LGTM" on a PR and we have no
idea if they know what they are talking about or if they even tried
the contribution to see if it works. Obviously a committer can also
write "+1 LGTM", but I think when that comes from a committer it holds
more weight.

I think we may also want to clarify if we are only talking about
"submitted by committer, reviewed by non-committer" or also talking
about "submitted by non-committer, reviewed by non-committer".

For the first case I can see the argument that since the contribution
is from a committer who is already trusted, they can make the right
judgement call based on the review. Where as in the second case, just
because a community member submitted something and another community
member says it looks good, doesn't necessarily mean a committer should
come along and automatically merge it in.


On Fri, Jul 7, 2017 at 4:13 AM, Michael Hogue
<[hidden email]> wrote:

> Thanks for fielding the question, Tony.
>
> Joe and James' statements both make sense. I suppose a case by case
> analysis could be carried out, too. For example, since I'm mostly
> unfamiliar with the code base but am looking to gain familiarity, I'm
> reviewing pretty straightforward or trivial PRs. My plan was to continue
> doing that until I felt comfortable reviewing something with a larger
> impact, such as the new TCPListenRecord processor implementation [1].
> However, as Tony explained, my original question was whether that sort of
> review would be binding or whether I should be doing it at all. I think
> both of those questions were answered here in that ultimately committer
> sign off is needed, but reviews may be binding regardless of source.
>
> Thanks for the feedback!
>
> Mike
>
>
> [1] https://github.com/apache/nifi/pull/1987
> On Fri, Jul 7, 2017 at 01:14 James Wing <[hidden email]> wrote:
>
>> We should definitely encourage review feedback from non-committers.
>> Getting additional perspectives, interest, and enthusiasm from users is
>> critical for any project, doubly so for an integrating application where
>> committers cannot be experts in all the systems we are integrating with.  I
>> believe NiFi could use more review bandwidth.  Are missing out on potential
>> reviewers because of the current policy?
>>
>> I do not have any experience with non-committer "binding reviews" as
>> described in the Apache Gossip thread.  How would that work?  Wouldn't a
>> committer have to review the review and decide to commit?  If we knew the
>> reviewer well enough to accept their judgement, why not make them a
>> committer?
>>
>> My expectation is that many non-committer reviews are helpful and
>> constructive, but not necessarily 100% comprehensive.  Reviewers might
>> comment on the JIRA ticket without working with the PR, or try the proposed
>> change without reviewing the code, tests, etc.  All great stuff, but
>> backstopped by committers.
>>
>> Thanks,
>>
>> James
>>
>> On Thu, Jul 6, 2017 at 7:30 PM, Joe Witt <[hidden email]> wrote:
>>
>> > It is undefined at this point and I agree we should reach consensus
>> > and document it.
>> >
>> > I am in favor making non-committer reviews binding.
>> >
>> > Why do we do RTC:
>> > - To help bring along new committers/grow the community
>> > - To help promote quality by having peer reviews
>> >
>> > Enabling non-committer reviews to be binding still allows both of
>> > those to be true.
>> >
>> > On Thu, Jul 6, 2017 at 10:10 PM, Tony Kurc <[hidden email]> wrote:
>> > > All, I was having a discussion with Mike Hogue - a recent contributor -
>> > off
>> > > list, and he had some questions about the review process. And largely
>> the
>> > > root of the question was, if a non-committer reviews a patch or PR
>> (which
>> > > Mike has spent some time doing), is that considered a "review"? I
>> didn't
>> > > have the answers, so I went on a hunt for documentation. I started with
>> > the
>> > > Contributor Guide [1]. The guide describes reviewing, and calls out a
>> > > Reviewer role, but doesn't specifically point out that Reviewer is a
>> > > committer, just that a committer "can actively promote contributions
>> into
>> > > the repository", and goes on to imply the non-committers can review.
>> > >
>> > > Given this, I was unable to answer this question:
>> > > If a committer "X" submits a patch or PR, it is reviewed by a
>> > non-committer
>> > > "Y", does that review satisfy the RTC requirement, and "X" may merge in
>> > the
>> > > patch?
>> > >
>> > > I found a related discussion on the email list in March [2], which I
>> > think
>> > > implies at least some of the community assumed the canonical review
>> must
>> > be
>> > > by a committer. I also went back a bit to early days [3], where Benson
>> > > implied a much less "formal" review process.
>> > >
>> > > What I'm hoping for is hopefully come to a consensus of what our
>> project
>> > > expectations are and document in the Contributor Guide. My expectation
>> > was
>> > > that non-committers could review, similar to what Sean discussed on
>> this
>> > > thread for Apache Gossip (incubating) [4]. However, I don't believe
>> this
>> > is
>> > > the current consensus.
>> > >
>> > > Thoughts?
>> > >
>> > > Tony
>> > >
>> > > 1.
>> > > https://cwiki.apache.org/confluence/display/NIFI/Contributor
>> > +Guide#ContributorGuide-CodeReviewProcess
>> > > 2.
>> > > https://mail-archives.apache.org/mod_mbox/nifi-dev/201703.mb
>> > ox/%3CCALJK9a7onOKo%3DXtAEmL7PSBUEBEqOOhcT9WSz-RZaNxqV6q%
>> > 3Dmg%40mail.gmail.com%3E
>> > > 3.
>> > > https://mail-archives.apache.org/mod_mbox/nifi-dev/201501.mb
>> > ox/%3CCALhtWkf67UpOe9W9HQ3RSn00xb_=C6ZMxN+_SEfWthpQrU5fsg@
>> > mail.gmail.com%3E
>> > > 4.
>> > > http://mail-archives.apache.org/mod_mbox/incubator-gossip-de
>> > v/201606.mbox/%3CCAN5cbe6P8aEEOLMA%2BPrfpQg9c_AWeSfvvmom8jAp%3Dk7wUpoVgQ%
>> > 40mail.gmail.com%3E
>> >
>>
Reply | Threaded
Open this post in threaded view
|

Re: RTC clarification

Brandon DeVries
There are always exceptions, but I think the best way to ensure that the
spirit of what we're going for is being followed is to say "no one commits
their own code".  While additional eyes are never going to be a bad thing,
requiring a second person to "sign off" on and then commit the code would
seem to help keep the writing and reviewing steps separate, as we want them
to be.

Secondarily, with no judgement attached to the statement, a review from
someone who's learning the code base might not have the same depth of
understanding and awareness of context as a review from someone with more
experience on the project.  I would think for a review to be trusted, we
would need a certain level of trust in the reviewer... and the best current
process we have for that is committer status.  Like James said, "If we knew
the reviewer well enough to accept their judgement, why not make them a
committer?"

Brandon

On Fri, Jul 7, 2017 at 9:24 AM Bryan Bende <[hidden email]> wrote:

> I agree with encouraging reviews from everyone, but I lean towards
> "binding" reviews coming from committers.
>
> If we allow any review to be binding, there could be completely
> different levels of review that occur...
>
> There could be someone who isn't a committer yet, but has been
> contributing already and will probably do a very thorough review of
> someone's contribution, and there could be someone else who we never
> interacted with us before and writes "+1 LGTM" on a PR and we have no
> idea if they know what they are talking about or if they even tried
> the contribution to see if it works. Obviously a committer can also
> write "+1 LGTM", but I think when that comes from a committer it holds
> more weight.
>
> I think we may also want to clarify if we are only talking about
> "submitted by committer, reviewed by non-committer" or also talking
> about "submitted by non-committer, reviewed by non-committer".
>
> For the first case I can see the argument that since the contribution
> is from a committer who is already trusted, they can make the right
> judgement call based on the review. Where as in the second case, just
> because a community member submitted something and another community
> member says it looks good, doesn't necessarily mean a committer should
> come along and automatically merge it in.
>
>
> On Fri, Jul 7, 2017 at 4:13 AM, Michael Hogue
> <[hidden email]> wrote:
> > Thanks for fielding the question, Tony.
> >
> > Joe and James' statements both make sense. I suppose a case by case
> > analysis could be carried out, too. For example, since I'm mostly
> > unfamiliar with the code base but am looking to gain familiarity, I'm
> > reviewing pretty straightforward or trivial PRs. My plan was to continue
> > doing that until I felt comfortable reviewing something with a larger
> > impact, such as the new TCPListenRecord processor implementation [1].
> > However, as Tony explained, my original question was whether that sort of
> > review would be binding or whether I should be doing it at all. I think
> > both of those questions were answered here in that ultimately committer
> > sign off is needed, but reviews may be binding regardless of source.
> >
> > Thanks for the feedback!
> >
> > Mike
> >
> >
> > [1] https://github.com/apache/nifi/pull/1987
> > On Fri, Jul 7, 2017 at 01:14 James Wing <[hidden email]> wrote:
> >
> >> We should definitely encourage review feedback from non-committers.
> >> Getting additional perspectives, interest, and enthusiasm from users is
> >> critical for any project, doubly so for an integrating application where
> >> committers cannot be experts in all the systems we are integrating
> with.  I
> >> believe NiFi could use more review bandwidth.  Are missing out on
> potential
> >> reviewers because of the current policy?
> >>
> >> I do not have any experience with non-committer "binding reviews" as
> >> described in the Apache Gossip thread.  How would that work?  Wouldn't a
> >> committer have to review the review and decide to commit?  If we knew
> the
> >> reviewer well enough to accept their judgement, why not make them a
> >> committer?
> >>
> >> My expectation is that many non-committer reviews are helpful and
> >> constructive, but not necessarily 100% comprehensive.  Reviewers might
> >> comment on the JIRA ticket without working with the PR, or try the
> proposed
> >> change without reviewing the code, tests, etc.  All great stuff, but
> >> backstopped by committers.
> >>
> >> Thanks,
> >>
> >> James
> >>
> >> On Thu, Jul 6, 2017 at 7:30 PM, Joe Witt <[hidden email]> wrote:
> >>
> >> > It is undefined at this point and I agree we should reach consensus
> >> > and document it.
> >> >
> >> > I am in favor making non-committer reviews binding.
> >> >
> >> > Why do we do RTC:
> >> > - To help bring along new committers/grow the community
> >> > - To help promote quality by having peer reviews
> >> >
> >> > Enabling non-committer reviews to be binding still allows both of
> >> > those to be true.
> >> >
> >> > On Thu, Jul 6, 2017 at 10:10 PM, Tony Kurc <[hidden email]> wrote:
> >> > > All, I was having a discussion with Mike Hogue - a recent
> contributor -
> >> > off
> >> > > list, and he had some questions about the review process. And
> largely
> >> the
> >> > > root of the question was, if a non-committer reviews a patch or PR
> >> (which
> >> > > Mike has spent some time doing), is that considered a "review"? I
> >> didn't
> >> > > have the answers, so I went on a hunt for documentation. I started
> with
> >> > the
> >> > > Contributor Guide [1]. The guide describes reviewing, and calls out
> a
> >> > > Reviewer role, but doesn't specifically point out that Reviewer is a
> >> > > committer, just that a committer "can actively promote contributions
> >> into
> >> > > the repository", and goes on to imply the non-committers can review.
> >> > >
> >> > > Given this, I was unable to answer this question:
> >> > > If a committer "X" submits a patch or PR, it is reviewed by a
> >> > non-committer
> >> > > "Y", does that review satisfy the RTC requirement, and "X" may
> merge in
> >> > the
> >> > > patch?
> >> > >
> >> > > I found a related discussion on the email list in March [2], which I
> >> > think
> >> > > implies at least some of the community assumed the canonical review
> >> must
> >> > be
> >> > > by a committer. I also went back a bit to early days [3], where
> Benson
> >> > > implied a much less "formal" review process.
> >> > >
> >> > > What I'm hoping for is hopefully come to a consensus of what our
> >> project
> >> > > expectations are and document in the Contributor Guide. My
> expectation
> >> > was
> >> > > that non-committers could review, similar to what Sean discussed on
> >> this
> >> > > thread for Apache Gossip (incubating) [4]. However, I don't believe
> >> this
> >> > is
> >> > > the current consensus.
> >> > >
> >> > > Thoughts?
> >> > >
> >> > > Tony
> >> > >
> >> > > 1.
> >> > > https://cwiki.apache.org/confluence/display/NIFI/Contributor
> >> > +Guide#ContributorGuide-CodeReviewProcess
> >> > > 2.
> >> > > https://mail-archives.apache.org/mod_mbox/nifi-dev/201703.mb
> >> > ox/%3CCALJK9a7onOKo%3DXtAEmL7PSBUEBEqOOhcT9WSz-RZaNxqV6q%
> >> > 3Dmg%40mail.gmail.com%3E
> >> > > 3.
> >> > > https://mail-archives.apache.org/mod_mbox/nifi-dev/201501.mb
> >> > ox/%3CCALhtWkf67UpOe9W9HQ3RSn00xb_=C6ZMxN+_SEfWthpQrU5fsg@
> >> > mail.gmail.com%3E
> >> > > 4.
> >> > > http://mail-archives.apache.org/mod_mbox/incubator-gossip-de
> >> >
> v/201606.mbox/%3CCAN5cbe6P8aEEOLMA%2BPrfpQg9c_AWeSfvvmom8jAp%3Dk7wUpoVgQ%
> >> > 40mail.gmail.com%3E
> >> >
> >>
>
Reply | Threaded
Open this post in threaded view
|

Re: RTC clarification

Joe Skora
In reply to this post by Bryan Bende
I agree that non-committer review make more sense if the reviewer has
established some level of credibility and involvement in the community.

Ultimately, the final committer has to be a community member and they will
have final responsibility for the code being Apache compliant and NiFi
ready, so even reviews by less well known members of the community should
still be low risk.

On Fri, Jul 7, 2017 at 9:24 AM, Bryan Bende <[hidden email]> wrote:

> I agree with encouraging reviews from everyone, but I lean towards
> "binding" reviews coming from committers.
>
> If we allow any review to be binding, there could be completely
> different levels of review that occur...
>
> There could be someone who isn't a committer yet, but has been
> contributing already and will probably do a very thorough review of
> someone's contribution, and there could be someone else who we never
> interacted with us before and writes "+1 LGTM" on a PR and we have no
> idea if they know what they are talking about or if they even tried
> the contribution to see if it works. Obviously a committer can also
> write "+1 LGTM", but I think when that comes from a committer it holds
> more weight.
>
> I think we may also want to clarify if we are only talking about
> "submitted by committer, reviewed by non-committer" or also talking
> about "submitted by non-committer, reviewed by non-committer".
>
> For the first case I can see the argument that since the contribution
> is from a committer who is already trusted, they can make the right
> judgement call based on the review. Where as in the second case, just
> because a community member submitted something and another community
> member says it looks good, doesn't necessarily mean a committer should
> come along and automatically merge it in.
>
>
> On Fri, Jul 7, 2017 at 4:13 AM, Michael Hogue
> <[hidden email]> wrote:
> > Thanks for fielding the question, Tony.
> >
> > Joe and James' statements both make sense. I suppose a case by case
> > analysis could be carried out, too. For example, since I'm mostly
> > unfamiliar with the code base but am looking to gain familiarity, I'm
> > reviewing pretty straightforward or trivial PRs. My plan was to continue
> > doing that until I felt comfortable reviewing something with a larger
> > impact, such as the new TCPListenRecord processor implementation [1].
> > However, as Tony explained, my original question was whether that sort of
> > review would be binding or whether I should be doing it at all. I think
> > both of those questions were answered here in that ultimately committer
> > sign off is needed, but reviews may be binding regardless of source.
> >
> > Thanks for the feedback!
> >
> > Mike
> >
> >
> > [1] https://github.com/apache/nifi/pull/1987
> > On Fri, Jul 7, 2017 at 01:14 James Wing <[hidden email]> wrote:
> >
> >> We should definitely encourage review feedback from non-committers.
> >> Getting additional perspectives, interest, and enthusiasm from users is
> >> critical for any project, doubly so for an integrating application where
> >> committers cannot be experts in all the systems we are integrating
> with.  I
> >> believe NiFi could use more review bandwidth.  Are missing out on
> potential
> >> reviewers because of the current policy?
> >>
> >> I do not have any experience with non-committer "binding reviews" as
> >> described in the Apache Gossip thread.  How would that work?  Wouldn't a
> >> committer have to review the review and decide to commit?  If we knew
> the
> >> reviewer well enough to accept their judgement, why not make them a
> >> committer?
> >>
> >> My expectation is that many non-committer reviews are helpful and
> >> constructive, but not necessarily 100% comprehensive.  Reviewers might
> >> comment on the JIRA ticket without working with the PR, or try the
> proposed
> >> change without reviewing the code, tests, etc.  All great stuff, but
> >> backstopped by committers.
> >>
> >> Thanks,
> >>
> >> James
> >>
> >> On Thu, Jul 6, 2017 at 7:30 PM, Joe Witt <[hidden email]> wrote:
> >>
> >> > It is undefined at this point and I agree we should reach consensus
> >> > and document it.
> >> >
> >> > I am in favor making non-committer reviews binding.
> >> >
> >> > Why do we do RTC:
> >> > - To help bring along new committers/grow the community
> >> > - To help promote quality by having peer reviews
> >> >
> >> > Enabling non-committer reviews to be binding still allows both of
> >> > those to be true.
> >> >
> >> > On Thu, Jul 6, 2017 at 10:10 PM, Tony Kurc <[hidden email]> wrote:
> >> > > All, I was having a discussion with Mike Hogue - a recent
> contributor -
> >> > off
> >> > > list, and he had some questions about the review process. And
> largely
> >> the
> >> > > root of the question was, if a non-committer reviews a patch or PR
> >> (which
> >> > > Mike has spent some time doing), is that considered a "review"? I
> >> didn't
> >> > > have the answers, so I went on a hunt for documentation. I started
> with
> >> > the
> >> > > Contributor Guide [1]. The guide describes reviewing, and calls out
> a
> >> > > Reviewer role, but doesn't specifically point out that Reviewer is a
> >> > > committer, just that a committer "can actively promote contributions
> >> into
> >> > > the repository", and goes on to imply the non-committers can review.
> >> > >
> >> > > Given this, I was unable to answer this question:
> >> > > If a committer "X" submits a patch or PR, it is reviewed by a
> >> > non-committer
> >> > > "Y", does that review satisfy the RTC requirement, and "X" may
> merge in
> >> > the
> >> > > patch?
> >> > >
> >> > > I found a related discussion on the email list in March [2], which I
> >> > think
> >> > > implies at least some of the community assumed the canonical review
> >> must
> >> > be
> >> > > by a committer. I also went back a bit to early days [3], where
> Benson
> >> > > implied a much less "formal" review process.
> >> > >
> >> > > What I'm hoping for is hopefully come to a consensus of what our
> >> project
> >> > > expectations are and document in the Contributor Guide. My
> expectation
> >> > was
> >> > > that non-committers could review, similar to what Sean discussed on
> >> this
> >> > > thread for Apache Gossip (incubating) [4]. However, I don't believe
> >> this
> >> > is
> >> > > the current consensus.
> >> > >
> >> > > Thoughts?
> >> > >
> >> > > Tony
> >> > >
> >> > > 1.
> >> > > https://cwiki.apache.org/confluence/display/NIFI/Contributor
> >> > +Guide#ContributorGuide-CodeReviewProcess
> >> > > 2.
> >> > > https://mail-archives.apache.org/mod_mbox/nifi-dev/201703.mb
> >> > ox/%3CCALJK9a7onOKo%3DXtAEmL7PSBUEBEqOOhcT9WSz-RZaNxqV6q%
> >> > 3Dmg%40mail.gmail.com%3E
> >> > > 3.
> >> > > https://mail-archives.apache.org/mod_mbox/nifi-dev/201501.mb
> >> > ox/%3CCALhtWkf67UpOe9W9HQ3RSn00xb_=C6ZMxN+_SEfWthpQrU5fsg@
> >> > mail.gmail.com%3E
> >> > > 4.
> >> > > http://mail-archives.apache.org/mod_mbox/incubator-gossip-de
> >> > v/201606.mbox/%3CCAN5cbe6P8aEEOLMA%2BPrfpQg9c_AWeSfvvmom8jAp%
> 3Dk7wUpoVgQ%
> >> > 40mail.gmail.com%3E
> >> >
> >>
>
Reply | Threaded
Open this post in threaded view
|

Re: RTC clarification

Joe Witt
We're looking at a possible set of scenarios comprised of a
'submitter' and a 'reviewer'.  There can be one or more reviewers.  A
submitter or reviewer is either a committer or is not a committer. If
there is at least one reviewer that is a committer it is a committer
reviewed scenario.  So with that in mind...

A) Committer submitted, Non-Committer reviewed = This can be merged in
the event of a positive review outcome.

B) Committer submitted, Committer reviewed = This can be merged in the
event of a positive review outcome.

C) Non-Committer submitted, Non-Committer reviewed = This cannot be
merged until there is at least one positive committer review.
However, that review is made easier for the committer as someone who
is still building merit in the community has already done work here
and further this is a great chance to work with/grow those folks in
the community.

D) Non-committer submitted, Committer reviewed = This can be merged in
the event of a positive review outcome.

Scenario B and D is easy and probably there is no discussion needed.
Same for scenario C as there has to be at least one committer involved
for the code to get pushed in the first place.  I suspect it is only
scenario A where we have some 'yeah but...' sort of thoughts going on.

I am favorable to us allowing scenario A to result in a merge because
at the end of the day there is a committer involved, heavily involved
in fact, and they've already been voted by the community to be a
committer.  That is a high bar as it should be.

The risk of bad commits getting through needs to be balanced against
the risk of community growth being hampered by slow review cycles.
Let's be honest, we need the help with review bandwidth.  It is really
hard to keep up and to my mind one of the best ways to annoy/deter
contributors from advancing is to make the time between contribution
to feedback take a long time.


On Fri, Jul 7, 2017 at 9:58 AM, Joe Skora <[hidden email]> wrote:

> I agree that non-committer review make more sense if the reviewer has
> established some level of credibility and involvement in the community.
>
> Ultimately, the final committer has to be a community member and they will
> have final responsibility for the code being Apache compliant and NiFi
> ready, so even reviews by less well known members of the community should
> still be low risk.
>
> On Fri, Jul 7, 2017 at 9:24 AM, Bryan Bende <[hidden email]> wrote:
>
>> I agree with encouraging reviews from everyone, but I lean towards
>> "binding" reviews coming from committers.
>>
>> If we allow any review to be binding, there could be completely
>> different levels of review that occur...
>>
>> There could be someone who isn't a committer yet, but has been
>> contributing already and will probably do a very thorough review of
>> someone's contribution, and there could be someone else who we never
>> interacted with us before and writes "+1 LGTM" on a PR and we have no
>> idea if they know what they are talking about or if they even tried
>> the contribution to see if it works. Obviously a committer can also
>> write "+1 LGTM", but I think when that comes from a committer it holds
>> more weight.
>>
>> I think we may also want to clarify if we are only talking about
>> "submitted by committer, reviewed by non-committer" or also talking
>> about "submitted by non-committer, reviewed by non-committer".
>>
>> For the first case I can see the argument that since the contribution
>> is from a committer who is already trusted, they can make the right
>> judgement call based on the review. Where as in the second case, just
>> because a community member submitted something and another community
>> member says it looks good, doesn't necessarily mean a committer should
>> come along and automatically merge it in.
>>
>>
>> On Fri, Jul 7, 2017 at 4:13 AM, Michael Hogue
>> <[hidden email]> wrote:
>> > Thanks for fielding the question, Tony.
>> >
>> > Joe and James' statements both make sense. I suppose a case by case
>> > analysis could be carried out, too. For example, since I'm mostly
>> > unfamiliar with the code base but am looking to gain familiarity, I'm
>> > reviewing pretty straightforward or trivial PRs. My plan was to continue
>> > doing that until I felt comfortable reviewing something with a larger
>> > impact, such as the new TCPListenRecord processor implementation [1].
>> > However, as Tony explained, my original question was whether that sort of
>> > review would be binding or whether I should be doing it at all. I think
>> > both of those questions were answered here in that ultimately committer
>> > sign off is needed, but reviews may be binding regardless of source.
>> >
>> > Thanks for the feedback!
>> >
>> > Mike
>> >
>> >
>> > [1] https://github.com/apache/nifi/pull/1987
>> > On Fri, Jul 7, 2017 at 01:14 James Wing <[hidden email]> wrote:
>> >
>> >> We should definitely encourage review feedback from non-committers.
>> >> Getting additional perspectives, interest, and enthusiasm from users is
>> >> critical for any project, doubly so for an integrating application where
>> >> committers cannot be experts in all the systems we are integrating
>> with.  I
>> >> believe NiFi could use more review bandwidth.  Are missing out on
>> potential
>> >> reviewers because of the current policy?
>> >>
>> >> I do not have any experience with non-committer "binding reviews" as
>> >> described in the Apache Gossip thread.  How would that work?  Wouldn't a
>> >> committer have to review the review and decide to commit?  If we knew
>> the
>> >> reviewer well enough to accept their judgement, why not make them a
>> >> committer?
>> >>
>> >> My expectation is that many non-committer reviews are helpful and
>> >> constructive, but not necessarily 100% comprehensive.  Reviewers might
>> >> comment on the JIRA ticket without working with the PR, or try the
>> proposed
>> >> change without reviewing the code, tests, etc.  All great stuff, but
>> >> backstopped by committers.
>> >>
>> >> Thanks,
>> >>
>> >> James
>> >>
>> >> On Thu, Jul 6, 2017 at 7:30 PM, Joe Witt <[hidden email]> wrote:
>> >>
>> >> > It is undefined at this point and I agree we should reach consensus
>> >> > and document it.
>> >> >
>> >> > I am in favor making non-committer reviews binding.
>> >> >
>> >> > Why do we do RTC:
>> >> > - To help bring along new committers/grow the community
>> >> > - To help promote quality by having peer reviews
>> >> >
>> >> > Enabling non-committer reviews to be binding still allows both of
>> >> > those to be true.
>> >> >
>> >> > On Thu, Jul 6, 2017 at 10:10 PM, Tony Kurc <[hidden email]> wrote:
>> >> > > All, I was having a discussion with Mike Hogue - a recent
>> contributor -
>> >> > off
>> >> > > list, and he had some questions about the review process. And
>> largely
>> >> the
>> >> > > root of the question was, if a non-committer reviews a patch or PR
>> >> (which
>> >> > > Mike has spent some time doing), is that considered a "review"? I
>> >> didn't
>> >> > > have the answers, so I went on a hunt for documentation. I started
>> with
>> >> > the
>> >> > > Contributor Guide [1]. The guide describes reviewing, and calls out
>> a
>> >> > > Reviewer role, but doesn't specifically point out that Reviewer is a
>> >> > > committer, just that a committer "can actively promote contributions
>> >> into
>> >> > > the repository", and goes on to imply the non-committers can review.
>> >> > >
>> >> > > Given this, I was unable to answer this question:
>> >> > > If a committer "X" submits a patch or PR, it is reviewed by a
>> >> > non-committer
>> >> > > "Y", does that review satisfy the RTC requirement, and "X" may
>> merge in
>> >> > the
>> >> > > patch?
>> >> > >
>> >> > > I found a related discussion on the email list in March [2], which I
>> >> > think
>> >> > > implies at least some of the community assumed the canonical review
>> >> must
>> >> > be
>> >> > > by a committer. I also went back a bit to early days [3], where
>> Benson
>> >> > > implied a much less "formal" review process.
>> >> > >
>> >> > > What I'm hoping for is hopefully come to a consensus of what our
>> >> project
>> >> > > expectations are and document in the Contributor Guide. My
>> expectation
>> >> > was
>> >> > > that non-committers could review, similar to what Sean discussed on
>> >> this
>> >> > > thread for Apache Gossip (incubating) [4]. However, I don't believe
>> >> this
>> >> > is
>> >> > > the current consensus.
>> >> > >
>> >> > > Thoughts?
>> >> > >
>> >> > > Tony
>> >> > >
>> >> > > 1.
>> >> > > https://cwiki.apache.org/confluence/display/NIFI/Contributor
>> >> > +Guide#ContributorGuide-CodeReviewProcess
>> >> > > 2.
>> >> > > https://mail-archives.apache.org/mod_mbox/nifi-dev/201703.mb
>> >> > ox/%3CCALJK9a7onOKo%3DXtAEmL7PSBUEBEqOOhcT9WSz-RZaNxqV6q%
>> >> > 3Dmg%40mail.gmail.com%3E
>> >> > > 3.
>> >> > > https://mail-archives.apache.org/mod_mbox/nifi-dev/201501.mb
>> >> > ox/%3CCALhtWkf67UpOe9W9HQ3RSn00xb_=C6ZMxN+_SEfWthpQrU5fsg@
>> >> > mail.gmail.com%3E
>> >> > > 4.
>> >> > > http://mail-archives.apache.org/mod_mbox/incubator-gossip-de
>> >> > v/201606.mbox/%3CCAN5cbe6P8aEEOLMA%2BPrfpQg9c_AWeSfvvmom8jAp%
>> 3Dk7wUpoVgQ%
>> >> > 40mail.gmail.com%3E
>> >> >
>> >>
>>
Reply | Threaded
Open this post in threaded view
|

Re: RTC clarification

Joey Frazee
Joe(s), as you mentioned, even if we have a non-committer review, it can’t be merged until a committer decides whether to accept whatever decision was provided. So, the burden is still on committers as to whether it’s really a +1 or not. And presumably this should only happen if there’s some evidence that the right things happened.

So, I think the big question is how much to encourage non-committers to review. And, I think there’s probably more to gain from trying to be very encouraging than not. It builds community, it can be a teaching moment, and it can offload work where there isn’t enough time or expertise.

This would be really easy if the project required a double +1 to merge because there wouldn’t be any confusion about whether any one reviewer had sole authority, but I think if the developer guide and README was very encouraging and very explicit about how non-committer reviews would be treated, it’ll be easy to roll with.

On Jul 7, 2017, 9:10 AM -0500, Joe Witt <[hidden email]>, wrote:

> We're looking at a possible set of scenarios comprised of a
> 'submitter' and a 'reviewer'. There can be one or more reviewers. A
> submitter or reviewer is either a committer or is not a committer. If
> there is at least one reviewer that is a committer it is a committer
> reviewed scenario. So with that in mind...
>
> A) Committer submitted, Non-Committer reviewed = This can be merged in
> the event of a positive review outcome.
>
> B) Committer submitted, Committer reviewed = This can be merged in the
> event of a positive review outcome.
>
> C) Non-Committer submitted, Non-Committer reviewed = This cannot be
> merged until there is at least one positive committer review.
> However, that review is made easier for the committer as someone who
> is still building merit in the community has already done work here
> and further this is a great chance to work with/grow those folks in
> the community.
>
> D) Non-committer submitted, Committer reviewed = This can be merged in
> the event of a positive review outcome.
>
> Scenario B and D is easy and probably there is no discussion needed.
> Same for scenario C as there has to be at least one committer involved
> for the code to get pushed in the first place. I suspect it is only
> scenario A where we have some 'yeah but...' sort of thoughts going on.
>
> I am favorable to us allowing scenario A to result in a merge because
> at the end of the day there is a committer involved, heavily involved
> in fact, and they've already been voted by the community to be a
> committer. That is a high bar as it should be.
>
> The risk of bad commits getting through needs to be balanced against
> the risk of community growth being hampered by slow review cycles.
> Let's be honest, we need the help with review bandwidth. It is really
> hard to keep up and to my mind one of the best ways to annoy/deter
> contributors from advancing is to make the time between contribution
> to feedback take a long time.
>
>
> On Fri, Jul 7, 2017 at 9:58 AM, Joe Skora <[hidden email]> wrote:
> > I agree that non-committer review make more sense if the reviewer has
> > established some level of credibility and involvement in the community.
> >
> > Ultimately, the final committer has to be a community member and they will
> > have final responsibility for the code being Apache compliant and NiFi
> > ready, so even reviews by less well known members of the community should
> > still be low risk.
> >
> > On Fri, Jul 7, 2017 at 9:24 AM, Bryan Bende <[hidden email]> wrote:
> >
> > > I agree with encouraging reviews from everyone, but I lean towards
> > > "binding" reviews coming from committers.
> > >
> > > If we allow any review to be binding, there could be completely
> > > different levels of review that occur...
> > >
> > > There could be someone who isn't a committer yet, but has been
> > > contributing already and will probably do a very thorough review of
> > > someone's contribution, and there could be someone else who we never
> > > interacted with us before and writes "+1 LGTM" on a PR and we have no
> > > idea if they know what they are talking about or if they even tried
> > > the contribution to see if it works. Obviously a committer can also
> > > write "+1 LGTM", but I think when that comes from a committer it holds
> > > more weight.
> > >
> > > I think we may also want to clarify if we are only talking about
> > > "submitted by committer, reviewed by non-committer" or also talking
> > > about "submitted by non-committer, reviewed by non-committer".
> > >
> > > For the first case I can see the argument that since the contribution
> > > is from a committer who is already trusted, they can make the right
> > > judgement call based on the review. Where as in the second case, just
> > > because a community member submitted something and another community
> > > member says it looks good, doesn't necessarily mean a committer should
> > > come along and automatically merge it in.
> > >
> > >
> > > On Fri, Jul 7, 2017 at 4:13 AM, Michael Hogue
> > > <[hidden email]> wrote:
> > > > Thanks for fielding the question, Tony.
> > > >
> > > > Joe and James' statements both make sense. I suppose a case by case
> > > > analysis could be carried out, too. For example, since I'm mostly
> > > > unfamiliar with the code base but am looking to gain familiarity, I'm
> > > > reviewing pretty straightforward or trivial PRs. My plan was to continue
> > > > doing that until I felt comfortable reviewing something with a larger
> > > > impact, such as the new TCPListenRecord processor implementation [1].
> > > > However, as Tony explained, my original question was whether that sort of
> > > > review would be binding or whether I should be doing it at all. I think
> > > > both of those questions were answered here in that ultimately committer
> > > > sign off is needed, but reviews may be binding regardless of source.
> > > >
> > > > Thanks for the feedback!
> > > >
> > > > Mike
> > > >
> > > >
> > > > [1] https://github.com/apache/nifi/pull/1987
> > > > On Fri, Jul 7, 2017 at 01:14 James Wing <[hidden email]> wrote:
> > > >
> > > > > We should definitely encourage review feedback from non-committers.
> > > > > Getting additional perspectives, interest, and enthusiasm from users is
> > > > > critical for any project, doubly so for an integrating application where
> > > > > committers cannot be experts in all the systems we are integrating
> > > with. I
> > > > > believe NiFi could use more review bandwidth. Are missing out on
> > > potential
> > > > > reviewers because of the current policy?
> > > > >
> > > > > I do not have any experience with non-committer "binding reviews" as
> > > > > described in the Apache Gossip thread. How would that work? Wouldn't a
> > > > > committer have to review the review and decide to commit? If we knew
> > > the
> > > > > reviewer well enough to accept their judgement, why not make them a
> > > > > committer?
> > > > >
> > > > > My expectation is that many non-committer reviews are helpful and
> > > > > constructive, but not necessarily 100% comprehensive. Reviewers might
> > > > > comment on the JIRA ticket without working with the PR, or try the
> > > proposed
> > > > > change without reviewing the code, tests, etc. All great stuff, but
> > > > > backstopped by committers.
> > > > >
> > > > > Thanks,
> > > > >
> > > > > James
> > > > >
> > > > > On Thu, Jul 6, 2017 at 7:30 PM, Joe Witt <[hidden email]> wrote:
> > > > >
> > > > > > It is undefined at this point and I agree we should reach consensus
> > > > > > and document it.
> > > > > >
> > > > > > I am in favor making non-committer reviews binding.
> > > > > >
> > > > > > Why do we do RTC:
> > > > > > - To help bring along new committers/grow the community
> > > > > > - To help promote quality by having peer reviews
> > > > > >
> > > > > > Enabling non-committer reviews to be binding still allows both of
> > > > > > those to be true.
> > > > > >
> > > > > > On Thu, Jul 6, 2017 at 10:10 PM, Tony Kurc <[hidden email]> wrote:
> > > > > > > All, I was having a discussion with Mike Hogue - a recent
> > > contributor -
> > > > > > off
> > > > > > > list, and he had some questions about the review process. And
> > > largely
> > > > > the
> > > > > > > root of the question was, if a non-committer reviews a patch or PR
> > > > > (which
> > > > > > > Mike has spent some time doing), is that considered a "review"? I
> > > > > didn't
> > > > > > > have the answers, so I went on a hunt for documentation. I started
> > > with
> > > > > > the
> > > > > > > Contributor Guide [1]. The guide describes reviewing, and calls out
> > > a
> > > > > > > Reviewer role, but doesn't specifically point out that Reviewer is a
> > > > > > > committer, just that a committer "can actively promote contributions
> > > > > into
> > > > > > > the repository", and goes on to imply the non-committers can review.
> > > > > > >
> > > > > > > Given this, I was unable to answer this question:
> > > > > > > If a committer "X" submits a patch or PR, it is reviewed by a
> > > > > > non-committer
> > > > > > > "Y", does that review satisfy the RTC requirement, and "X" may
> > > merge in
> > > > > > the
> > > > > > > patch?
> > > > > > >
> > > > > > > I found a related discussion on the email list in March [2], which I
> > > > > > think
> > > > > > > implies at least some of the community assumed the canonical review
> > > > > must
> > > > > > be
> > > > > > > by a committer. I also went back a bit to early days [3], where
> > > Benson
> > > > > > > implied a much less "formal" review process.
> > > > > > >
> > > > > > > What I'm hoping for is hopefully come to a consensus of what our
> > > > > project
> > > > > > > expectations are and document in the Contributor Guide. My
> > > expectation
> > > > > > was
> > > > > > > that non-committers could review, similar to what Sean discussed on
> > > > > this
> > > > > > > thread for Apache Gossip (incubating) [4]. However, I don't believe
> > > > > this
> > > > > > is
> > > > > > > the current consensus.
> > > > > > >
> > > > > > > Thoughts?
> > > > > > >
> > > > > > > Tony
> > > > > > >
> > > > > > > 1.
> > > > > > > https://cwiki.apache.org/confluence/display/NIFI/Contributor
> > > > > > +Guide#ContributorGuide-CodeReviewProcess
> > > > > > > 2.
> > > > > > > https://mail-archives.apache.org/mod_mbox/nifi-dev/201703.mb
> > > > > > ox/%3CCALJK9a7onOKo%3DXtAEmL7PSBUEBEqOOhcT9WSz-RZaNxqV6q%
> > > > > > 3Dmg%40mail.gmail.com%3E
> > > > > > > 3.
> > > > > > > https://mail-archives.apache.org/mod_mbox/nifi-dev/201501.mb
> > > > > > ox/%3CCALhtWkf67UpOe9W9HQ3RSn00xb_=C6ZMxN+_SEfWthpQrU5fsg@
> > > > > > mail.gmail.com%3E
> > > > > > > 4.
> > > > > > > http://mail-archives.apache.org/mod_mbox/incubator-gossip-de
> > > > > > v/201606.mbox/%3CCAN5cbe6P8aEEOLMA%2BPrfpQg9c_AWeSfvvmom8jAp%
> > > 3Dk7wUpoVgQ%
> > > > > > 40mail.gmail.com%3E
> > > > > >
> > > > >
> > >
Reply | Threaded
Open this post in threaded view
|

Re: RTC clarification

Kevin Doran
In reply to this post by Joe Witt
I am in favor of allowing Scenario A and leaving it up to the committer / submitter to determine if the review is sufficient to merge or if an additional review(s) should take place.

I think for committers will know the difference between, say, a documentation update or minor enhancement / fix to an extension vs. a major refactoring or re-write to part of the core framework.  In the latter case, the submitter probably wants someone familiar with that part of the code base to do a thorough review, and therefore will wait until that has a chance to occur prior to merging regardless of other approvals. I trust committers to make the correct call in that case, so I don't see a downside to Scenario A. And as others have stated, Scenario A has the upside of encouraging non-committers to get involved in code reviews... it will seem a little bit pointless for a non-committer to take the time to do a review if the project policy dictates it will not be binding and require a review by a committer anyway prior to merge.

Cheers,
Kevin

On 7/7/17, 10:10, "Joe Witt" <[hidden email]> wrote:

    We're looking at a possible set of scenarios comprised of a
    'submitter' and a 'reviewer'.  There can be one or more reviewers.  A
    submitter or reviewer is either a committer or is not a committer. If
    there is at least one reviewer that is a committer it is a committer
    reviewed scenario.  So with that in mind...
   
    A) Committer submitted, Non-Committer reviewed = This can be merged in
    the event of a positive review outcome.
   
    B) Committer submitted, Committer reviewed = This can be merged in the
    event of a positive review outcome.
   
    C) Non-Committer submitted, Non-Committer reviewed = This cannot be
    merged until there is at least one positive committer review.
    However, that review is made easier for the committer as someone who
    is still building merit in the community has already done work here
    and further this is a great chance to work with/grow those folks in
    the community.
   
    D) Non-committer submitted, Committer reviewed = This can be merged in
    the event of a positive review outcome.
   
    Scenario B and D is easy and probably there is no discussion needed.
    Same for scenario C as there has to be at least one committer involved
    for the code to get pushed in the first place.  I suspect it is only
    scenario A where we have some 'yeah but...' sort of thoughts going on.
   
    I am favorable to us allowing scenario A to result in a merge because
    at the end of the day there is a committer involved, heavily involved
    in fact, and they've already been voted by the community to be a
    committer.  That is a high bar as it should be.
   
    The risk of bad commits getting through needs to be balanced against
    the risk of community growth being hampered by slow review cycles.
    Let's be honest, we need the help with review bandwidth.  It is really
    hard to keep up and to my mind one of the best ways to annoy/deter
    contributors from advancing is to make the time between contribution
    to feedback take a long time.
   
   
    On Fri, Jul 7, 2017 at 9:58 AM, Joe Skora <[hidden email]> wrote:
    > I agree that non-committer review make more sense if the reviewer has
    > established some level of credibility and involvement in the community.
    >
    > Ultimately, the final committer has to be a community member and they will
    > have final responsibility for the code being Apache compliant and NiFi
    > ready, so even reviews by less well known members of the community should
    > still be low risk.
    >
    > On Fri, Jul 7, 2017 at 9:24 AM, Bryan Bende <[hidden email]> wrote:
    >
    >> I agree with encouraging reviews from everyone, but I lean towards
    >> "binding" reviews coming from committers.
    >>
    >> If we allow any review to be binding, there could be completely
    >> different levels of review that occur...
    >>
    >> There could be someone who isn't a committer yet, but has been
    >> contributing already and will probably do a very thorough review of
    >> someone's contribution, and there could be someone else who we never
    >> interacted with us before and writes "+1 LGTM" on a PR and we have no
    >> idea if they know what they are talking about or if they even tried
    >> the contribution to see if it works. Obviously a committer can also
    >> write "+1 LGTM", but I think when that comes from a committer it holds
    >> more weight.
    >>
    >> I think we may also want to clarify if we are only talking about
    >> "submitted by committer, reviewed by non-committer" or also talking
    >> about "submitted by non-committer, reviewed by non-committer".
    >>
    >> For the first case I can see the argument that since the contribution
    >> is from a committer who is already trusted, they can make the right
    >> judgement call based on the review. Where as in the second case, just
    >> because a community member submitted something and another community
    >> member says it looks good, doesn't necessarily mean a committer should
    >> come along and automatically merge it in.
    >>
    >>
    >> On Fri, Jul 7, 2017 at 4:13 AM, Michael Hogue
    >> <[hidden email]> wrote:
    >> > Thanks for fielding the question, Tony.
    >> >
    >> > Joe and James' statements both make sense. I suppose a case by case
    >> > analysis could be carried out, too. For example, since I'm mostly
    >> > unfamiliar with the code base but am looking to gain familiarity, I'm
    >> > reviewing pretty straightforward or trivial PRs. My plan was to continue
    >> > doing that until I felt comfortable reviewing something with a larger
    >> > impact, such as the new TCPListenRecord processor implementation [1].
    >> > However, as Tony explained, my original question was whether that sort of
    >> > review would be binding or whether I should be doing it at all. I think
    >> > both of those questions were answered here in that ultimately committer
    >> > sign off is needed, but reviews may be binding regardless of source.
    >> >
    >> > Thanks for the feedback!
    >> >
    >> > Mike
    >> >
    >> >
    >> > [1] https://github.com/apache/nifi/pull/1987
    >> > On Fri, Jul 7, 2017 at 01:14 James Wing <[hidden email]> wrote:
    >> >
    >> >> We should definitely encourage review feedback from non-committers.
    >> >> Getting additional perspectives, interest, and enthusiasm from users is
    >> >> critical for any project, doubly so for an integrating application where
    >> >> committers cannot be experts in all the systems we are integrating
    >> with.  I
    >> >> believe NiFi could use more review bandwidth.  Are missing out on
    >> potential
    >> >> reviewers because of the current policy?
    >> >>
    >> >> I do not have any experience with non-committer "binding reviews" as
    >> >> described in the Apache Gossip thread.  How would that work?  Wouldn't a
    >> >> committer have to review the review and decide to commit?  If we knew
    >> the
    >> >> reviewer well enough to accept their judgement, why not make them a
    >> >> committer?
    >> >>
    >> >> My expectation is that many non-committer reviews are helpful and
    >> >> constructive, but not necessarily 100% comprehensive.  Reviewers might
    >> >> comment on the JIRA ticket without working with the PR, or try the
    >> proposed
    >> >> change without reviewing the code, tests, etc.  All great stuff, but
    >> >> backstopped by committers.
    >> >>
    >> >> Thanks,
    >> >>
    >> >> James
    >> >>
    >> >> On Thu, Jul 6, 2017 at 7:30 PM, Joe Witt <[hidden email]> wrote:
    >> >>
    >> >> > It is undefined at this point and I agree we should reach consensus
    >> >> > and document it.
    >> >> >
    >> >> > I am in favor making non-committer reviews binding.
    >> >> >
    >> >> > Why do we do RTC:
    >> >> > - To help bring along new committers/grow the community
    >> >> > - To help promote quality by having peer reviews
    >> >> >
    >> >> > Enabling non-committer reviews to be binding still allows both of
    >> >> > those to be true.
    >> >> >
    >> >> > On Thu, Jul 6, 2017 at 10:10 PM, Tony Kurc <[hidden email]> wrote:
    >> >> > > All, I was having a discussion with Mike Hogue - a recent
    >> contributor -
    >> >> > off
    >> >> > > list, and he had some questions about the review process. And
    >> largely
    >> >> the
    >> >> > > root of the question was, if a non-committer reviews a patch or PR
    >> >> (which
    >> >> > > Mike has spent some time doing), is that considered a "review"? I
    >> >> didn't
    >> >> > > have the answers, so I went on a hunt for documentation. I started
    >> with
    >> >> > the
    >> >> > > Contributor Guide [1]. The guide describes reviewing, and calls out
    >> a
    >> >> > > Reviewer role, but doesn't specifically point out that Reviewer is a
    >> >> > > committer, just that a committer "can actively promote contributions
    >> >> into
    >> >> > > the repository", and goes on to imply the non-committers can review.
    >> >> > >
    >> >> > > Given this, I was unable to answer this question:
    >> >> > > If a committer "X" submits a patch or PR, it is reviewed by a
    >> >> > non-committer
    >> >> > > "Y", does that review satisfy the RTC requirement, and "X" may
    >> merge in
    >> >> > the
    >> >> > > patch?
    >> >> > >
    >> >> > > I found a related discussion on the email list in March [2], which I
    >> >> > think
    >> >> > > implies at least some of the community assumed the canonical review
    >> >> must
    >> >> > be
    >> >> > > by a committer. I also went back a bit to early days [3], where
    >> Benson
    >> >> > > implied a much less "formal" review process.
    >> >> > >
    >> >> > > What I'm hoping for is hopefully come to a consensus of what our
    >> >> project
    >> >> > > expectations are and document in the Contributor Guide. My
    >> expectation
    >> >> > was
    >> >> > > that non-committers could review, similar to what Sean discussed on
    >> >> this
    >> >> > > thread for Apache Gossip (incubating) [4]. However, I don't believe
    >> >> this
    >> >> > is
    >> >> > > the current consensus.
    >> >> > >
    >> >> > > Thoughts?
    >> >> > >
    >> >> > > Tony
    >> >> > >
    >> >> > > 1.
    >> >> > > https://cwiki.apache.org/confluence/display/NIFI/Contributor
    >> >> > +Guide#ContributorGuide-CodeReviewProcess
    >> >> > > 2.
    >> >> > > https://mail-archives.apache.org/mod_mbox/nifi-dev/201703.mb
    >> >> > ox/%3CCALJK9a7onOKo%3DXtAEmL7PSBUEBEqOOhcT9WSz-RZaNxqV6q%
    >> >> > 3Dmg%40mail.gmail.com%3E
    >> >> > > 3.
    >> >> > > https://mail-archives.apache.org/mod_mbox/nifi-dev/201501.mb
    >> >> > ox/%3CCALhtWkf67UpOe9W9HQ3RSn00xb_=C6ZMxN+_SEfWthpQrU5fsg@
    >> >> > mail.gmail.com%3E
    >> >> > > 4.
    >> >> > > http://mail-archives.apache.org/mod_mbox/incubator-gossip-de
    >> >> > v/201606.mbox/%3CCAN5cbe6P8aEEOLMA%2BPrfpQg9c_AWeSfvvmom8jAp%
    >> 3Dk7wUpoVgQ%
    >> >> > 40mail.gmail.com%3E
    >> >> >
    >> >>
    >>