[OpenAFS-devel] Gerrit reviews and the rate of acceptance

[Matt W. Benjamin]

Hi,

----- "Simon Wilkinson" <simonxwilkinson@gmail.com> wrote:

> On 11 Apr 2012, at 18:09, Matt W. Benjamin wrote:
> > I don't think number of reviewers is the limiting factor.  For
> example, I submitted a change to enhance Simon's contributed red-black
> tree.  Simon himself reviewed it, had no objections to committing the
> change.  I don't care if the change is committed, but I don't think
> you can legitimately fault non-present reviewers.
> 
> The rbtree change actually exposes some additional issues, so whilst
> it is a single example, I think it is worth exploring further.
> 
> Whilst the rbtree change is simple, in and of itself, it introduces a
> CUnit based set of tests. These tests aren't useful without a CUnit
> test driver, as pointed out during the review process. In fact, we
> probably don't want to push the rbtree change without that driver,
> given the general problem that we have with dead, unbuilt, code in the
> tree.

Russ had no problem with it.  This is in conference logs.

> 
> 6242 is that CUnit test suite driver. However, this raises two broader
> questions. A number of developers have spent significant effort on
> building TAP based tests, building on the work that Russ did
> originally to hook his TAP test suite up into the OpenAFS build
> machinery. Do we really want to support two different test suites, and
> two different test suite drivers in OpenAFS? 

Yes.  Every developer who worked on byte-range locking code, for example, which
is mainly but not strictly me, strongly preferred it.  We wanted to use a mainstream,
open-source unit testing suite, and CUnit is that suite.

> In addition, some of the
> code in 6242 is CDDL licensed, which adds yet another copyright to the
> OpenAFS licensing swamp, and may well cause problems for distributions
> that are CDDL averse.

I don't recall what the overlap is with CDDL.  There is a CDDL AVL implementation in
a number of components I've worked on, and I got approval from Derrick to use it.
Replacing it at some point would be manageable, at some point, but in fact, this sort of
blocker is a great example of how OpenAFS creates unnecessary obstacles to new
code, under the guise of simplifying maintenance.

> 
> Those issues with 6242 are what is blocking the (trivial) rbtree
> change. In both cases these issues have been commented on in review,
> but no resolutions agreed upon. If anything, the principal problem
> with our review process is that unless the original submitters drive
> their patches forwards, by proactively responding to review comments,
> and uploaded amended patchsets, then things will stall.

Yes, given an upstream uninterested in new work, developers will perceive it as not 
worth their time to keep such a process moving.

> 
> Cheers,
> 
> Simon.
> 
> _______________________________________________
> OpenAFS-devel mailing list
> OpenAFS-devel@openafs.org
> https://lists.openafs.org/mailman/listinfo/openafs-devel

-- 
Matt Benjamin
The Linux Box
206 South Fifth Ave. Suite 150
Ann Arbor, MI  48104

http://linuxbox.com

tel. 734-761-4689
fax. 734-769-8938
cel. 734-216-5309