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 <[email protected]> wrote: > 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 > > > > > > > > > > > > > > >
