We have two swat windows every day. It's magical... I post a request for a deploy on a Wiki page and someone deploys it.

Could we try a similar thing with code review. Code review window (maximum 1 patch per person) and have a group of +2ers look at a maximum set of patches?

It would need a few more rules than that and a bit of tweaking but seems like a good experiment. I'd sign up to help it if it was a maximum 2 windows for me a week. On 16 Mar 2016 12:50 p.m., "Daniel Kinzler" daniel@brightbyte.de wrote:

There will be an RFC meeting tonight about this: https://phabricator.wikimedia.org/E148

One thing that I don't remember coming up during the summit is:

Can we perhaps get all the people with +1 rights to use them and actually review stuff? So that people with +2 rights can look at things that already have a +1, and can thus avoid getting swamped with lots of patches with lots of low grade issues?

Where would this idea fit into the zoo of tickets we have on the subject?

Am 15.03.2016 um 17:11 schrieb Andre Klapper:

...

Hey everybody!

At the Wikimedia Developer Summit there was a session about "Making Code Review not suck" [1]. The outcome are Phabricator (sub)tasks of the task "Define potential actions to reduce code review queues and waiting times" in https://phabricator.wikimedia.org/T101686

The following potential actions items have been identified:

• https://phabricator.wikimedia.org/T129842 : Decide whether to have a "Code Review" committee / working group
• https://phabricator.wikimedia.org/T129067 : Agree on and document a structured, standardized approach for reviewing code contributions
• https://phabricator.wikimedia.org/T129068 : Improve code contribution guidelines for patch authors
• https://phabricator.wikimedia.org/T128370 : Update ownership list on [[mw:Developers/Maintainers]]
• https://phabricator.wikimedia.org/T128371 : Set up Code Review office hours
• https://phabricator.wikimedia.org/T128372 : Document use of Owners in Phabricator and advertise it
• https://phabricator.wikimedia.org/T115852 : Fix unclear maintenance

responsibilities for some parts of MediaWiki core repository

Discussion on each of those proposals is welcomed in the corresponding tasks!

Thanks, andre

[1] https://phabricator.wikimedia.org/T114419

---

Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Changing subject to focus on the code-review office hours idea.

Also, a request at the end for people with +2 to indicate if they would be willing to participate in an experiment or not :)

<quote name="Jon Robson" date="2016-03-16" time="15:47:49 -0700">

We have two swat windows every day. It's magical... I post a request for a deploy on a Wiki page and someone deploys it.

:) glad you like it

Could we try a similar thing with code review. Code review window (maximum 1 patch per person) and have a group of +2ers look at a maximum set of patches?

It would need a few more rules than that and a bit of tweaking but seems like a good experiment. I'd sign up to help it if it was a maximum 2 windows for me a week.

That sounds like an interesting idea indeed, Jon.

An action item from me from the DevSummit was https://phabricator.wikimedia.org/T128371, which is basically "setup code-review office hours" without much more detail than that (it was a drive-by idea during one of the sessions that I volunteered to follow-up with).

My initial idea was very minimal (basically just a time and a virtual place to do code-review together) but adding in the explicit support of +2ers helping merge ready patches during the time gives it more effectiveness.

The hardest part will, I assume, be getting enough people with commit rights to volunteer for at least one day/week.

Any one reading this far with +2 willing to? If so, please comment on the task (to save the mailing list): https://phabricator.wikimedia.org/T128371

Thanks,

Greg

Le 16/03/2016 20:49, Daniel Kinzler a écrit :

There will be an RFC meeting tonight about this: https://phabricator.wikimedia.org/E148

One thing that I don't remember coming up during the summit is:

Can we perhaps get all the people with +1 rights to use them and actually review stuff? So that people with +2 rights can look at things that already have a +1, and can thus avoid getting swamped with lots of patches with lots of low grade issues?

Where would this idea fit into the zoo of tickets we have on the subject?

Hello,

I once proposed to get an additional tier in our code review system, a level that would approve what has been reviewed. I can not find the link though, but I believe I got the inspiration from OpenStack. Their workflow is:

Anyone can Code-Review +1 Core reviewers do Code-Review +2 Maintainers Approve +1 Release manager deal with branches and accepting hotfixes.

They have a convention or a Gerrit enforcement that a changes need two CR+2 before it get approved. Once Approved +1 , the change get merged.

What does it mean for a maintainer? They can craft a query to look for changes that have received a few CR+2, focus on the release quality and almost blindly approve changes in bulk. I am part of such group and it makes it very easy to do the final review and land patches.

Drawback, as a dev you need to seek +2 from a couple core reviewers...

That got shoot down as not being the wiki way. Instead we are much more liberal in giving CR+2 right on the repo, in turn trusting people with such privilege to not CR+2 patches on code they are unfamiliar with. I think it is a good compromise.

If we had some kind of dashboards to list patches that are ready enough, got a fair amount of CR votes, that would probably make life easier for people landing changes. I myself have bookmarked a couple Gerrit queries:

A) for changes on which I am a reviewer and haven't voted yet:

is:open reviewer:self NOT owner:self label:code-review=0,self

That is the first page I hit every morning which list reviews I need to do for others.

B) Changes reviewed with no CR-1/-2 and no verified -1 (Jenkins fail)

is:open reviewer:self NOT owner:self label:code-review=0,self label:code-review=1 NOT label:code-review=-1 NOT label:code-review=-2 NOT label:verified<=0

That is more or less changes I can CR+2 right away.