[DISCUSS] Voting on pull requests

[Andor Molnar <an...@apache.org>]

Hi folks,

I’ve seen 2 patches committed recently with “-1s" from committers on it.

https://github.com/apache/zookeeper/pull/899 <https://github.com/apache/zookeeper/pull/899>
https://github.com/apache/zookeeper/pull/944 <https://github.com/apache/zookeeper/pull/944>

Not a big deal in this case and I think they were in a good shape and ready to commit, but I’d like to clarify how do we handle voting on pull requests. We use github to prepare patches by creating pull requests. Github also has a feature of “reviewing” which means that reviewers are able to “approve”, “comment” and “request for changes”. In terms of voting this means:

- “approve” = +1
- “comment” = 0
- “request for changes” = -1

In order to commit a patch we need at least 2 binding +1s without binding -1. Committers/PMCs are able to veto this way.

Do we agree on this process completely?

I know that activity in ZooKeeper community is usually very flaky and sometimes it’s hard to find committers to review patches. In these cases we usually just commit smaller patches with a single binding vote, but I think we should be more careful about binding -1s.

Please in the future if you see my -1 on a patch which you think is ready to commit, bug me as hard as it takes. I’ll make every effort to review as soon as possible and apologies for any delay.

Thanks,
Andor

Re: [DISCUSS] Voting on pull requests

[Enrico Olivelli <eo...@gmail.com>]

Il ven 14 giu 2019, 23:34 Andor Molnar <an...@cloudera.com.invalid> ha
scritto:

> Personally I'd like to check whether the requested change has been
> addressed as I expected and validate the contributor and myself are on the
> same page. That's why I usually provide feedback with "request changes" (in
> big RED color on github indicating that the patch should not be merged
> yet.)
>
> For minor issues I usually approve at the same time I'm commenting - so not
> really care about.
>
> Either way, if we agree in "request changes" == -1, then please don't
> commit until the reviewer removes it.
>

I agree
As told before, we cuold enhance the commit script in a way thay it checks
for at least one approval, no 'request changes' and all green CI.

I will compare the script with Bookkeeper one (which was taken from the
same source)

The more we enforce rules automatically the less misunderstanding we will
have


Enrico



