Il ven 14 giu 2019, 23:34 Andor Molnar <[email protected]> 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 <[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 > > > > > > > > > > > > > > > > > > > > >
