Experimenting with a shared review queue for Core::Build Config

[Chris Cooper]

Many of the build peers have long review queues. I'm not convinced
that all of the review requests going to any particular build peer
need to be exclusive. We're going to try an experiment to see if we
can make this better for patch authors and reviewers alike. To this
end, we've set up a shared review queue for patches submitted to the
Core::Build Config module.

How to participate:

When you submit your next Build Config patch, simply select the new,
shared "user" core-build-c...@mozilla.bugs as the reviewer
instead of a specific build peer. The build peers are watching this
new user, and will triage and review your patch accordingly.

This new arrangement should hopefully shorten patch queues for
specific reviewers and improve turnaround times for everybody. It also
has the added benefit of automatically working around absences,
vacations, and departures of build peers.

Note: this system still allows for targeting reviews to specific build
peers. Indeed, the build peers may do exactly that in triage if the
patch in question touches a particular sub-domain. However, I would
encourage patch authors to use the shared bucket unless you really
understand the sub-domain yourself or are collaborating directly with
a particular build peer.

As indicated in the subject, this is an experiment. I will monitor
patch queues and turnaround time over the next few months, and then
decide in January whether we should continue or try something else.

Thanks for your patience, and for trying something new.

cheers,
--
coop

[Nathan Froyd]

Does this user have a bugzilla :alias so that folks submitting patches
via MozReview or similar can just write r=build-peer or something,
rather than having to manually select the appropriate shared queue
after submitting their patch for review?

-Nathan

> _______________________________________________
> dev-builds mailing list
> dev-b...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-builds

[Chris Cooper]

On Wed, Oct 11, 2017 at 1:46 PM, Nathan Froyd <nfr...@mozilla.com> wrote:
> Does this user have a bugzilla :alias so that folks submitting patches
> via MozReview or similar can just write r=build-peer or something,
> rather than having to manually select the appropriate shared queue
> after submitting their patch for review?

I see this as an added efficiency, so I'll see how much effort it is
to set this up. The people behind MozReview are explicitly not
expending effort there right now in favor of phabricator though.

cheers,
--
coop

[Botond Ballo]

It does not require any setup on the MozReview side - you just need to
add ":build-peer" (or whatever the chosen alias) to the "Real name"
field of the user's Bugzilla profile, and MozReview will autodetect
it.

Cheers,
Botond

[Andreas Tolfsen]

+tools-marionette

Also sprach Chris Cooper:

> Many of the build peers have long review queues. I'm not convinced
> that all of the review requests going to any particular build peer
> need to be exclusive.

I think it is great that you’re experimenting with this, and I’m
very excited about it!

In addition to peers doing review triage to balance out review
queues, there’s an argument to be made that for a certain category
of work we do, who reviews the code is of secondary importance.

Speaking from my own personal experience, the vast majority of
patches I write could feasibly be reviewed by any one of the peers
of Testing :: Marionette. There is certainly the odd occasion
when I need to explicitly draw from the expertise of a particular
individual, but I have found this is more the exception than the
norm in the line of work I do on the remote protocol.

In a small team it can also be tricky to keep track of who is
around on any given day across multiple continents and timezones.
A first-come-first-served review system I think would also help
smaller teams with not-so-big review queues improve turnaround times
for patches.

Improving the time from patch written, through review, to
integration in mozilla-central, is doubly important when we receive
patches from new contributors who don’t know who to flag.

> When you submit your next Build Config patch, simply select the
> new, shared "user" core-build-c...@mozilla.bugs as the
> reviewer instead of a specific build peer. The build peers are
> watching this new user, and will triage and review your patch
> accordingly.

I would love to see the modules I’m peer of participate in the
same experiment. Can I ask you to elaborate what a ‘user’ is in
this context and how practically this ‘triage’ happens?

> This new arrangement should hopefully shorten patch queues for
> specific reviewers and improve turnaround times for everybody. It
> also has the added benefit of automatically working around
> absences, vacations, and departures of build peers.

When I worked for Opera we had a similar system where a pool of
reviewers and watchers would get notified about incoming reviews.
In the review tool you would save a glob-filter stating that you
were interested in either reviewing or “watching”, meaning you
only cared about the notification, a change in a certain subsection
of the repository.

