[pulseaudio-discuss] Rethinking how we do reviews

[Arun Raghavan]

On Wed, 2016-04-06 at 16:25 +0200, David Henningsson wrote:
> 
> On 2016-04-06 10:25, Arun Raghavan wrote:
> > 
> > On Tue, 2016-04-05 at 19:28 +0300, Tanu Kaskinen wrote:
> > > 
> > > On Mon, 2016-04-04 at 13:35 +0530, Arun Raghavan wrote:
> > > > 
> > > > On 4 April 2016 at 13:01, David Henningsson <diwic at ubuntu.com> wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > On 2016-04-04 04:51, Arun Raghavan wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > On Sat, 2016-04-02 at 17:19 +0300, Tanu Kaskinen wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > On Thu, 2016-03-31 at 10:55 +0530, Arun Raghavan wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 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?
> > > > > > 
> > > > > > 
> > > > > For me, the demotivator has been when we disagree on things, if someone
> > > > > wants my patch written in another way: I can then choose either to rewrite
> > > > > my code to someone else's liking even though I like it better the way it is
> > > > > (which is demotivating), or I can choose to respond back which can lead to
> > > > > long discussions, which is in itself demotivating and time consuming.
> > > > > I don't know if there exists a good solution to that problem, but I've been
> > > > > thinking along the lines of having areas of responsibility, where one is
> > > > > effectively dictator for some pieces of the code, and someone else is
> > > > > dictator for other pieces, etc.
> > > > I think there are ways to short-circuit long discussions, such as
> > > > setting aside some time for real-time discussion on IRC.
> > > > 
> > > > And perhaps it does make sense to take on more autonomy for certain
> > > > areas if that helps us work better. Note that my review proposal does
> > > > overlap with your suggestion in that non-external-facing changes can
> > > > be done more autonomously.
> > > I'm not sure David's proposal overlaps with your proposal. David didn't
> > > explicitly say whether these dictators themselves should still have
> > > their own patches in their own area reviewed. I understood David's
> > > proposal only as a tool to make it easier to decide things when there
> > > are disagreements, so it wouldn't imply a right to skip reviews.
> > You're right, and it's quite possible I interpreted David's suggestion
> > differently from what he meant.
> It's not a finished thought, but maybe the amount of review dictators 
> need for their own area is something like: simple patches can go 
> straight in, medium patches can have what we have today for simple 
> patches (i e wait a week, if there is no reviews then pushing can be 
> okay), and difficult world-changing patches should have a pair of extra 
> eyes.
> 
> And what patches are "simple", "medium" and "difficult" would be a 
> learning / gut feeling thing.
> 
> > 
> > 
> > > 
> > > I quite like the general idea of having a dictator to resolve
> > > disagreements, but it's not obvious how the code base could best be
> > > divided. I don't myself have any particular desire to be the dictator
> > > for any part of the code, but I don't have a problem with taking that
> > > role either if it's given to me.
> > I guess the idea isn't different from subsystem maintainers in Linux,
> > but I'm not sure we're a large enough project to be able to divide up
> > responsibilities quite like that. Maybe there's a way to do it in a way
> > that makes sense, though.
> > 
> > > 
> > > > 
> > > > > 
> > > > > As for Tanu's suggestion (to prioritise Arun and see if that helps) vs
> > > > > Arun's (every committer is free to push their own stuff), I prefer
> > > > > evaluating Tanu's, as I see a risk with Arun's proposal that committers will
> > > > > spend more time on their own stuff rather than reviewing, if the former is
> > > > > what gives that positive feedback loop.
> > > > To me, the latter gives a stronger positive feedback loop than the
> > > > former, because you're still waiting for a few days at least for each
> > > > patch set.
> > > Does it really demotivate you if your patches take a few days to get
> > > reviewed? I can understand that it's frustrating if reviews take weeks
> > > or even months, but I have trouble imagining a few days of review delay
> > > being a problem.
> > It does take away from the development flow, because it becomes hard to
> > continue working on a thing until there's a review (because otherwise
> > you're spending significant effort on rebasing). So instead of focusing
> > on a problem and completing it to your satisfaction, there's a waiting
> > period where you do other things, and when things are finally reviewed,
> > you've lost some amount of context and motivation.
> If reviews are done within a week, then rebasing is not much of a 
> problem. Combine this with being less picky about patches and you get a 
> workflow that hopefully means that the local branch you worked on can be 
> simply be merged to master. Hence you can get some of the "this is done" 
> feeling when coding is done already.
> 
> > 
> > 
> > > 
> > > You didn't directly address my earlier point of reviews saving time. I
> > > hesitate to accept your proposal because I believe that reviews save
> > > time overall (our time and our users' time), since there are fewer bugs
> > > to investigate and fix later. Do you believe differently?
> > I thought I did, but to be explicit -- I have a different perspective
> > on this. I think we're balancing code quality vs. developer motivation,
> > and with the current resources that we have, the negative impact on
> > developer motivation is higher than the positive impact on code
> > quality.
> > 
> >  From a broader perspective, therefore, I think our current process is
> > harming our ability to sustain the project.
> > 
> > I know you said you don't feel that much of a loss of motivation from
> > the waiting period, but I think that being able to go from "I want to
> > do this" to "this is done" much more quickly than now would actually
> > have a positive impact.
> OTOH; the demotivation by suddenly discovering that your code / use case 
> broke because someone (who was not considering your particular use case) 
> pushed a patch is diminished if there are reviews. Hence the one week 
> "notice period".
> > 
> > > 
> > > > 
> > > > Committers spending more time on their own stuff is something that can
> > > > just as well happen now
> > > It doesn't make sense to spend more time on your own stuff, though, if
> > > you produce patches faster than they get reviewed.
> > Sure, but this is also part of the problem. Even if you want to spend
> > more time on your own stuff, you end up not doing so because you're
> > waiting for your existing work to be completed.
> And in the mean time, you can review some patches from other people? :-)

I guess what I'm trying (ineffectively) to say is that these two are
only not exactly related the way you're implying here.

If you have a total T time for PulseAudio of which t1 is doing your own
thing, and t2 is reviews, bug triage, etc. -- if t1 feels unproductive,
t2 (and t1, and thus T) will decrease. Conversely, being productive
with t1 increases the motivation to increase both t1 and t2 (and T).

-- Arun