> Thanks,
> Andor
>
>
>
> On Thu, Jun 13, 2019 at 8:19 PM Patrick Hunt <ph...@apache.org> wrote:
>
> > On Thu, Jun 13, 2019 at 11:08 AM Fangmin Lv <lv...@gmail.com> wrote:
> >
> > > Agree to not commit if there is a -1, and we should align with that
> rule.
> > >
> > > I'm not sure if "request to change" is equal to -1 though, in theory
> all
> > > comments may require to change something. It would be great for the
> > > reviewers who provided opinions to review again,
> > > but it seems to me if it's not a critical thing with fundamental
> changes,
> > > the other committers can review and see if the comments being addressed
> > as
> > > well. So it's not being blocked by a single
> > > reviewer.
> > >
> > > I would suggest to distinguish the general comments from -1, and let's
> > use
> > > -1 explicitly for PR which have big issue like direction or
> correctness,
> > > etc.
> > >
> > >
> > If feedback is given by a committer it should be addressed before
> committed
> > either by making the recommended changes or providing insight on why not.
> >
> > Voting "-1" is pretty strong handed -
> > https://www.apache.org/foundation/voting.html#votes-on-code-modification
> > Although
> > "+1" is good practice - note that there's technically a minimum length
> of 1
> > day consensus period.
> >
> > If you're not sure ask and be respectful of other people's feedback.
> >
> > Patrick
> >
> >
> >
> > > Thanks,
> > > Fangmin
> > >
> > >
> > > On Thu, Jun 6, 2019 at 10:55 AM Brian Nixon <br...@gmail.com>
> > > wrote:
> > >
> > > > The community should absolutely stand by its bylaws. :)
> > > >
> > > > Those two pull requests (899 and 944) were mine so I'd like to sketch
> > > what
> > > > I saw as a contributor and hopefully figure out a healthier way
> > forward.
> > > > Both were opened in the last two months and got reviewer action
> within
> > > > days. There was a reasonable back and forth between comments and new
> > > > commits addressing concerns. Some of the concerns were back by
> > "requested
> > > > changes" as is proper. Subsequent commits addressed those comments
> just
> > > the
> > > > same as other review comments and this is where I think communication
> > > fell
> > > > down. I was expecting each reviewer to follow up with some change to
> > > their
> > > > vote status. In one case, the reviewer continued to interact with the
> > > pull
> > > > request without clarifying whether more changes were expected. This
> > > > generates confusion at a minimum and both pull requests had -1 votes
> > that
> > > > had been addressed for three weeks before they were eventually
> merged.
> > > >
> > > > Ultimately, as a contributor it is our responsibility to push for our
> > > > contribution and I could certainly have more aggressively pinged
> folks
> > on
> > > > my pull requests (busy people need helpful reminders sometimes). I'd
> > > > recommend though, a bit of followup when using "requested changes" on
> > > pull
> > > > requests given that it requires three active "approve"s to override
> one
> > > > dangling "requested changes" and there generally aren't four
> committers
> > > > simultaneously interacting with a single pull request.
> > > >
> > > > -Brian
> > > >
> > > >
> > > >
> > > > On Thu, Jun 6, 2019 at 9:51 AM Andor Molnár <an...@apache.org>
> wrote:
> > > >
> > > > > Exactly.
> > > > >
> > > > >
> > > > > On 2019. 06. 06. 14:51, Flavio Junqueira wrote:
> > > > > > That's covered in the project bylaws, right?
> > > > > >
> > > > > > https://zookeeper.apache.org/bylaws.html <
> > > > > https://zookeeper.apache.org/bylaws.html>
> > > > > >
> > > > > > -Flavio
> > > > > >
> > > > > >> On 6 Jun 2019, at 13:49, Enrico Olivelli <eo...@gmail.com>
> > > wrote:
> > > > > >>
> > > > > >> Il gio 6 giu 2019, 12:44 Andor Molnar <andor@apache.org
> <mailto:
> > > > > andor@apache.org>> ha scritto:
> > > > > >>
> > > > > >>> Hi folks,
> > > > > >>>
> > > > > >>> I’ve seen 2 patches committed recently with “-1s" from
> committers
> > > on
> > > > > it.
> > > > > >>>
> > > > > >>> https://github.com/apache/zookeeper/pull/899 <
> > > > > >>> https://github.com/apache/zookeeper/pull/899>
> > > > > >>> https://github.com/apache/zookeeper/pull/944 <
> > > > > >>> https://github.com/apache/zookeeper/pull/944>
> > > > > >>>
> > > > > >>> Not a big deal in this case and I think they were in a good
> shape
> > > and
> > > > > >>> ready to commit, but I’d like to clarify how do we handle
> voting
> > on
> > > > > pull
> > > > > >>> requests. We use github to prepare patches by creating pull
> > > requests.
> > > > > >>> Github also has a feature of “reviewing” which means that
> > reviewers
> > > > are
> > > > > >>> able to “approve”, “comment” and “request for changes”. In
> terms
> > of
> > > > > voting
> > > > > >>> this means:
> > > > > >>>
> > > > > >>> - “approve” = +1
> > > > > >>> - “comment” = 0
> > > > > >>> - “request for changes” = -1
> > > > > >>>
> > > > > >> We should enhance the script (we already did it on Bookkeeper
> for
> > > > > instance)
> > > > > >>
> > > > > >>> In order to commit a patch we need at least 2 binding +1s
> without
> > > > > binding
> > > > > >>> -1. Committers/PMCs are able to veto this way.
> > > > > >>>
> > > > > >>> Do we agree on this process completely?
> > > > > >>>
> > > > > >> Sure
> > > > > >>
> > > > > >>> I know that activity in ZooKeeper community is usually very
> flaky
> > > and
> > > > > >>> sometimes it’s hard to find committers to review patches.
> > > > > >>
> > > > > >> We have a new wave of contributions and new committers, so
> > > fortunately
> > > > > this
> > > > > >> is changing.
> > > > > >>
> > > > > >>
> > > > > >> In these cases we usually just commit smaller patches with a
> > single
> > > > > binding
> > > > > >>> vote, but I think we should be more careful about binding -1s.
> > > > > >>>
> > > > > >>> Please in the future if you see my -1 on a patch which you
> think
> > is
> > > > > ready
> > > > > >>> to commit, bug me as hard as it takes. I’ll make every effort
> to
> > > > > review as
> > > > > >>> soon as possible and apologies for any delay.
> > > > > >>>
> > > > > >> Sure.
> > > > > >>
> > > > > >>
> > > > > >> Enrico
> > > > > >>
> > > > > >>
> > > > > >>> Thanks,
> > > > > >>> Andor
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: [DISCUSS] Voting on pull requests

[Andor Molnar <an...@cloudera.com.INVALID>]

