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