Home | About | Sematext search-lucene.com search-hadoop.com
NEW: Monitor These Apps!
elasticsearch, apache solr, apache hbase, hadoop, redis, casssandra, amazon cloudwatch, mysql, memcached, apache kafka, apache zookeeper, apache storm, ubuntu, centOS, red hat, debian, puppet labs, java, senseiDB
 Search Hadoop and all its subprojects:

Switch to Threaded View
Hadoop >> mail # dev >> Requirements for patch review


Copy link to this message
-
Requirements for patch review
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
NEW: Monitor These Apps!
elasticsearch, apache solr, apache hbase, hadoop, redis, casssandra, amazon cloudwatch, mysql, memcached, apache kafka, apache zookeeper, apache storm, ubuntu, centOS, red hat, debian, puppet labs, java, senseiDB