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
