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
