[pulseaudio-discuss] Rethinking how we do reviews

[Arun Raghavan]

On Sat, 2016-04-02 at 17:19 +0300, Tanu Kaskinen wrote:
> On Thu, 2016-03-31 at 10:55 +0530, Arun Raghavan wrote:
> > 
> > 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.
> That sounds like a plausible theory, although I don't personally feel
> demotivated by the large amount of time it takes for my own patches to
> get merged. If you really think that your review bandwidth increases if
> your own patches go through faster, it seems to me that I could
> prioritize reviewing your patches over others, and that way get more
> reviews from you. What do you think?

I think that a this would help, but I fear that this does not do
enough. My preference is still the relaxed (but still existent) review
process that I outlined before, so that we have /some/ hope of catching
up on the patch and bug backlog over time.

Perhaps we can give it a try what I'd proposed for a couple of release
cycles and evaluate how things stand?

-- Arun