Personally I don’t see how technical justification can be formalized. As I already mentioned performance degradation should be considered as technical justification in one case and it is not in others. I don’t recollect any PR from the past that were -1 due to missing code refactoring (at the same time it is perfectly fine to discuss amount of code refactoring during PR review) and it was never considered to be a valid technical justification to block a PR. A badly written code is a valid technical justification not to accept it as it makes much harder for others to contribute, so I don’t see why to accept one contribution over others. IMO, code quality of a submission needs to match existing code or be better (what can be accepted in Malhar and core is different).
Yes, in the attic discussion thread there were several complains about “high bar” but when I asked for details/examples nobody replied. Thank you, Vlad > On Jan 28, 2019, at 20:35, amol kekre <amolhke...@gmail.com> wrote: > > 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 >>
