Sumana shared an interesting blog post some time ago on how to do code review in a way that reduces time spent on it (by both parties): http://sarah.thesharps.us/2014/09/01/the-gentle-art-of-patch-review/

For core (and possibly other large projects), sending non-realtime notifications to maintainers is probably blocked by T63463.

Phabricator made things worse for those who cared because it lacks basic functionality, see T76825.

This task is about code review times, not about sending reminders about some open tasks.

I dispute the difference (or rather, the dichotomy).

@Nemo_bis: This would (inter alia) require that there were no commits to be reviewed that did not have an associated Phabricator task, which you know to be false.

Perfection is the enemy of good. Improving the ability of people to get regular reports on things they are supposed to do (reaching at least pre-phabricator levels) is a good thing even without 100% coverage.

Nobody (except perhaps @Qgil) has disputed that solving T76825 would be a good thing. But you didn't claim that it was a good thing, but that it was a blocker for a SLA for code review, and that is not the case.

Just dropping some thoughts from yesterday evening here which will likely require dedicated subtasks:

An SLA would require a clear definition which WMF teams are responsible for which exact code areas.
For example, MediaWiki core has areas which are rather clearly volunteer-maintained (e.g. PostgreSQL and MSSQL database backends?) plus areas that noone (WMF or not) might feel responsible for.
Likely a similar case are "old" extensions deployed on WMF servers (mw.org category; CommonSettings.php; InitialiseSettings.php) where some WMF team might fix breakage but be less committed to reviewing changesets that implement new functionality.

Maintainer/team↔codebase linkage: By implication via watching projects in Gerrit on an individual level; also a small number of Phabricator team projects document their responsibilities (examples: Multimedia; Collaboration) while most don't so that linkage happens non-visibly (e.g. teams using custom Phab dashboards). Documentation-only there is mw:Developers/Maintainers and extension homepages on mediawiki.org which list authors but not necessarily maintainers (whether teams or individuals).

There is a proposed WikiDev16 session about improving code review, especially for volunteers: T114419: Event on "Make code review not suck". You are welcome to comment there if interested.

In T114419#1933847 ("Make code review not suck"), @Qgil wrote:

If I recall correctly, @greg, @ori, and... ? talked about the creation of a working group to analyze this problem and bring recommendations for the (WMF?) development teams. Is this correct?

@Aklapper can help with research, proposals, and probably some wrangling, but we will need more muscle to push actual changes in routines within the WMF teams. Also, I wonder whether T113707: Decision on Wikimedia Foundation service-level agreement on code review times is considered as a feasible, desirable goal.

@Qgil, what do you mean by "more muscle"? I believe we need to establish better trust-building mechanisms to establish shared responsibility and learning before instituting SLAs. Hence: T123753: Establish retrospective reports for #security and #performance incidents

Maybe "more muscle" was not the right term. At this point I believe what we need is a clear owner of the problem of code review waiting times. A person empowered to drive the solution of this problem. Someone that , for instance, could drive the discussion and the decision on whether a Service Level Agreement is what we're after or not.

I'm glad to see @mmodell pushing T129842: Launch the Wikimedia Code Review Working Group. On the other hand, T115852: Fix unclear maintenance responsibilities for some parts of MediaWiki core repository keeps not having an owner, and we keep suffering the problem of lack of responsibilities, even for owning that task.

... although just now I realize that T128370: Update ownership list on [[mw:Developers/Maintainers]] does have @Legoktm as owner. Good. :)

As Release-Engineering-Team is taking the initiative on code review process improvements, I wonder what do you think about this idea.

Rather than define a service agreement that likely won't be upheld, I think that the current initiatives like E179: Code Review Office Hours and Differential offer more tangible benefits in the short term.

When we manage to get owners for every repository and reasonable backlogs, then maybe a service level commitment would be more feasible / realistic.

Has E179 or Differential yielded "more tangile benefits"? The graph at https://korma.wmflabs.org/browser/scr.html does not show a longer term downward trend. (And a system where code reviews have to be done synchronously and comments may be posted out-of-band does not appear to be more productive per se.)

But I agree that "service-level agreement" is way too formal, and not what I proposed at T113378#1670621. I just wanted to point out then that the Mobile team apparently had some policy regarding code reviews, and they seemed to stick with it. This doesn't mean that all files in all repositories need to have an "owner" before someone can start to feel responsible for a single one of them. We use a similar strategy for tests: Even though not all code paths are tested, we use them wherever possible so that regressions there are noted early and loud.

@scfc:

I suspect that reviewing patches for mobile is a lot different than reviewing mediawiki core and extensions. With mediawiki code review, there are a lot of dependencies and unknowns, plus competing interests to consider. My perception is that mediawiki code review is a lot more complicated than mobile app code review.

Although a policy might be helpful, I don't really know if what works for one team will necessarily work as well for the whole organization, with WMF teams and outside contributors included.

MobileFrontend *is* a MediaWiki extension.

@mmodell: I don't know if MediaWiki core and extensions have different requirements for code review, but that would still leave a lot of repositories where such an "SAL" could be defined now (and as @Nemo_bis pointed out, a lot of extensions are probably easier to review than core as well).

The problem with code review is visibility: For example when you submit a change in a repository that @hashar watches or you add him as a reviewer to the change, he will give you prompt feedback (he even posted the links he uses (daily?) as dashboards on wikitech-l). In other repositories, changes can rot until they show up in "Oldest open Gerrit changesets without code review". So (IMHO) we need to find a way to make other developers as responsive as @hashar without having his self-discipline of daily checking his to-do list, for example by a daily/weekly mail reminder of open changes waiting for a review.

General request: Please discuss general code review process aspects in T78768 (also see T78768#2186465 for a summary).
This task here is specifically about the aspect of having an SLA. Thanks for keeping discussion focused! :)

Removing Team Practices Group for now. If you need something specific from the TPG, we suggest you consider using a template along the lines of the example requests here:

https://www.mediawiki.org/wiki/Team_Practices_Group/How_To_Engage_With_TPG

Language team has https://www.mediawiki.org/wiki/Wikimedia_Language_engineering/Code_review_statement_of_intent to help set expectations for us and patch submitters.

At this point, I would either push this proposal to Developer-Wishlist (2017) to check what is the interest or decline the task.