On Thu, Jun 13, 2019 at 11:08 AM Fangmin Lv <[email protected]> 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 <[email protected]> > 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 <[email protected]> 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 <[email protected]> > wrote: > > > >> > > > >> Il gio 6 giu 2019, 12:44 Andor Molnar <[email protected] <mailto: > > > [email protected]>> 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 > > > > > > > > > >
