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 <[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 > > > > > >
