Requirements for patch review

[Todd Lipcon <to...@cloudera.com>]

Hi folks,

Some discussion between Nicholas, Aaron, and me started in the
comments of HDFS-3168 which I think is better exposed on the mailing
list instead of trailing an already-committed JIRA.

The question at hand is what the policy is with regarding our
review-then-commit policies. The bylaws state:

>>>
*Code Change*
A change made to a codebase of the project and committed by a
committer. This includes source code, documentation, website content,
etc. Lazy consensus of active committers, but with a minimum of one
+1. The code can be committed after the first +1, unless the code
change represents a merge from a branch, in which case three +1s are
required.
<<<

The wording here is ambiguous, though, whether the committer who
provides the minimum one +1 may also be the author of the code change.
If so, that would seem to imply that committers may always make code
changes by merely +1ing their own patches, which seems counter to the
whole point of "review-then-commit". So, I'm pretty sure that's not
what it means.

The question that came up, however, was whether a non-committer
contributor may provide a binding +1 for a patch written by a
committer. So, if I write a patch as a committer, and then a community
member reviews it, am I free to commit it without another committer
looking at it? My understanding has always been that this is not the
case, but we should clarify the by-laws if there is some ambiguity.

I would propose the following amendments:
A committer may not provide a binding +1 for his or her own patch.
However, in the case of trivial patches only, a committer may use a +1
from the problem reporter or other contributor in lieu of another
committer's +1. The definition of a trivial patch is subject to the
committer's best judgment, but in general should consist of things
such as: documentation fixes, spelling mistakes, log message changes,
or additional test cases.

I think the above strikes a reasonable balance between pragmatism for
quick changes, and keeping a rigorous review process for patches that
should have multiple experienced folks look over.

Thoughts?

Todd
-- 
Todd Lipcon
Software Engineer, Cloudera

Re: Adding documentation patches to hadoop

[Harsh J <ha...@cloudera.com>]

Amir,

Perhaps you'd prefer maintaining a Wiki page at
http://wiki.apache.org/hadoop/ for this?

Or if not, please read up on
http://wiki.apache.org/hadoop/HowToContribute. Do open up a JIRA to
discuss the specifics.

On Thu, Apr 5, 2012 at 10:05 PM, Amir Sanjar <v1...@us.ibm.com> wrote:
> We would like to add a documentation patch ( i.e. a readme file)
> to explain the build process and the requirements for hadoop on. POWER.
> What is the process?
>
> Best Regards
> Amir Sanjar
>
> Linux System Management Architect and Lead
> IBM Senior Software Engineer
> Phone# 512-286-8393
> Fax#      512-838-8858
>
>



-- 
Harsh J

Adding documentation patches to hadoop

[Amir Sanjar <v1...@us.ibm.com>]

We would like to add a documentation patch ( i.e. a readme file)
to explain the build process and the requirements for hadoop on. POWER.
What is the process?

Best Regards
Amir Sanjar

Linux System Management Architect and Lead
IBM Senior Software Engineer
Phone# 512-286-8393
Fax#      512-838-8858

Fw: Requirements for patch review

["Tsz Wo (Nicholas), Sze" <s2...@yahoo.com>]

Resent.



----- Forwarded Message -----
From: Tsz Wo Sze <sz...@yahoo.com>
To: "common-dev@hadoop.apache.org" <co...@hadoop.apache.org>
Cc: 
Sent: Wednesday, April 4, 2012 8:24 PM
Subject: Re: Requirements for patch review

>> The wording here is ambiguous, though, whether the committer who
>> provides the minimum one +1 may also be the author of the code change.
>> If so, that would seem to imply that committers may always make code
>> changes by merely +1ing their own patches, which seems counter to the
>> whole point of "review-then-commit". So, I'm pretty sure that's not
>> what it means.
>>
>> The question that came up, however, was whether a non-committer
>> contributor may provide a binding +1 for a patch written by a
>> committer. So, if I write a patch as a committer, and then a community
>> member reviews it, am I free to commit it without another committer
>> looking at it? My understanding has always been that this is not the
>> case, but we should clarify the by-laws if there is some ambiguity.
>>
>> I would propose the following amendments:
>> A committer may not provide a binding +1 for his or her own patch.
>> However, in the case of trivial patches only, a committer may use a +1
>> from the problem reporter or other contributor in lieu of another
>> committer's +1. The definition of a trivial patch is subject to the
>> committer's best judgment, but in general should consist of things
>> such as: documentation fixes, spelling mistakes, log message changes,
>> or additional test cases.

