apex-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From amol kekre <amolhke...@gmail.com>
Subject Re: Contribution and committer guidelines
Date Tue, 29 Jan 2019 04:35:34 GMT
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.immaneni@gmail.com>
wrote:

> On Mon, Jan 28, 2019 at 12:45 PM Vlad Rozov <vrozov@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.immaneni@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 <vrozov@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 <
> > >> chinmaykolhatkar01@gmail.com> wrote:
> > >>>
> > >>> +1.
> > >>>
> > >>> On Sat, 26 Jan 2019, 11:56 pm amol kekre <amolhkekre@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.pujare@gmail.com>
> > >>>> wrote:
> > >>>>
> > >>>>> +1
> > >>>>>
> > >>>>>
> > >>>>> On Fri, Jan 25, 2019 at 5:20 PM Pramod Immaneni <
> > >>>> pramod.immaneni@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
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message