Personally I'd like to check whether the requested change has been
addressed as I expected and validate the contributor and myself are on the
same page. That's why I usually provide feedback with "request changes" (in
big RED color on github indicating that the patch should not be merged yet.)

For minor issues I usually approve at the same time I'm commenting - so not
really care about.

Either way, if we agree in "request changes" == -1, then please don't
commit until the reviewer removes it.

Thanks,
Andor



On Thu, Jun 13, 2019 at 8:19 PM Patrick Hunt <ph...@apache.org> wrote:

> On Thu, Jun 13, 2019 at 11:08 AM Fangmin Lv <lv...@gmail.com> wrote:
>
> > Agree to not commit if there is a -1, and we should align with that rule.
> >
> > I'm not sure if "request to change" is equal to -1 though, in theory all
> > comments may require to change something. It would be great for the
> > reviewers who provided opinions to review again,
> > but it seems to me if it's not a critical thing with fundamental changes,
> > the other committers can review and see if the comments being addressed
> as
> > well. So it's not being blocked by a single
> > reviewer.
> >
> > I would suggest to distinguish the general comments from -1, and let's
> use
> > -1 explicitly for PR which have big issue like direction or correctness,
> > etc.
> >
> >
> If feedback is given by a committer it should be addressed before committed
> either by making the recommended changes or providing insight on why not.
>
> Voting "-1" is pretty strong handed -
> https://www.apache.org/foundation/voting.html#votes-on-code-modification
> Although
> "+1" is good practice - note that there's technically a minimum length of 1
> day consensus period.
>
> If you're not sure ask and be respectful of other people's feedback.
>
> Patrick
>
>
>
> > Thanks,
> > Fangmin
> >
> >
> > On Thu, Jun 6, 2019 at 10:55 AM Brian Nixon <br...@gmail.com>
> > wrote:
> >
> > > The community should absolutely stand by its bylaws. :)
> > >
> > > Those two pull requests (899 and 944) were mine so I'd like to sketch
> > what
> > > I saw as a contributor and hopefully figure out a healthier way
> forward.
> > > Both were opened in the last two months and got reviewer action within
> > > days. There was a reasonable back and forth between comments and new
> > > commits addressing concerns. Some of the concerns were back by
> "requested
> > > changes" as is proper. Subsequent commits addressed those comments just
> > the
> > > same as other review comments and this is where I think communication
> > fell
> > > down. I was expecting each reviewer to follow up with some change to
> > their
> > > vote status. In one case, the reviewer continued to interact with the
> > pull
> > > request without clarifying whether more changes were expected. This
> > > generates confusion at a minimum and both pull requests had -1 votes
> that
> > > had been addressed for three weeks before they were eventually merged.
> > >
> > > Ultimately, as a contributor it is our responsibility to push for our
> > > contribution and I could certainly have more aggressively pinged folks
> on
> > > my pull requests (busy people need helpful reminders sometimes). I'd
> > > recommend though, a bit of followup when using "requested changes" on
> > pull
> > > requests given that it requires three active "approve"s to override one
> > > dangling "requested changes" and there generally aren't four committers
> > > simultaneously interacting with a single pull request.
> > >
> > > -Brian
> > >
> > >
> > >
> > > On Thu, Jun 6, 2019 at 9:51 AM Andor Molnár <an...@apache.org> wrote:
> > >
> > > > Exactly.
> > > >
> > > >
> > > > On 2019. 06. 06. 14:51, Flavio Junqueira wrote:
> > > > > That's covered in the project bylaws, right?
> > > > >
> > > > > https://zookeeper.apache.org/bylaws.html <
> > > > https://zookeeper.apache.org/bylaws.html>
> > > > >
> > > > > -Flavio
> > > > >
> > > > >> On 6 Jun 2019, at 13:49, Enrico Olivelli <eo...@gmail.com>
> > wrote:
> > > > >>
> > > > >> Il gio 6 giu 2019, 12:44 Andor Molnar <andor@apache.org <mailto:
> > > > andor@apache.org>> ha scritto:
> > > > >>
> > > > >>> Hi folks,
> > > > >>>
> > > > >>> I’ve seen 2 patches committed recently with “-1s" from committers
> > on
> > > > it.
> > > > >>>
> > > > >>> https://github.com/apache/zookeeper/pull/899 <
> > > > >>> https://github.com/apache/zookeeper/pull/899>
> > > > >>> https://github.com/apache/zookeeper/pull/944 <
> > > > >>> https://github.com/apache/zookeeper/pull/944>
> > > > >>>
> > > > >>> Not a big deal in this case and I think they were in a good shape
> > and
> > > > >>> ready to commit, but I’d like to clarify how do we handle voting
> on
> > > > pull
> > > > >>> requests. We use github to prepare patches by creating pull
> > requests.
> > > > >>> Github also has a feature of “reviewing” which means that
> reviewers
> > > are
> > > > >>> able to “approve”, “comment” and “request for changes”. In terms
> of
> > > > voting
> > > > >>> this means:
> > > > >>>
> > > > >>> - “approve” = +1
> > > > >>> - “comment” = 0
> > > > >>> - “request for changes” = -1
> > > > >>>
> > > > >> We should enhance the script (we already did it on Bookkeeper for
> > > > instance)
> > > > >>
> > > > >>> In order to commit a patch we need at least 2 binding +1s without
> > > > binding
> > > > >>> -1. Committers/PMCs are able to veto this way.
> > > > >>>
> > > > >>> Do we agree on this process completely?
> > > > >>>
> > > > >> Sure
> > > > >>
> > > > >>> I know that activity in ZooKeeper community is usually very flaky
> > and
> > > > >>> sometimes it’s hard to find committers to review patches.
> > > > >>
> > > > >> We have a new wave of contributions and new committers, so
> > fortunately
> > > > this
> > > > >> is changing.
> > > > >>
> > > > >>
> > > > >> In these cases we usually just commit smaller patches with a
> single
> > > > binding
> > > > >>> vote, but I think we should be more careful about binding -1s.
> > > > >>>
> > > > >>> Please in the future if you see my -1 on a patch which you think
> is
> > > > ready
> > > > >>> to commit, bug me as hard as it takes. I’ll make every effort to
> > > > review as
> > > > >>> soon as possible and apologies for any delay.
> > > > >>>
> > > > >> Sure.
> > > > >>
> > > > >>
> > > > >> Enrico
> > > > >>
> > > > >>
> > > > >>> Thanks,
> > > > >>> Andor
> > > > >
> > > >
> > >
> >
>

