You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apex.apache.org by Pramod Immaneni <pr...@gmail.com> on 2019/01/26 01:21:47 UTC
Contribution and committer guidelines
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
Re: Contribution and committer guidelines
Posted by Vlad Rozov <vr...@apache.org>.
It was always the case that -1 needs to be accompanied by justification [1]. I don’t recollect any PR where it was stopped by a veto due to missing code refactoring.
VETOS <https://www.apache.org/foundation/voting.html#Veto>
A code-modification proposal may be stopped dead in its tracks by a -1 vote by a qualified voter. This constitutes a veto, and it cannot be overruled nor overridden by anyone. Vetos stand until and unless withdrawn by their casters.
To prevent vetos from being used capriciously, they must be accompanied by a technical justification showing why the change is bad (opens a security exposure, negatively affects performance, etc. ). A veto without a justification is invalid and has no weight.
[1] https://www.apache.org/foundation/voting.html <https://www.apache.org/foundation/voting.html>
> On Jan 28, 2019, at 12:58, amol kekre <am...@gmail.com> wrote:
>
> Vlad,
> This thread is in regards to code review itself, and we are discussing what
> conditions can a reviewer do a -1. In case a reviewer is ignoring the new
> policies and continues to give -1 using previous thought process, we will
> need to have a mechanism where the -1 is overridden. -1 needs an
> explanation that fits with what we come up with on this thread.
>
> Amol
>
>
> On Mon, Jan 28, 2019 at 12:45 PM Vlad Rozov <vr...@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.
>>
>> 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.
>>
>> Thank you,
>>
>> Vlad
>>
>>
>>> On Jan 28, 2019, at 11:58, Pramod Immaneni <pr...@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 <vr...@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
>>
>>
Re: Contribution and committer guidelines
Posted by amol kekre <am...@gmail.com>.
Vlad,
This thread is in regards to code review itself, and we are discussing what
conditions can a reviewer do a -1. In case a reviewer is ignoring the new
policies and continues to give -1 using previous thought process, we will
need to have a mechanism where the -1 is overridden. -1 needs an
explanation that fits with what we come up with on this thread.
Amol
On Mon, Jan 28, 2019 at 12:45 PM Vlad Rozov <vr...@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.
>
> 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.
>
> Thank you,
>
> Vlad
>
>
> > On Jan 28, 2019, at 11:58, Pramod Immaneni <pr...@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 <vr...@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
>
>
Re: Contribution and committer guidelines
Posted by priyanka gugale <pr...@apache.org>.
Hi,
IMO let's not spend lot of time in digging up past just to check proofs for
people's opinion like hight bar for code submission or long pending
reviews. If it's a common feeling amongst most of committers and they are
saying it, we should find some way to work our way to improve things. May
be we do monitor next 5-10 PRs closely and then refine our policies "if
required", rather than spending lot of time in finding old PRs and trying
to remember what happened back then.
-Priyanka
On Wed, Jan 30, 2019 at 10:06 AM Vlad Rozov <vr...@apache.org> wrote:
>
>
> > On Jan 29, 2019, at 20:04, Justin Mclean <ju...@classsoftware.com>
> wrote:
> >
> > Hi,
> >
> >> Badly written code, missing and failing unit tests make it harder for
> everyone to contribute.
> >
> > IMO having code that can be easily improved makes it easier for people
> to contribute.
>
> Possibly you are interested in fixing badly written code but my prior
> experience is that developers are more interested in implementing new
> features and functionality. Personally, I prefer to stay away from badly
> designed and written code.
>
> >
> >> Justin, -1 are infrequent during PR reviews in Apex.
> >
> > I could easily find 5 from a quick search, which in my experience is
> unusual, but it may be different on other projects. Most projects I’ve been
> involved in almost never have vetos as things are usually sorted out by
> discussion or worked on colatoratively to find a solution before it gets to
> that point. Can I suggest in future rather than casting a veto right away
> people try and work with the person who made the contribution and improve
> what they have contributed.
>
> Please list them all here and let’s take a look whether or not -1 were
> justified on a case by case basis.
>
> >
> > Thanks,
> > Justin
>
>
Re: Contribution and committer guidelines
Posted by Vlad Rozov <vr...@apache.org>.
> On Jan 29, 2019, at 20:04, Justin Mclean <ju...@classsoftware.com> wrote:
>
> Hi,
>
>> Badly written code, missing and failing unit tests make it harder for everyone to contribute.
>
> IMO having code that can be easily improved makes it easier for people to contribute.
Possibly you are interested in fixing badly written code but my prior experience is that developers are more interested in implementing new features and functionality. Personally, I prefer to stay away from badly designed and written code.
>
>> Justin, -1 are infrequent during PR reviews in Apex.
>
> I could easily find 5 from a quick search, which in my experience is unusual, but it may be different on other projects. Most projects I’ve been involved in almost never have vetos as things are usually sorted out by discussion or worked on colatoratively to find a solution before it gets to that point. Can I suggest in future rather than casting a veto right away people try and work with the person who made the contribution and improve what they have contributed.
Please list them all here and let’s take a look whether or not -1 were justified on a case by case basis.
>
> Thanks,
> Justin
Re: Contribution and committer guidelines
Posted by Justin Mclean <ju...@classsoftware.com>.
Hi,
> Badly written code, missing and failing unit tests make it harder for everyone to contribute.
IMO having code that can be easily improved makes it easier for people to contribute.
> Justin, -1 are infrequent during PR reviews in Apex.
I could easily find 5 from a quick search, which in my experience is unusual, but it may be different on other projects. Most projects I’ve been involved in almost never have vetos as things are usually sorted out by discussion or worked on colatoratively to find a solution before it gets to that point. Can I suggest in future rather than casting a veto right away people try and work with the person who made the contribution and improve what they have contributed.
Thanks,
Justin
Re: Contribution and committer guidelines
Posted by Vlad Rozov <vr...@apache.org>.
Sanjay,
I never threaten to -1 https://github.com/apache/apex-core/pull/607 <https://github.com/apache/apex-core/pull/607>. The -1 would apply to the proposal to drop SSL support and your suggestion to check on user@apex. The discussion regarding SSL including backward compatibility needs to be brought to dev@apex. Apex is not an enterprise project and it requires the community consensus prior to dropping SSL support or moving forward with incompatible changes.
Badly written code, missing and failing unit tests make it harder for everyone to contribute. It is the case that Aaron now faces when he tried to upgrade Hadoop version in the PR https://github.com/apache/apex-core/pull/607 <https://github.com/apache/apex-core/pull/607>. Travis CI and Jenkins PR builds must pass before a PR can be merged. This applies to all PRs including https://github.com/apache/apex-core/pull/607 <https://github.com/apache/apex-core/pull/607> and https://github.com/apache/apex-core/pull/569 <https://github.com/apache/apex-core/pull/569> that you mentioned. I put -1 on it as you asked to merge the PR while unit tests were still failing.
Amol, when you mentioned that -1 are not rare, can you provide examples of 10 PRs that have vetos.
Justin, -1 are infrequent during PR reviews in Apex. Discussions and long list of comments and suggestions to improve code is a common practice and equally applies to Apex. For example Drill (https://github.com/apache/drill/pull/1504 <https://github.com/apache/drill/pull/1504>) and Flink (https://github.com/apache/flink/pull/7351 <https://github.com/apache/flink/pull/7351>).
Thank you,
Vlad
> On Jan 29, 2019, at 17:34, Sanjay Pujare <sa...@gmail.com> wrote:
>
> I too agree with the sentiments expressed here and good to know there are
> people who think like me.
>
> In terms of examples, you can see the PR
> https://github.com/apache/apex-core/pull/569 which has a long discussion
> about failing tests and who has the onus to prove what etc. In a recent PR
> that abossert
> <https://github.com/apache/apex-core/issues?q=is%3Apr+is%3Aopen+author%3Aabossert>
> opened,
> Vlad threatened to -1 an idea that was not even suggested (
> https://github.com/apache/apex-core/pull/607#discussion_r248498412) . As
> Amol mentioned vetoes are not rare here and I didn't have to wait too long
> for an example to turn up.
>
>
> On Tue, Jan 29, 2019 at 3:14 PM amol kekre <am...@gmail.com> wrote:
>
>> Justin,
>> I agree with your thoughts. Vetos are not rare in Apex. We are trying to
>> figure a way to get there.
>>
>> Amol
>>
>> On Tue, Jan 29, 2019 at 3:01 PM Justin Mclean <ju...@classsoftware.com>
>> wrote:
>>
>>> Hi,
>>>
>>> If someone submits what you think is poor quality code just point it out
>>> to them and ask them to fix it or even better fix it yourself to show
>> them
>>> what is expected. Vetoing something list that seems a little heavy handed
>>> and is not the best way to encourage community growth. It’s better to
>>> improve the quality of others contributions rather than blocking them
>> from
>>> contributing. Vetos in practice are very rare, how many have actually
>>> occurred in this project? Wouldn't it be better to focus on practical
>> ways
>>> to get people involved and increase contribution rather than hypothetical
>>> situations of when to veto a code change?
>>>
>>> Thanks,
>>> Justin
>>
Re: Contribution and committer guidelines
Posted by Sanjay Pujare <sa...@gmail.com>.
I too agree with the sentiments expressed here and good to know there are
people who think like me.
In terms of examples, you can see the PR
https://github.com/apache/apex-core/pull/569 which has a long discussion
about failing tests and who has the onus to prove what etc. In a recent PR
that abossert
<https://github.com/apache/apex-core/issues?q=is%3Apr+is%3Aopen+author%3Aabossert>
opened,
Vlad threatened to -1 an idea that was not even suggested (
https://github.com/apache/apex-core/pull/607#discussion_r248498412) . As
Amol mentioned vetoes are not rare here and I didn't have to wait too long
for an example to turn up.
On Tue, Jan 29, 2019 at 3:14 PM amol kekre <am...@gmail.com> wrote:
> Justin,
> I agree with your thoughts. Vetos are not rare in Apex. We are trying to
> figure a way to get there.
>
> Amol
>
> On Tue, Jan 29, 2019 at 3:01 PM Justin Mclean <ju...@classsoftware.com>
> wrote:
>
> > Hi,
> >
> > If someone submits what you think is poor quality code just point it out
> > to them and ask them to fix it or even better fix it yourself to show
> them
> > what is expected. Vetoing something list that seems a little heavy handed
> > and is not the best way to encourage community growth. It’s better to
> > improve the quality of others contributions rather than blocking them
> from
> > contributing. Vetos in practice are very rare, how many have actually
> > occurred in this project? Wouldn't it be better to focus on practical
> ways
> > to get people involved and increase contribution rather than hypothetical
> > situations of when to veto a code change?
> >
> > Thanks,
> > Justin
>
Re: Contribution and committer guidelines
Posted by amol kekre <am...@gmail.com>.
Justin,
I agree with your thoughts. Vetos are not rare in Apex. We are trying to
figure a way to get there.
Amol
On Tue, Jan 29, 2019 at 3:01 PM Justin Mclean <ju...@classsoftware.com>
wrote:
> Hi,
>
> If someone submits what you think is poor quality code just point it out
> to them and ask them to fix it or even better fix it yourself to show them
> what is expected. Vetoing something list that seems a little heavy handed
> and is not the best way to encourage community growth. It’s better to
> improve the quality of others contributions rather than blocking them from
> contributing. Vetos in practice are very rare, how many have actually
> occurred in this project? Wouldn't it be better to focus on practical ways
> to get people involved and increase contribution rather than hypothetical
> situations of when to veto a code change?
>
> Thanks,
> Justin
Re: Contribution and committer guidelines
Posted by Justin Mclean <ju...@classsoftware.com>.
Hi,
If someone submits what you think is poor quality code just point it out to them and ask them to fix it or even better fix it yourself to show them what is expected. Vetoing something list that seems a little heavy handed and is not the best way to encourage community growth. It’s better to improve the quality of others contributions rather than blocking them from contributing. Vetos in practice are very rare, how many have actually occurred in this project? Wouldn't it be better to focus on practical ways to get people involved and increase contribution rather than hypothetical situations of when to veto a code change?
Thanks,
Justin
Re: Contribution and committer guidelines
Posted by Vlad Rozov <vr...@apache.org>.
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 <am...@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 <pr...@gmail.com>
> wrote:
>
>> On Mon, Jan 28, 2019 at 12:45 PM Vlad Rozov <vr...@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 <pr...@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 <vr...@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
>>
Re: Contribution and committer guidelines
Posted by amol kekre <am...@gmail.com>.
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 <pr...@gmail.com>
wrote:
> On Mon, Jan 28, 2019 at 12:45 PM Vlad Rozov <vr...@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 <pr...@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 <vr...@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
>
Re: Contribution and committer guidelines
Posted by Pramod Immaneni <pr...@gmail.com>.
On Mon, Jan 28, 2019 at 12:45 PM Vlad Rozov <vr...@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 <pr...@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 <vr...@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
Re: Contribution and committer guidelines
Posted by Vlad Rozov <vr...@apache.org>.
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.
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.
Thank you,
Vlad
> On Jan 28, 2019, at 11:58, Pramod Immaneni <pr...@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 <vr...@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 <sa...@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
Re: Contribution and committer guidelines
Posted by Pramod Immaneni <pr...@gmail.com>.
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 <vr...@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 <sa...@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
Re: Contribution and committer guidelines
Posted by Vlad Rozov <vr...@apache.org>.
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 <ch...@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 <sa...@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
>>>>
>>>
>>
Re: Contribution and committer guidelines
Posted by Chinmay Kolhatkar <ch...@gmail.com>.
+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 <sa...@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
> > >
> >
>
Re: Contribution and committer guidelines
Posted by amol kekre <am...@gmail.com>.
+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 <sa...@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
> >
>
Re: Contribution and committer guidelines
Posted by Sanjay Pujare <sa...@gmail.com>.
+1
On Fri, Jan 25, 2019 at 5:20 PM Pramod Immaneni <pr...@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
>