I agree that the bylaws is not clear about this.  For reviewing patches, my understanding is that any contributor, a committer or not, could review patches and the +1 counts.  I have worked on Hadoop almost five years.  This is what we are doing for a long time (if it is not from the beginning of the Hadoop project.)  Could other people confirm this?

From the HowToContribute wiki, it does advise committers to find another committer to review difficult patches: "Committers: for non-trivial changes, it is best to get another committer to review your patches before commit. ..."  It seems saying that it is okay for non-committers reviewing simple and medium patches.  Todd's amendments use different wording which seems implying a different requirement: the +1's from non-committers could be counted only for simple patches but not medium and difficult patches.

I think we should keep allowing everyone to review patches.  It slows down the development and is discouraging if non-committer's +1 does not count.  I believe the judgement of the committer who commits the patch won't commit bad code.  We have svn and we could revert patches if necessary.  Lastly, if a committer keeps committing bad code, we could exercise "Committer Removal".

BTW, does anyone know what other Apache projects do?

PS: since this is a bylaws change discussion, should we discuss it in general@?

Regards,
Tsz-Wo

Re: Requirements for patch review

[Todd Lipcon <to...@cloudera.com>]

I filed HADOOP-8248 with a diff to the bylaws.

Thanks
-Todd

On Wed, Apr 4, 2012 at 3:26 PM, Eli Collins <el...@cloudera.com> wrote:
> On Wed, Apr 4, 2012 at 2:12 PM, Todd Lipcon <to...@cloudera.com> wrote:
>> Hi folks,
>>
>> Some discussion between Nicholas, Aaron, and me started in the
>> comments of HDFS-3168 which I think is better exposed on the mailing
>> list instead of trailing an already-committed JIRA.
>>
>> The question at hand is what the policy is with regarding our
>> review-then-commit policies. The bylaws state:
>>
>>>>>
>> *Code Change*
>> A change made to a codebase of the project and committed by a
>> committer. This includes source code, documentation, website content,
>> etc. Lazy consensus of active committers, but with a minimum of one
>> +1. The code can be committed after the first +1, unless the code
>> change represents a merge from a branch, in which case three +1s are
>> required.
>> <<<
>>
>> The wording here is ambiguous, though, whether the committer who
>> provides the minimum one +1 may also be the author of the code change.
>> If so, that would seem to imply that committers may always make code
>> changes by merely +1ing their own patches, which seems counter to the
>> whole point of "review-then-commit". So, I'm pretty sure that's not
>> what it means.
>>
>> The question that came up, however, was whether a non-committer
>> contributor may provide a binding +1 for a patch written by a
>> committer. So, if I write a patch as a committer, and then a community
>> member reviews it, am I free to commit it without another committer
>> looking at it? My understanding has always been that this is not the
>> case, but we should clarify the by-laws if there is some ambiguity.
>>
>> I would propose the following amendments:
>> A committer may not provide a binding +1 for his or her own patch.
>> However, in the case of trivial patches only, a committer may use a +1
>> from the problem reporter or other contributor in lieu of another
>> committer's +1. The definition of a trivial patch is subject to the
>> committer's best judgment, but in general should consist of things
>> such as: documentation fixes, spelling mistakes, log message changes,
>> or additional test cases.
>>
>> I think the above strikes a reasonable balance between pragmatism for
>> quick changes, and keeping a rigorous review process for patches that
>> should have multiple experienced folks look over.
>>
>> Thoughts?
>>
>
> Sounds reasonable to me.
>
> Maybe file a jira with the proposed diff to the bylaws xml  and we can
> have quick vote on it here.
>
> Thanks,
> Eli



-- 
Todd Lipcon
Software Engineer, Cloudera

Re: Requirements for patch review

[Eli Collins <el...@cloudera.com>]