Re: [DISCUSS] Voting on pull requests

[Patrick Hunt <ph...@apache.org>]

On Thu, Jun 13, 2019 at 11:08 AM Fangmin Lv <lv...@gmail.com> wrote:

> Agree to not commit if there is a -1, and we should align with that rule.
>
> I'm not sure if "request to change" is equal to -1 though, in theory all
> comments may require to change something. It would be great for the
> reviewers who provided opinions to review again,
> but it seems to me if it's not a critical thing with fundamental changes,
> the other committers can review and see if the comments being addressed as
> well. So it's not being blocked by a single
> reviewer.
>
> I would suggest to distinguish the general comments from -1, and let's use
> -1 explicitly for PR which have big issue like direction or correctness,
> etc.
>
>
If feedback is given by a committer it should be addressed before committed
either by making the recommended changes or providing insight on why not.

Voting "-1" is pretty strong handed -
https://www.apache.org/foundation/voting.html#votes-on-code-modification
Although
"+1" is good practice - note that there's technically a minimum length of 1
day consensus period.

If you're not sure ask and be respectful of other people's feedback.

Patrick



> Thanks,
> Fangmin
>
>
> On Thu, Jun 6, 2019 at 10:55 AM Brian Nixon <br...@gmail.com>
> wrote:
>
> > The community should absolutely stand by its bylaws. :)
> >
> > Those two pull requests (899 and 944) were mine so I'd like to sketch
> what
> > I saw as a contributor and hopefully figure out a healthier way forward.
> > Both were opened in the last two months and got reviewer action within
> > days. There was a reasonable back and forth between comments and new
> > commits addressing concerns. Some of the concerns were back by "requested
> > changes" as is proper. Subsequent commits addressed those comments just
> the
> > same as other review comments and this is where I think communication
> fell
> > down. I was expecting each reviewer to follow up with some change to
> their
> > vote status. In one case, the reviewer continued to interact with the
> pull
> > request without clarifying whether more changes were expected. This
> > generates confusion at a minimum and both pull requests had -1 votes that
> > had been addressed for three weeks before they were eventually merged.
> >
> > Ultimately, as a contributor it is our responsibility to push for our
> > contribution and I could certainly have more aggressively pinged folks on
> > my pull requests (busy people need helpful reminders sometimes). I'd
> > recommend though, a bit of followup when using "requested changes" on
> pull
> > requests given that it requires three active "approve"s to override one
> > dangling "requested changes" and there generally aren't four committers
> > simultaneously interacting with a single pull request.
> >
> > -Brian
> >
> >
> >
> > On Thu, Jun 6, 2019 at 9:51 AM Andor Molnár <an...@apache.org> wrote:
> >
> > > Exactly.
> > >
> > >
> > > On 2019. 06. 06. 14:51, Flavio Junqueira wrote:
> > > > That's covered in the project bylaws, right?
> > > >
> > > > https://zookeeper.apache.org/bylaws.html <
> > > https://zookeeper.apache.org/bylaws.html>
> > > >
> > > > -Flavio
> > > >
> > > >> On 6 Jun 2019, at 13:49, Enrico Olivelli <eo...@gmail.com>
> wrote:
> > > >>
> > > >> Il gio 6 giu 2019, 12:44 Andor Molnar <andor@apache.org <mailto:
> > > andor@apache.org>> ha scritto:
> > > >>
> > > >>> Hi folks,
> > > >>>
> > > >>> I’ve seen 2 patches committed recently with “-1s" from committers
> on
> > > it.
> > > >>>
> > > >>> https://github.com/apache/zookeeper/pull/899 <
> > > >>> https://github.com/apache/zookeeper/pull/899>
> > > >>> https://github.com/apache/zookeeper/pull/944 <
> > > >>> https://github.com/apache/zookeeper/pull/944>
> > > >>>
> > > >>> Not a big deal in this case and I think they were in a good shape
> and
> > > >>> ready to commit, but I’d like to clarify how do we handle voting on
> > > pull
> > > >>> requests. We use github to prepare patches by creating pull
> requests.
> > > >>> Github also has a feature of “reviewing” which means that reviewers
> > are
> > > >>> able to “approve”, “comment” and “request for changes”. In terms of
> > > voting
> > > >>> this means:
> > > >>>
> > > >>> - “approve” = +1
> > > >>> - “comment” = 0
> > > >>> - “request for changes” = -1
> > > >>>
> > > >> We should enhance the script (we already did it on Bookkeeper for
> > > instance)
> > > >>
> > > >>> In order to commit a patch we need at least 2 binding +1s without
> > > binding
> > > >>> -1. Committers/PMCs are able to veto this way.
> > > >>>
> > > >>> Do we agree on this process completely?
> > > >>>
> > > >> Sure
> > > >>
> > > >>> I know that activity in ZooKeeper community is usually very flaky
> and
> > > >>> sometimes it’s hard to find committers to review patches.
> > > >>
> > > >> We have a new wave of contributions and new committers, so
> fortunately
> > > this
> > > >> is changing.
> > > >>
> > > >>
> > > >> In these cases we usually just commit smaller patches with a single
> > > binding
> > > >>> vote, but I think we should be more careful about binding -1s.
> > > >>>
> > > >>> Please in the future if you see my -1 on a patch which you think is
> > > ready
> > > >>> to commit, bug me as hard as it takes. I’ll make every effort to
> > > review as
> > > >>> soon as possible and apologies for any delay.
> > > >>>
> > > >> Sure.
> > > >>
> > > >>
> > > >> Enrico
> > > >>
> > > >>
> > > >>> Thanks,
> > > >>> Andor
> > > >
> > >
> >
>

