zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andor Molnar <an...@cloudera.com.INVALID>
Subject Re: [DISCUSS] Voting on pull requests
Date Fri, 14 Jun 2019 21:34:10 GMT
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 <phunt@apache.org> wrote:

> 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