On Wed, Apr 4, 2012 at 2:12 PM, Todd Lipcon <to...@cloudera.com> wrote:
> Hi folks,
>
> Some discussion between Nicholas, Aaron, and me started in the
> comments of HDFS-3168 which I think is better exposed on the mailing
> list instead of trailing an already-committed JIRA.
>
> The question at hand is what the policy is with regarding our
> review-then-commit policies. The bylaws state:
>
>>>>
> *Code Change*
> A change made to a codebase of the project and committed by a
> committer. This includes source code, documentation, website content,
> etc. Lazy consensus of active committers, but with a minimum of one
> +1. The code can be committed after the first +1, unless the code
> change represents a merge from a branch, in which case three +1s are
> required.
> <<<
>
> The wording here is ambiguous, though, whether the committer who
> provides the minimum one +1 may also be the author of the code change.
> If so, that would seem to imply that committers may always make code
> changes by merely +1ing their own patches, which seems counter to the
> whole point of "review-then-commit". So, I'm pretty sure that's not
> what it means.
>
> The question that came up, however, was whether a non-committer
> contributor may provide a binding +1 for a patch written by a
> committer. So, if I write a patch as a committer, and then a community
> member reviews it, am I free to commit it without another committer
> looking at it? My understanding has always been that this is not the
> case, but we should clarify the by-laws if there is some ambiguity.
>
> I would propose the following amendments:
> A committer may not provide a binding +1 for his or her own patch.
> However, in the case of trivial patches only, a committer may use a +1
> from the problem reporter or other contributor in lieu of another
> committer's +1. The definition of a trivial patch is subject to the
> committer's best judgment, but in general should consist of things
> such as: documentation fixes, spelling mistakes, log message changes,
> or additional test cases.
>
> I think the above strikes a reasonable balance between pragmatism for
> quick changes, and keeping a rigorous review process for patches that
> should have multiple experienced folks look over.
>
> Thoughts?
>

Sounds reasonable to me.

Maybe file a jira with the proposed diff to the bylaws xml  and we can
have quick vote on it here.

Thanks,
Eli

Re: Requirements for patch review

[Robert Evans <ev...@yahoo-inc.com>]

I personally like the clarification and it is in line with how I understood the original bylaw when I read it.  I don't really want this to turn into a legal document but as this is getting more explicit with clarification it would be nice to put in a small exception for release managers when they are changing versions and setting up a new release branch.

--Bobby Evans

On 4/4/12 4:12 PM, "Todd Lipcon" <to...@cloudera.com> wrote:

Hi folks,

Some discussion between Nicholas, Aaron, and me started in the
comments of HDFS-3168 which I think is better exposed on the mailing
list instead of trailing an already-committed JIRA.

The question at hand is what the policy is with regarding our
review-then-commit policies. The bylaws state:

>>>
*Code Change*
A change made to a codebase of the project and committed by a
committer. This includes source code, documentation, website content,
etc. Lazy consensus of active committers, but with a minimum of one
+1. The code can be committed after the first +1, unless the code
change represents a merge from a branch, in which case three +1s are
required.
<<<

The wording here is ambiguous, though, whether the committer who
provides the minimum one +1 may also be the author of the code change.
If so, that would seem to imply that committers may always make code
changes by merely +1ing their own patches, which seems counter to the
whole point of "review-then-commit". So, I'm pretty sure that's not
what it means.

The question that came up, however, was whether a non-committer
contributor may provide a binding +1 for a patch written by a
committer. So, if I write a patch as a committer, and then a community
member reviews it, am I free to commit it without another committer
looking at it? My understanding has always been that this is not the
case, but we should clarify the by-laws if there is some ambiguity.

I would propose the following amendments:
A committer may not provide a binding +1 for his or her own patch.
However, in the case of trivial patches only, a committer may use a +1
from the problem reporter or other contributor in lieu of another
committer's +1. The definition of a trivial patch is subject to the
committer's best judgment, but in general should consist of things
such as: documentation fixes, spelling mistakes, log message changes,
or additional test cases.

I think the above strikes a reasonable balance between pragmatism for
quick changes, and keeping a rigorous review process for patches that
should have multiple experienced folks look over.

Thoughts?

Todd
--
Todd Lipcon
Software Engineer, Cloudera