Page MenuHomePhabricator

Decision on Wikimedia Foundation service-level agreement on code review times
Open, LowPublic

Description

I don't find the posting to wikitech-l at the moment, but IIRC the mobile team had a policy to review each contribution to "their" repositories in x days. IMHO that's the way to go, no organization or travelling and cookies needed, but a simple script that alerts "maintainers" (to be defined) every week that they have y patches waiting for review.

Should there be a service-level agreement from Wikimedia Foundation teams to developers contributing patches? If we want to have a SLA, many details need to be defined and homework needs to be done in several steps to get there.

See also: https://lwn.net/Articles/688560/

Related Objects

StatusSubtypeAssignedTask
ResolvedQgil
ResolvedDicortazar
InvalidNone
InvalidNone
ResolvedQgil
Resolved greg
OpenNone
OpenNone
ResolvedQgil
ResolvedAklapper
ResolvedAklapper
ResolvedAklapper
ResolvedDicortazar
ResolvedDicortazar
ResolvedAcs
ResolvedDicortazar
ResolvedDicortazar
ResolvedDicortazar
ResolvedDicortazar
ResolvedDicortazar
ResolvedDicortazar
InvalidDicortazar
ResolvedDicortazar
ResolvedDicortazar
ResolvedAklapper
ResolvedDicortazar
ResolvedDicortazar
DuplicateNone
ResolvedDicortazar
ResolvedBawolff
Resolvedmmodell
ResolvedNone
Resolvedmmodell
ResolvedLegoktm
Resolvedtstarling
Resolved greg
ResolvedAklapper
ResolvedAklapper
ResolvedAklapper
ResolvedAklapper
ResolvedNone
DeclinedNone

Event Timeline

Qgil created this task.Sep 25 2015, 9:41 AM
Qgil raised the priority of this task from to Medium.
Qgil updated the task description. (Show Details)
Qgil added subscribers: Jay8g, mmodell, Tgr and 17 others.
Tgr awarded a token.Sep 25 2015, 5:50 PM
Tgr added a comment.Sep 25 2015, 6:13 PM

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/

Tgr added a comment.Sep 25 2015, 6:15 PM

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.

brion added a subscriber: brion.Sep 25 2015, 7:30 PM
Nemo_bis added a comment.EditedSep 25 2015, 7:39 PM

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

scfc added a comment.Sep 25 2015, 8:11 PM

@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.

scfc added a comment.Sep 26 2015, 5:53 PM

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.

ggellerman moved this task from Team radar to To Triage on the Team-Practices board.

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).

Tgr added a comment.Oct 13 2015, 11:00 PM

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

Aklapper lowered the priority of this task from Medium to Low.Apr 2 2016, 8:21 PM
Qgil added a comment.Apr 26 2016, 7:42 AM

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.

Qgil added a subscriber: Legoktm.EditedApr 26 2016, 8:25 AM

... 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.

scfc added a comment.Jun 2 2016, 1:21 AM

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.

Qgil renamed this task from Wikimedia Foundation service-level agreement on code review times to Decision on Wikimedia Foundation service-level agreement on code review times.Jun 2 2016, 10:14 AM
Qgil updated the task description. (Show Details)
Qgil set Security to None.
Nemo_bis updated the task description. (Show Details)Jun 2 2016, 3:36 PM

@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.

scfc added a subscriber: hashar.EditedJun 2 2016, 4:59 PM

@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

ggellerman removed a subscriber: ggellerman.
Qgil moved this task from To triage to Team radar on the Developer-Advocacy board.Sep 28 2016, 8:17 PM
Qgil added a comment.Jan 17 2017, 8:59 AM

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

hashar removed a subscriber: hashar.Jul 11 2019, 12:35 PM