zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Enrico Olivelli <eolive...@gmail.com>
Subject Re: [DISCUSS] Voting on pull requests
Date Fri, 14 Jun 2019 21:37:19 GMT
Il ven 14 giu 2019, 23:34 Andor Molnar <andor@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 <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