hadoop-common-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Eli Collins <...@cloudera.com>
Subject Re: Requirements for patch review
Date Wed, 04 Apr 2012 22:26:11 GMT
On Wed, Apr 4, 2012 at 2:12 PM, Todd Lipcon <todd@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

Mime
View raw message