zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Patrick Hunt <ph...@apache.org>
Subject Re: [DISCUSS] Voting on pull requests
Date Thu, 13 Jun 2019 18:18:19 GMT
On Thu, Jun 13, 2019 at 11:08 AM Fangmin Lv <lvfangmin@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 <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