Re: [DISCUSS] Voting on pull requests

[Fangmin Lv <lv...@gmail.com>]

Agree to not commit if there is a -1, and we should align with that rule.

I'm not sure if "request to change" is equal to -1 though, in theory all
comments may require to change something. It would be great for the
reviewers who provided opinions to review again,
but it seems to me if it's not a critical thing with fundamental changes,
the other committers can review and see if the comments being addressed as
well. So it's not being blocked by a single
reviewer.

I would suggest to distinguish the general comments from -1, and let's use
-1 explicitly for PR which have big issue like direction or correctness,
etc.

Thanks,
Fangmin


On Thu, Jun 6, 2019 at 10:55 AM Brian Nixon <br...@gmail.com>
wrote:

> The community should absolutely stand by its bylaws. :)
>
> Those two pull requests (899 and 944) were mine so I'd like to sketch what
> I saw as a contributor and hopefully figure out a healthier way forward.
> Both were opened in the last two months and got reviewer action within
> days. There was a reasonable back and forth between comments and new
> commits addressing concerns. Some of the concerns were back by "requested
> changes" as is proper. Subsequent commits addressed those comments just the
> same as other review comments and this is where I think communication fell
> down. I was expecting each reviewer to follow up with some change to their
> vote status. In one case, the reviewer continued to interact with the pull
> request without clarifying whether more changes were expected. This
> generates confusion at a minimum and both pull requests had -1 votes that
> had been addressed for three weeks before they were eventually merged.
>
> Ultimately, as a contributor it is our responsibility to push for our
> contribution and I could certainly have more aggressively pinged folks on
> my pull requests (busy people need helpful reminders sometimes). I'd
> recommend though, a bit of followup when using "requested changes" on pull
> requests given that it requires three active "approve"s to override one
> dangling "requested changes" and there generally aren't four committers
> simultaneously interacting with a single pull request.
>
> -Brian
>
>
>
> On Thu, Jun 6, 2019 at 9:51 AM Andor Molnár <an...@apache.org> wrote:
>
> > Exactly.
> >
> >
> > On 2019. 06. 06. 14:51, Flavio Junqueira wrote:
> > > That's covered in the project bylaws, right?
> > >
> > > https://zookeeper.apache.org/bylaws.html <
> > https://zookeeper.apache.org/bylaws.html>
> > >
> > > -Flavio
> > >
> > >> On 6 Jun 2019, at 13:49, Enrico Olivelli <eo...@gmail.com> wrote:
> > >>
> > >> Il gio 6 giu 2019, 12:44 Andor Molnar <andor@apache.org <mailto:
> > andor@apache.org>> ha scritto:
> > >>
> > >>> Hi folks,
> > >>>
> > >>> I’ve seen 2 patches committed recently with “-1s" from committers on
> > it.
> > >>>
> > >>> https://github.com/apache/zookeeper/pull/899 <
> > >>> https://github.com/apache/zookeeper/pull/899>
> > >>> https://github.com/apache/zookeeper/pull/944 <
> > >>> https://github.com/apache/zookeeper/pull/944>
> > >>>
> > >>> Not a big deal in this case and I think they were in a good shape and
> > >>> ready to commit, but I’d like to clarify how do we handle voting on
> > pull
> > >>> requests. We use github to prepare patches by creating pull requests.
> > >>> Github also has a feature of “reviewing” which means that reviewers
> are
> > >>> able to “approve”, “comment” and “request for changes”. In terms of
> > voting
> > >>> this means:
> > >>>
> > >>> - “approve” = +1
> > >>> - “comment” = 0
> > >>> - “request for changes” = -1
> > >>>
> > >> We should enhance the script (we already did it on Bookkeeper for
> > instance)
> > >>
> > >>> In order to commit a patch we need at least 2 binding +1s without
> > binding
> > >>> -1. Committers/PMCs are able to veto this way.
> > >>>
> > >>> Do we agree on this process completely?
> > >>>
> > >> Sure
> > >>
> > >>> I know that activity in ZooKeeper community is usually very flaky and
> > >>> sometimes it’s hard to find committers to review patches.
> > >>
> > >> We have a new wave of contributions and new committers, so fortunately
> > this
> > >> is changing.
> > >>
> > >>
> > >> In these cases we usually just commit smaller patches with a single
> > binding
> > >>> vote, but I think we should be more careful about binding -1s.
> > >>>
> > >>> Please in the future if you see my -1 on a patch which you think is
> > ready
> > >>> to commit, bug me as hard as it takes. I’ll make every effort to
> > review as
> > >>> soon as possible and apologies for any delay.
> > >>>
> > >> Sure.
> > >>
> > >>
> > >> Enrico
> > >>
> > >>
> > >>> Thanks,
> > >>> Andor
> > >
> >
>

