It was always the case that -1 needs to be accompanied by justification [1]. I don’t recollect any PR where it was stopped by a veto due to missing code refactoring.
VETOS <https://www.apache.org/foundation/voting.html#Veto> A code-modification proposal may be stopped dead in its tracks by a -1 vote by a qualified voter. This constitutes a veto, and it cannot be overruled nor overridden by anyone. Vetos stand until and unless withdrawn by their casters. To prevent vetos from being used capriciously, they must be accompanied by a technical justification showing why the change is bad (opens a security exposure, negatively affects performance, etc. ). A veto without a justification is invalid and has no weight. [1] https://www.apache.org/foundation/voting.html <https://www.apache.org/foundation/voting.html> > On Jan 28, 2019, at 12:58, amol kekre <amolhke...@gmail.com> wrote: > > Vlad, > This thread is in regards to code review itself, and we are discussing what > conditions can a reviewer do a -1. In case a reviewer is ignoring the new > policies and continues to give -1 using previous thought process, we will > need to have a mechanism where the -1 is overridden. -1 needs an > explanation that fits with what we come up with on this thread. > > Amol > > > On Mon, Jan 28, 2019 at 12:45 PM Vlad Rozov <vro...@apache.org> wrote: > >> IMO, the performance criteria is quite vague and it needs to be taken on a >> case by case basis. Fixing not critical bug or adding minor functionality >> is different from fixing security issue or data loss/corruption and while >> the first one never justifies performance degradation, the second one may >> justify a significant performance degradation. >> >> My question is specific to refactoring and/or code quality. Whether this >> policy is accepted or not, -1 in code review is still a veto. >> >> Thank you, >> >> Vlad >> >> >>> On Jan 28, 2019, at 11:58, Pramod Immaneni <pramod.imman...@gmail.com> >> wrote: >>> >>> Amol regarding performance my thoughts were along similar lines but was >>> concerned about performance degradation to the real-time path, that new >>> changes can bring in. I would use stronger language than "do not degrade >>> current performance significantly" at least for the real-time path, we >>> could say something like "real-time path should have as minimum >> performance >>> degradation as possible". Regarding logic flaws, typically it is cut and >>> dry and not very subjective. There are exceptions of course. Also, what I >>> have seen with functionality testing, at least in this context where >> there >>> is no dedicated QA testing the code, is that not all code paths and >>> combinations are exercised. Fixing, logic issues in the lower level >>> functions etc, of the code, leads to overall better quality. We could >> have >>> the language in the guideline such that it defaults to resolving all >>> logical flaws but also leaves the door open for exceptions. If there are >>> any scenarios you have in mind, we can discuss those and call it out as >>> part of those exceptions. >>> >>> Regarding Vlad's question, I would encourage folks who brought up this >>> point in the earlier discussion, point to examples where they personally >>> faced this problem. In my case I have seen long delays in merging PRs, >>> sometimes months, not because the reviewer(s) didn't have time but >> because >>> it was stuck in back and forth discussions and disagreement on one or >>> two points between contributor and reviewer(s). In the bigger scheme of >>> things, in my opinion, those points were trivial and caused more angst >>> than what would have taken to correct them in the future, had we gone one >>> way vs the other. I have seen this both as a contributor and as >> co-reviewer >>> from my peer reviewers in the PR. I can dig into the archives and find >>> those if needed. >>> >>> Thanks >>> >>> On Mon, Jan 28, 2019 at 8:43 AM Vlad Rozov <vro...@apache.org> wrote: >>> >>>> Is there an example from prior PRs where it was not accepted/merged due >> to >>>> a disagreement between a contributor and a committer on the amount of >>>> refactoring or code quality? >>>> >>>> Thank you, >>>> >>>> Vlad >>>> >>>>> On Jan 27, 2019, at 06:56, Chinmay Kolhatkar < >>>> chinmaykolhatka...@gmail.com> wrote: >>>>> >>>>> +1. >>>>> >>>>> On Sat, 26 Jan 2019, 11:56 pm amol kekre <amolhke...@gmail.com wrote: >>>>> >>>>>> +1 for this proposal. The only caveat I have is >>>>>> -> "acceptable performance and resolving logical flaws identified >> during >>>>>> the review process" >>>>>> >>>>>> is subjective. Functionally working should cover any logical issues. >>>>>> Performance should be applicable only to bug fixes and small >>>> enhancements >>>>>> to current features. I will word is as "do not degrade current >>>> performance >>>>>> significantly". >>>>>> >>>>>> Amol >>>>>> >>>>>> >>>>>> On Fri, Jan 25, 2019 at 9:41 PM Sanjay Pujare < >> sanjay.puj...@gmail.com> >>>>>> wrote: >>>>>> >>>>>>> +1 >>>>>>> >>>>>>> >>>>>>> On Fri, Jan 25, 2019 at 5:20 PM Pramod Immaneni < >>>>>> pramod.imman...@gmail.com >>>>>>>> >>>>>>> wrote: >>>>>>> >>>>>>>> Our contributor and committer guidelines haven't changed in a while. >>>> In >>>>>>>> light of the discussion that happened a few weeks ago, where >>>>>>>> high commit threshold was cited as one of the factors discouraging >>>>>>>> submissions, I suggest we discuss some ideas and see if the >> guidelines >>>>>>>> should be updated. >>>>>>>> >>>>>>>> I have one. We pick some reasonable time period like a month after a >>>> PR >>>>>>> is >>>>>>>> submitted. If the PR review process is still going on *and* there >> is a >>>>>>>> disagreement between the contributor and reviewer, we will look to >> see >>>>>> if >>>>>>>> the submission satisfies some acceptable criteria and if it does we >>>>>>> accept >>>>>>>> it. We can discuss what those criteria should be in this thread. >>>>>>>> >>>>>>>> The basics should be met, such as code format, license, copyright, >>>> unit >>>>>>>> tests passing, functionality working, acceptable performance and >>>>>>> resolving >>>>>>>> logical flaws identified during the review process. Beyond that, if >>>>>> there >>>>>>>> is a disagreement with code quality or refactor depth between >>>> committer >>>>>>> and >>>>>>>> contributor or the contributor agrees but does not want to spend >> more >>>>>>> time >>>>>>>> on it at that moment, we accept the submission and create a separate >>>>>> JIRA >>>>>>>> to track any future work. We can revisit the policy in future once >>>> code >>>>>>>> submissions have picked up and do what's appropriate at that time. >>>>>>>> >>>>>>>> Thanks >>>>>>>> >>>>>>> >>>>>> >>>> >>>> >>> >>> -- >>> Thanks, >>> Pramod >>> http://ts.la/pramod3443 >> >>
