[pulseaudio-discuss] Rethinking how we do reviews

[Arun Raghavan]

On Wed, 2016-03-30 at 15:43 +0300, Tanu Kaskinen wrote:
> On Mon, 2016-03-28 at 12:43 +0530, Arun Raghavan wrote:
> > 
> > Hello,
> > I'm sure it's not shocking or surprising to state that our patch review
> > bandwidth is significantly lower than the rate at which contributions
> > are coming in.
> > 
> > It is also quite demotivating to send patches to the list and have to
> > wait weeks to hear about them, and kudos to all our patient
> > contributors and everyone who's been pitching in on the review process,
> > especially Tanu who's taken up the bulk of the heavy-lifting on this
> > front.
> > 
> > While having Patchwork in place helps keep track of patches so we don't
> > lose some through the cracks, it does not help decrease our review
> > turnaround time by a whole lot.
> > 
> > To this end, I propose that we ease our review policy a bit. My
> > proposal is that:
> > 
> > * Committers should have the ability to go ahead and push out their own
> > changes without review, except ...
> > 
> > * User-facing changes should have some announcement and/or discussion
> > (changing dependencies, new modules, etc.)
> > 
> > * Changes to API or protocol should undergo review at least to the
> > extent of the API/protocol change
> > 
> > * Large infrastructure changes should go through a full review
> > (slightly subjective, but I think we can leave this to individual
> > judgment)
> > 
> > Our current way of doing things is good for keeping up code quality,
> > but I think over time, with such a large patch backlog, we end up
> > spending more and more time performing reviews, and less and less time
> > working on features. This becomes quite draining and drops our overall
> > productivity in contributing to the project.
> > 
> > At this point, I guess this is mostly for Tanu to buy into, and maybe
> > David if he'd like to continue contributing at least on the ALSA side.
> > Thoughts and suggestions from others are still welcome, of course.
> The point of reviewing is to save time: it's more efficient to catch
> bugs in review than debug them in production, and making the code as
> easy to understand as possible saves time later when reading the code.
> If this assumption of saving time is correct, isn't it then misguided
> to skip reviews so that you could spend more time on writing new
> features? Either the uncatched bugs start to pile up, or you won't have
> time for writing new features anyway, since you'll be fixing bugs.

We have two opposing axes to optimise along. The first is code quality,
as you point out. The other is developer motivation.

My feeling is that being able to work on things and push them out
without a long wait would help us feel more productive and motivated to
work on all aspects of the project.

With the current system, I think we're spending more and more time
reviewing things and waiting for reviews.

Having the ability to work on things and push them out provides a much
shorter path between "this is something I'd like to do" and "this thing
is done". This allows for a positive feedback loop, and I think that
will bolster our motivation to perform reviews, bug triaging, and
everything else.

> Being less picky about patches, as David suggested, can help make the
> review process more efficient, and I have already started to ignore
> some small coding style issues. Also, if a patch contains only trivial
> problems, I prefer to fix it up myself instead of requiring a new
> submission. If you see me complaining about something that isn't worth
> complaining about, I welcome feedback about that. That said, I'm a bit
> sceptical about the trivial coding style stuff contributing a lot to
> the review times.
> 
> The best solution would be to get more reviewers, but I don't really
> have good proposals for achieveing that (I'd be ready to invite
> Alexander to be a maintainer, though, but I'm not sure he has much time
> anyway). Another question is that what is limiting the current
> maintainers' capacity? Is it other commitments and the fixed number of
> hours in a day, or is more about motivation? For myself, it's certainly
> motivation. In theory I nowadays have all the time in the world to do
> reviews, but in practice I have very limited energy for it. I've been
> experimenting with different approaches, trying to optimize my output.
> Planning to spend many hours at a time doesn't seem to work. Currently
> my approach is to do one review each day, and I've been able to keep up
> with that quite well; maybe I'll try two patches per day at some point.
> This "slowly but surely" approach certainly makes big patch sets less
> intimidating.

I think it is motivation, and I think the slow and steady approach is
indeed commendable (certainly better than my bursty patch review
method), but it doesn't address the problem of motivation, and I think
it iss worth trying to see if my proposal does.

-- Arun