Re: [DISCUSS] Voting on pull requests

[Brian Nixon <br...@gmail.com>]

The community should absolutely stand by its bylaws. :)

Those two pull requests (899 and 944) were mine so I'd like to sketch what
I saw as a contributor and hopefully figure out a healthier way forward.
Both were opened in the last two months and got reviewer action within
days. There was a reasonable back and forth between comments and new
commits addressing concerns. Some of the concerns were back by "requested
changes" as is proper. Subsequent commits addressed those comments just the
same as other review comments and this is where I think communication fell
down. I was expecting each reviewer to follow up with some change to their
vote status. In one case, the reviewer continued to interact with the pull
request without clarifying whether more changes were expected. This
generates confusion at a minimum and both pull requests had -1 votes that
had been addressed for three weeks before they were eventually merged.

Ultimately, as a contributor it is our responsibility to push for our
contribution and I could certainly have more aggressively pinged folks on
my pull requests (busy people need helpful reminders sometimes). I'd
recommend though, a bit of followup when using "requested changes" on pull
requests given that it requires three active "approve"s to override one
dangling "requested changes" and there generally aren't four committers
simultaneously interacting with a single pull request.

-Brian



On Thu, Jun 6, 2019 at 9:51 AM Andor Molnár <an...@apache.org> wrote:

