Re: [Ksummit-discuss] [TOPIC] Encouraging more reviewers

[Trond Myklebust <trond.myklebust@primarydata.com>]

On Sat, May 24, 2014 at 5:53 AM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> A lot of the problems accepting patches into the kernel stem from trying
> to get them reviewed ... some patch series even go for months without
> being reviewed.  This is caused by a couple of problems.  The first one
> is a submission issue, where you get a long N patch series (usually N >
> 10) you review and provide feedback, then you get a long N+Y series back
> eventually with no indication what disposition was taken on any of the
> review points ... even the most hardy reviewers get a bit demotivated at
> this point. The second is an actual lack of reviewers in particular
> fields, meaning that a small number of people tend to be the ones
> reviewing patches in particular subsystems.
>
> The former is probably just an addition to SubmittingPatches explaining
> how to resend after addressing review comments.  The latter was supposed
> to be helped by having the Reviewed-by: tag so we gave credit to
> reviewers.  I've found the Reviewed-by tag to be a bit of a double edged
> sword: it is a good way of giving review credits, but I also see patches
> that come in initially with it on (usually the signoff and the
> reviewed-by are from people in the same company) ... it's not
> necessarily a bad thing, but it doesn't add much value to the kernel
> review process, because we're looking for independent reviews.  The
> other thing I find problematic is that some people respond to a patch
> with a Reviewed-by: tag and nothing more.  I'm really looking for
> evidence of actually having read (and understood) the patch, so the best
> review usually comes with a sequence of comments, questions and minor
> nits and a reviewed-by at the end.
>
> However, the fundamental problem is we still need to increase our review
> pool, so we need more than the Reviewed-by: tag.  It has been suggested
> that people who submit patches should also be required to review patches
> from other people ... I think that's a bit drastic, but it is a
> possibility. Another possibility for the kernel summit might be to have
> review time instead of hack time: we each nominate a patch set needing
> review and it gets reviewed by others in that time ... if that one's
> successful, we could extend the concept to other conferences and
> development sprints which do hack time.  Another possibility is to
> publish a subsystem to review list (similar to the to do list idea).
> This at least shows submitters why their patch series is languishing and
> could serve as a call to arms if it gets too long.


Hi James,

You are assuming that organising reviews of patches is the
responsibility of the maintainer.
In my opinion, it should rather be the responsibility of the submitter
to do so as part of the process of rallying support for what they are
proposing. It is fine for the maintainer to assist that process, to
set parameters around it (e.g. how many reviews are required before it
can be merged), and to act as one of the reviewers, but there is no
reason why the maintainer must be the one to drive the process.

I agree that ensuring quality of review is difficult. Part of the
problem there is that our toolchain has nothing to capture reviews,
and to preserve them for posterity other than the mailing lists. The
best you can do then is to use 'Link:' in order to tag the review
thread in the commit log. It would be nice to have a better solution.
Patchworks is one option, but it doesn't really work well for capture
the full process if the submitter pushes out a new patchset in
response to a review (you end up starting afresh).
Bugzillas are another option, but their main task isn't really to
preserve patch reviews, and furthermore, they are less well adapted to
our working habits (which has lead to pushback from a number of
maintainers).
Other options?

Cheers
  Trond
-- 
Trond Myklebust

Linux NFS client maintainer, PrimaryData

trond.myklebust@primarydata.com