Vlad, We are discussing what qualifies as " technical justification". The proposal also is for putting a time bound on the process.
Amol On Mon, Jan 28, 2019 at 1:36 PM Pramod Immaneni <pramod.imman...@gmail.com> wrote: > 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 >
