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. > Yes it is only a guideline and the situation demands like security issue would necessiate or determine the appropriate thing to do. > > 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. > So we are discussing how we can improve the situation so contributors feel like contributing to the project as opposed to staying away from it, which I think we all agree is happening. In your email thread about attic discussion, a high bar was cited as the reason by at least 3 members and it has come up in the past as well. Hence this discussion on what we to do in this aspect. Could we relax some requirements without leading to unstable or unreliable software. The alternative is nothing would change and those contributors will keep away and the paucity of contributions will continue. It is wishful thinking but if some contributors come back and start contributing again, others might too and who knows in future we may be able to go back to the high bar. Thanks > 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 > > -- Thanks, Pramod http://ts.la/pramod3443