> Exactly.
>
>
> On 2019. 06. 06. 14:51, Flavio Junqueira wrote:
> > That's covered in the project bylaws, right?
> >
> > https://zookeeper.apache.org/bylaws.html <
> https://zookeeper.apache.org/bylaws.html>
> >
> > -Flavio
> >
> >> On 6 Jun 2019, at 13:49, Enrico Olivelli <eo...@gmail.com> wrote:
> >>
> >> Il gio 6 giu 2019, 12:44 Andor Molnar <andor@apache.org <mailto:
> andor@apache.org>> ha scritto:
> >>
> >>> Hi folks,
> >>>
> >>> I’ve seen 2 patches committed recently with “-1s" from committers on
> it.
> >>>
> >>> https://github.com/apache/zookeeper/pull/899 <
> >>> https://github.com/apache/zookeeper/pull/899>
> >>> https://github.com/apache/zookeeper/pull/944 <
> >>> https://github.com/apache/zookeeper/pull/944>
> >>>
> >>> Not a big deal in this case and I think they were in a good shape and
> >>> ready to commit, but I’d like to clarify how do we handle voting on
> pull
> >>> requests. We use github to prepare patches by creating pull requests.
> >>> Github also has a feature of “reviewing” which means that reviewers are
> >>> able to “approve”, “comment” and “request for changes”. In terms of
> voting
> >>> this means:
> >>>
> >>> - “approve” = +1
> >>> - “comment” = 0
> >>> - “request for changes” = -1
> >>>
> >> We should enhance the script (we already did it on Bookkeeper for
> instance)
> >>
> >>> In order to commit a patch we need at least 2 binding +1s without
> binding
> >>> -1. Committers/PMCs are able to veto this way.
> >>>
> >>> Do we agree on this process completely?
> >>>
> >> Sure
> >>
> >>> I know that activity in ZooKeeper community is usually very flaky and
> >>> sometimes it’s hard to find committers to review patches.
> >>
> >> We have a new wave of contributions and new committers, so fortunately
> this
> >> is changing.
> >>
> >>
> >> In these cases we usually just commit smaller patches with a single
> binding
> >>> vote, but I think we should be more careful about binding -1s.
> >>>
> >>> Please in the future if you see my -1 on a patch which you think is
> ready
> >>> to commit, bug me as hard as it takes. I’ll make every effort to
> review as
> >>> soon as possible and apologies for any delay.
> >>>
> >> Sure.
> >>
> >>
> >> Enrico
> >>
> >>
> >>> Thanks,
> >>> Andor
> >
>

Re: [DISCUSS] Voting on pull requests

[Andor Molnár <an...@apache.org>]

Exactly.


On 2019. 06. 06. 14:51, Flavio Junqueira wrote:
> That's covered in the project bylaws, right?
>
> https://zookeeper.apache.org/bylaws.html <https://zookeeper.apache.org/bylaws.html>
>
> -Flavio
>
>> On 6 Jun 2019, at 13:49, Enrico Olivelli <eo...@gmail.com> wrote:
>>
>> Il gio 6 giu 2019, 12:44 Andor Molnar <andor@apache.org <ma...@apache.org>> ha scritto:
>>
>>> Hi folks,
>>>
>>> I’ve seen 2 patches committed recently with “-1s" from committers on it.
>>>
>>> https://github.com/apache/zookeeper/pull/899 <
>>> https://github.com/apache/zookeeper/pull/899>
>>> https://github.com/apache/zookeeper/pull/944 <
>>> https://github.com/apache/zookeeper/pull/944>
>>>
>>> Not a big deal in this case and I think they were in a good shape and
>>> ready to commit, but I’d like to clarify how do we handle voting on pull
>>> requests. We use github to prepare patches by creating pull requests.
>>> Github also has a feature of “reviewing” which means that reviewers are
>>> able to “approve”, “comment” and “request for changes”. In terms of voting
>>> this means:
>>>
>>> - “approve” = +1
>>> - “comment” = 0
>>> - “request for changes” = -1
>>>
>> We should enhance the script (we already did it on Bookkeeper for instance)
>>
>>> In order to commit a patch we need at least 2 binding +1s without binding
>>> -1. Committers/PMCs are able to veto this way.
>>>
>>> Do we agree on this process completely?
>>>
>> Sure
>>
>>> I know that activity in ZooKeeper community is usually very flaky and
>>> sometimes it’s hard to find committers to review patches.
>>
>> We have a new wave of contributions and new committers, so fortunately this
>> is changing.
>>
>>
>> In these cases we usually just commit smaller patches with a single binding
>>> vote, but I think we should be more careful about binding -1s.
>>>
>>> Please in the future if you see my -1 on a patch which you think is ready
>>> to commit, bug me as hard as it takes. I’ll make every effort to review as
>>> soon as possible and apologies for any delay.
>>>
>> Sure.
>>
>>
>> Enrico
>>
>>
>>> Thanks,
>>> Andor
>