More importantly, new contributors to the codebase didn’t have
to know who to flag for review. They’d submit the patch and the
review tool would figure out who to notify. The first respondent
reviewer would then, similarly to how you describe, triage the
change to the most appropriate person, or if it was a simple change,
simply do the review him- or herself.

In the danger of derailing this thread to talk about the new review
tool that is meant to replace Splinter and MozReview, I would be
absolutely thrilled if it had support for “pooled review queues”
like this. If there was a way to annotate directories with OWNER
files or similar it would be even better.

>From what I understand—and please feel free to correct me—the
“shared user” you talk about is simply another Bugzilla account?
That is great for experimentation, but it it turns out a success,
having better ergonomics would be desirable.

[smaug]

On 10/11/2017 09:55 PM, Andreas Tolfsen wrote:
> +tools-marionette
>
> Also sprach Chris Cooper:
>
>> Many of the build peers have long review queues.

Is having a long review queue the actual issue? Isn't (too) high throughput
at least equally bad issue. Does the new setup somehow try to ensure
reviews actually do split more evenly among devs? (and that has nothing to do with
review queue lengths)

How did the setup actually work?
I've asked this from farre too before, and IIRC, the reply was that is wasn't
working that well. Certain devs still ended up doing majority of the reviews.
But perhaps I misremember or certainly I don't know the details.

I'm all for trying new setups to split review load more evenly and
I'm very eager to hear how this experimentation works out!


-Olli

[Andreas Tolfsen]

Also sprach smaug:

> How did the setup actually work?

I think farre gave a satisfactory summary of the review tool above.

> I've asked this from farre too before, and IIRC, the reply was
> that is wasn't working that well. Certain devs still ended up
> doing majority of the reviews. But perhaps I misremember or
> certainly I don't know the details.

Shared review queues isn’t a silver bullet balancing reviews
between peers for a couple of different reasons, but it arguably
makes it easier for peers to collaborate on (and be informed of)
reviews.

For certain parts of the codebase, it is a sad fact the bus factor
is low and we shouldn’t be fooled that a system which practically
allows any one of one’s peers to pick up the review, will somehow
magically improve the turnaround time for patches in those areas.
You will still have reviews that can practically only be reviewed by
one single person who is the domain expert.

On the flipside, when you have a patch for a piece of code multiple
peers know well and feel comfortable accepting, turnaround time
could be improved compared to the status quo where the single
r?<person> could be travelling, on PTO, or otherwise preoccupied.

[Nicholas Alexander]

On Fri, Oct 13, 2017 at 7:47 AM, Andreas Tolfsen <a...@sny.no> wrote:

> Also sprach smaug:

>
> How did the setup actually work?
>>
>

> I think farre gave a satisfactory summary of the review tool above.
>

> I've asked this from farre too before, and IIRC, the reply was
>> that is wasn't working that well. Certain devs still ended up
>> doing majority of the reviews. But perhaps I misremember or
>> certainly I don't know the details.
>>
>

> Shared review queues isn’t a silver bullet balancing reviews
> between peers for a couple of different reasons, but it arguably
> makes it easier for peers to collaborate on (and be informed of)
> reviews.
>
> For certain parts of the codebase, it is a sad fact the bus factor
> is low and we shouldn’t be fooled that a system which practically
> allows any one of one’s peers to pick up the review, will somehow
> magically improve the turnaround time for patches in those areas.
> You will still have reviews that can practically only be reviewed by
> one single person who is the domain expert.
>
> On the flipside, when you have a patch for a piece of code multiple
> peers know well and feel comfortable accepting, turnaround time
> could be improved compared to the status quo where the single
> r?<person> could be travelling, on PTO, or otherwise preoccupied.

This last point is what I think matters. As a build peer (for a subset of
the build system), I know exactly who has the expertise to review my build
system patches -- and I also know which of my patches don't require deep
expertise and can be reviewed by any build peer. I manually balance my
requests to keep this chaff out of the busy build peers' queues. I'm
hoping the shared queue can let the set of build peers opt in to this
balancing, keeping the simple patches out of the busy build peers' queues.

Nick

[Mark Banner]

In case anyone hits this, you need to add "r?Build" for mozreview to
detect the reviewer properly.

Mark.