zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Fangmin Lv <lvfang...@gmail.com>
Subject Re: [DISCUSS] Voting on pull requests
Date Thu, 13 Jun 2019 18:07:51 GMT
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 <brian.nixon.cs@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 <andor@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 <eolivelli@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
> > >
> >
>

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