Re: [DISCUSS] Voting on pull requests

[Flavio Junqueira <fp...@apache.org>]

That's covered in the project bylaws, right?

https://zookeeper.apache.org/bylaws.html <https://zookeeper.apache.org/bylaws.html>

-Flavio

> On 6 Jun 2019, at 13:49, Enrico Olivelli <eo...@gmail.com> wrote:
> 
> Il gio 6 giu 2019, 12:44 Andor Molnar <andor@apache.org <ma...@apache.org>> ha scritto:
> 
>> Hi folks,
>> 
>> I’ve seen 2 patches committed recently with “-1s" from committers on it.
>> 
>> https://github.com/apache/zookeeper/pull/899 <
>> https://github.com/apache/zookeeper/pull/899>
>> https://github.com/apache/zookeeper/pull/944 <
>> https://github.com/apache/zookeeper/pull/944>
>> 
>> Not a big deal in this case and I think they were in a good shape and
>> ready to commit, but I’d like to clarify how do we handle voting on pull
>> requests. We use github to prepare patches by creating pull requests.
>> Github also has a feature of “reviewing” which means that reviewers are
>> able to “approve”, “comment” and “request for changes”. In terms of voting
>> this means:
>> 
>> - “approve” = +1
>> - “comment” = 0
>> - “request for changes” = -1
>> 
> 
> We should enhance the script (we already did it on Bookkeeper for instance)
> 
>> 
>> In order to commit a patch we need at least 2 binding +1s without binding
>> -1. Committers/PMCs are able to veto this way.
>> 
>> Do we agree on this process completely?
>> 
> 
> Sure
> 
>> 
>> I know that activity in ZooKeeper community is usually very flaky and
>> sometimes it’s hard to find committers to review patches.
> 
> 
> We have a new wave of contributions and new committers, so fortunately this
> is changing.
> 
> 
> In these cases we usually just commit smaller patches with a single binding
>> vote, but I think we should be more careful about binding -1s.
>> 
>> Please in the future if you see my -1 on a patch which you think is ready
>> to commit, bug me as hard as it takes. I’ll make every effort to review as
>> soon as possible and apologies for any delay.
>> 
> 
> Sure.
> 
> 
> Enrico
> 
> 
>> Thanks,
>> Andor

Re: [DISCUSS] Voting on pull requests

[Enrico Olivelli <eo...@gmail.com>]

Il gio 6 giu 2019, 12:44 Andor Molnar <an...@apache.org> ha scritto:

> Hi folks,
>
> I’ve seen 2 patches committed recently with “-1s" from committers on it.
>
> https://github.com/apache/zookeeper/pull/899 <
> https://github.com/apache/zookeeper/pull/899>
> https://github.com/apache/zookeeper/pull/944 <
> https://github.com/apache/zookeeper/pull/944>
>
> Not a big deal in this case and I think they were in a good shape and
> ready to commit, but I’d like to clarify how do we handle voting on pull
> requests. We use github to prepare patches by creating pull requests.
> Github also has a feature of “reviewing” which means that reviewers are
> able to “approve”, “comment” and “request for changes”. In terms of voting
> this means:
>
> - “approve” = +1
> - “comment” = 0
> - “request for changes” = -1
>

We should enhance the script (we already did it on Bookkeeper for instance)

>
> In order to commit a patch we need at least 2 binding +1s without binding
> -1. Committers/PMCs are able to veto this way.
>
> Do we agree on this process completely?
>

Sure

>
> I know that activity in ZooKeeper community is usually very flaky and
> sometimes it’s hard to find committers to review patches.


We have a new wave of contributions and new committers, so fortunately this
is changing.


In these cases we usually just commit smaller patches with a single binding
> vote, but I think we should be more careful about binding -1s.
>
> Please in the future if you see my -1 on a patch which you think is ready
> to commit, bug me as hard as it takes. I’ll make every effort to review as
> soon as possible and apologies for any delay.
>

Sure.


Enrico


> Thanks,
> Andor
>
>
>