@mmodell: Got any thoughts / impressions / "lessons learned" to share, as you were one of the folks who helped trying this attempt, IIRC?
(or got anyone else in mind involved?)

@Aklapper: I think it really just needs more publicity to encourage participation. Someone will usually respond to requests for review either in IRC or via comments on the phabricator calendar entries. That is to say code review does take place but the recurring time slot is not always utilized.

IMHO:

• I think #wikimedia-codereview shouldn't exist. This was already discussed on T128371 right after that channel was created and of course it "feels right" to have a dedicated place for something as "important as code review", but the effect basically is that fewer people take notice of these events. As I write this, 27 accounts are idle in that channel. #wikimedia-tech has 237. Move there, that automatically increases the number of people aware of the event - at least on the side of people being able to review and CR+2, as much more of them seem to actively look into #wikimedia-tech regularly.
• The Technical Advice IRC meeting sends mails to wikitech-l (labs-l? mediawiki-l?) once a week (e.g. [2]). In addition, they announce these events on #mediawiki a few minutes before they start. Maybe CR hours should do the same.
• When I asked for review in that channel several times around Feb/Mar this year, the only one noticing or able/willing to review was @mmodell and when looking into the logs it seems this hasn't changed. Tackle T129842 or find some other way to get a group of people in for actively looking for requests and reviewing changes (just like there's a group of people who are "swat deployers" and there's always somebody saying "I can swat today").

[1] https://www.mediawiki.org/wiki/Technical_Advice_IRC_Meeting
[2] https://lists.wikimedia.org/pipermail/wikitech-l/2017-September/088715.html

Honestly I feel that Wikimedia's code-review culture is a complete failure and needs significant overhaul.

@EddieGP: I agree that the IRC channel isn't the best way to get noticed by a lot of people, however, the low volume of messages has an advantage. If you paste a link into that channel it's much more likely to get seen by someone instead of being drowned out by wikibugs notifications or other unrelated discussion. So, while it's not regularly monitored by many developers, at least I check there often and regularly do code reviews when someone requests them.

Announcing the code review hours regularly could help, however, I would really like to have ideas on ways to reach newcomers who have just submitted their first patch to gerrit and don't know who to ask for review.

In T173770#3628207, @mmodell wrote:

Honestly I feel that Wikimedia's code-review culture is a complete failure and needs significant overhaul.

I've submitted my first patch in February and I've felt that the system in place isn't perfect really quickly. That impression continuously increased and my motivation for further contributions, especially to mw core, continuously shrunk. As of today, I can only say that I completely agree with that statement and code review is the most frustrating part of wikimedia-land for me. And judging from the amounts of tasks, mailing lists threads, events, ... on this topic, there seem to be quite a few people who feel (or at least felt) that way.

@EddieGP: I agree that the IRC channel isn't the best way to get noticed by a lot of people, however, the low volume of messages has an advantage. If you paste a link into that channel it's much more likely to get seen by someone instead of being drowned out by wikibugs notifications or other unrelated discussion. So, while it's not regularly monitored by many developers, at least I check there often and regularly do code reviews when someone requests them.

I suggested #wikimedia-tech especially because it doesn't have wikibugs and is rather low-traffic (not like #mediawiki or #wikimedia-dev), but still regularly active (unlike #wikimedia-codereview).

Announcing the code review hours regularly could help, however, I would really like to have ideas on ways to reach newcomers who have just submitted their first patch to gerrit and don't know who to ask for review.

I think that the key point has already been pointed out often enough: For each patchset there should be somebody who (personally) feels and (externally) is responsible for reviewing that patch within timeframe X (whatever X might be, it should at least not be bigger than one week). I've got the impression that discussing the details about "who is responsible for which repository, which part of core, which (insert another way to distinguish patches here)" often lead to not solving the main problem "there shouldn't be any patch nobody feels responsible for".

In T173770#3628669, @EddieGP wrote:

Announcing the code review hours regularly could help, however, I would really like to have ideas on ways to reach newcomers who have just submitted their first patch to gerrit and don't know who to ask for review.

I think that the key point has already been pointed out often enough: For each patchset there should be somebody who (personally) feels and (externally) is responsible for reviewing that patch within timeframe X (whatever X might be, it should at least not be bigger than one week). I've got the impression that discussing the details about "who is responsible for which repository, which part of core, which (insert another way to distinguish patches here)" often lead to not solving the main problem "there shouldn't be any patch nobody feels responsible for".

These are good points. I think that there is a definite lack of responsibility for, not to mention visibility of patches submitted. 1 week seems pretty ambitious considering we currently have patches sitting for > 6 months, however, I agree that 1 week seems like a reasonable expectation for contributors. I can certainly imagine that, as a new contributor, I might start to feel discouraged after 1 weeks' time.

I wonder whether we could have a saved query that would show "Open changesets submitted by volunteers waiting a first code review, sorted by most recent". And I wonder whether this weekly hour could be useful for, in a worst case scenario, go through all these changesets and at least CC/ping the maintainers that could review them and might have overlooked them.

As a start. I am sure that this process can be improved with practice.

@Qgil: That's an excellent suggestion. I don't know about how to make such a query but it does seem like a useful thing to do.

If we had a gerrit group for new users we could do a search for ownerin:'GROUP' but other than that I can't see any way to identify new users in gerrit.

1 week seems pretty ambitious considering we currently have patches sitting for > 6 months, however, I agree that 1 week seems like a reasonable expectation for contributors. I can certainly imagine that, as a new contributor, I might start to feel discouraged after 1 weeks' time.

It depends on the definition of "code review". I liked an article we already linked to in various places [0] splitting it into three phases, the first of which is "Do we want this patch at all?":

The first phase of the contribution review should only require a simple yes or no answer from the maintainer: “Is this contribution a good idea?” If the contribution isn’t useful or it’s a bad idea, it isn’t worth reviewing further.

Sure it's ambitious to have all our patches reviewed within one week (coming from more than six months) when we think about review in terms of "the maintainer carefully reads the code, tests it to check whether it works and doesn't break something else and either merges it or gives a detailed description what has to be changed". This is something that takes time and that usually should be done by someone familar with the code the patch touches (that is, the maintainer).

However, we could think about it as an first response, meaning

• if you aren't the maintainer: "Hey, thanks for your contribution, unfortunately I don't know this code base and thus can't review this patch, but I cc'd the maintainer so they'll look into it."
• if you are a maintainer, something like "Hey, I like the idea of this patch, but I can't go through it in detail right now - remind me when I haven't done it in 3 weeks."

Note the "3 weeks" in there: It'd be perfectly fine (to me) to have that timeframe be something larger than a week (it probably should still be less than a month). The important part is that the uploader now knows that his patch isn't unwanted (an impression which is easy to create) and has a hint until when code review can be expected. "Waiting" just feels completely different when you know someone cares about your contribution and plans to look into it within a certain time frame as opposed to not getting any comment at all (which leads to the impression nobody gives a ...) because the maintainer just thinks but not writes "not now, will look into it later".

That's why I said X shouldn't be larger than a week: For such a first response (NOT a thorough review), I'd actually expect X = 48h or even X = 24h (given a big part of the reviewers are WMF employess one might add "on weekdays") . Again, the article sums it up nicely (which likely means this isn't news to you):

If the contribution is worthwhile, but you don’t have time to go onto the second phase of patch review, do NOT say nothing. Instead, drop the contributor an email that says, “Thanks for this contribution! I like the concept of this patch, but I don’t have time to thoroughly review it right now. Ping me if I haven’t reviewed it in a week. [...] It also gives you incentive to actually move onto phase two, because the contributor will bug you again if you haven’t reviewed the contribution.

I want to add that this article is linked on mw:Gerrit/Code_review and I've seen links to it in various old mailing list threads on the code review topic, so it seems to be well known. Yet, I don't remember to actually have seen a single person commenting something as simple as "No time right now, bug me again next week" in gerrit. How much time does that take, ten seconds? I for one haven't seen it a single time.

In T173770#3637193, @Qgil wrote:

I wonder whether we could have a saved query that would show "Open changesets submitted by volunteers waiting a first code review, sorted by most recent".

AFAIK it's hard to filter gerrit for volunteers, especially as quite a few employees don't use @wikimedia.(org|de) mail adresses. Not saying it shouldn't be tried though.

And I wonder whether this weekly hour could be useful for, in a worst case scenario, go through all these changesets and at least CC/ping the maintainers that could review them and might have overlooked them.

Yes! It'd definitely help to bring attention to these patches and decrease the backlog. Even if no or almost no patches really get reviewed in that hour, it'd still be worth it as a good step towards solving the "nobody responsible" problem.

When Mukunda says "newcomers who have just submitted their first patch to gerrit" and Quim mentions patches "waiting a first code review" (just as examples, seen phrases like that on various mails/tasks) I wonder whether the focus is too much on really really new contributors. I sometimes get the impression that we don't do a too bad job in identifying first-time patch-submitters and reviewing that patch. Instead those new contributors may be hardly finding somebody to review their second or third patch.
In the same way many patches actually find someone to +1 or -1 the first patchset and give a thorough review within an acceptable time. However, after a second patchset is uploaded, trying to solve problems the first patchset had, the patch starts to rot and there is no feedback on the second patchset for month (been there). For somebody actually trying to get involved with wikimedia-tech (as opposed to "I want to fix this bug that annoys me on my wiki"), the "first impression" does not equal "the first patchset of my first patch", but it's more like "for each of my first 3 patches, the first 5 patchsets". While the very first part of the very first impression is important, I sometimes feel we actually loose people over the rest of the "first impression".

[0] The gentle art of patch review, http://sarah.thesharps.us/2014/09/01/the-gentle-art-of-patch-review/

In T173770#3637538, @mmodell wrote:

If we had a gerrit group for new users we could do a search for ownerin:'GROUP' but other than that I can't see any way to identify new users in gerrit.

As long as we still need a whitelist in CI (which is a pain for newcomers by itself, we should get rid of it rather sooner than later) we could match newcomers as "people having submitted so few patches they weren't added to the whitelist already" by searching for Verified=+1. AFAIK the only situation jenkins issues a +1 is for newcomers, in every other case it'd issue an -1 or +2. However, this does treats patchsets with failing CI tests as false negatives. Including -1 will have a lot of false positives on the other hand. I've just tried status:open NOT ownerin:ldap/wmf NOT ownerin:ldap/wmde label:Verified=+1 NOT label:Code-Review=-2.

Here is a query for starters. Open changesets submitted by volunteers sorted by most recent: https://wikimedia.biterg.io/goto/f1ef9781ad4ed9ec90650323ccb2f25a

Perhaps it is better to filter out changesets younger than one week, not to mess with those that have just landed and will be taken care in the next hours / days most likely.

In any case, this proposal doesn't aim to be perfect. It aims to be a tangible starting point to break the current no-hours, no-content status.

/me forces himself to not repeat things...

In T173770#3637193, @Qgil wrote:

I wonder whether we could have a saved query that would show "Open changesets submitted by volunteers waiting a first code review, sorted by most recent". And I wonder whether this weekly hour could be useful for, in a worst case scenario, go through all these changesets and at least CC/ping the maintainers that could review them and might have overlooked them.

Related but not what you're looking for:

• MZ's (?) Gerrit stats (which do not sort by most recent but alphabetically show anyone with less than 5 patches)
• the "New Authors" list widget on wikimedia.biterg.io (which does not offer links to the changesets in Gerrit but author names). Note: Data not updated automatically.

I used the latter (on defunct korma.wmflabs.org, which got superseded by wikimedia.biterg.io) in 2016 to send a weekly email to wikitech-l@ about unreviewed patches by new contributors (example). I'd love to continue but this is currently blocked on T151161 due to migration regressions.

IMO it's great that we are experimenting with new approaches to this very fundamental and underacknowledged problem, but code review hours never made sense to me. Reviewing a patch is not like answering a question, it takes way more time, setting up a test environment etc (or I can +1 it without testing in which case I haven't really helped anyone). It's a good idea to have a small queue where (typically not-so-newbie) volunteer developers can put their patches and it gets reviewed within a reasonable amount of time, but that needs way more visibility (such as a weekly wikitech-l reminder with links to patches, plus ircspam) and there is no point in limiting participants to those who are available in a certain hour of the week. Something like "put your patch on this phabricator page" without the focus on IRC and realtime would way more sense. (The CR office hour event pages are used like that to some extent but the way Phabricator events work makes that not very effective.)

There is a deeper organizational problem where code review of patches outside one's team is not really recognized as a valuable activity, but this is probably not the place to go into that.

I personally find real time code reviews useful in some circumstances:

• if there is something really complicated or tricky happening, going over it in irc can help to make sure all the cases are covered (not really relavent to this ticket since that is rare and generally applies to experianced contribs who are easy to contact over irc)
• for very new contributors, where we are not really reviewing the patch so much as advising people how to contribute - real time communication can be very helpful.
• if there are problems with the patch, real time means a very small feedback loop. Issue is identified and fixed immediately instead of problem noted than fixed a week later and then relooked at 2 weeks later by which time the reviewer has totally forgotten context/doesnt care anymore. (Part of this is a cultural issue where reviewers are discouraged from "helping" get a patch to a ready state because they are then no longer qualified as a neutal reviewer)
• [i suspect] having code review office hours means time gets specificly set aside to do code review. Well this is a poor substitute for actually setting time aside, I suspect its better than the status quo.

I agree that anything requring the reviewer to setup a test environment is a non starter in an office hours situation. I think office hours can work when the reviewer is very familar with the code area and the patch is smallish. Other types of code review situations need different solutions. The idea of having a widely published queue sounds good because i think part of the problem is that gerrit is too overwhelming to actually find things in need of review.

I think if we were to ressurect code review office hours we should have some rules to recognize its not a one size fits all sort of thing:

• it should take place in a busyish channel (vitally important to draw in idling devs who arent specificly attending but could be drawn in)
• Smallish patches only (lines is a poor metric, but say 250 at most)
• the person who brings up the patch must be capable of explaining the patch and addressing any issues (i.e. this is not the place for non-programmers to try and lobby for their pet bug with patch to be fixed)
• patch should not have merge conflicts
• Even if patch cant be reviewed, patch authors should revieve advise on what they have to do next (e.g. who to contact).

r

First we need a goal. The goal cannot be "organize a code review hour", the goal must be something like "Let's review all the changesets submitted by newcomers in the past quarter". How to do this (i.e. with real-time sprints or not) will depend on the patches and the reviewers available.

https://www.mediawiki.org/wiki/New_Developers/Quarterly/2017-10#Review_of_changesets_by_new_volunteers says that, as of October 6, 33 changesets submitted by newcomers between July and September were still open. Wouldn't it make sense to address those patches, keeping those new developers engaged and learning?

@Aklapper, is it possible to have that list of changesets published somewhere? Then we can see which projects are affected, and therefore which maintainers and other volunteers might be able to help, and how they prefer to do it.

It would be even better if we could also have a list of open changesets of newcomers submitted during this quarter. Real-time might be too much, but perhaps renewed each month? With the same goal: identify which projects and reviewers could help, and keep newcomers engaged and learning.

Whats the point of keeping newcomers engaged if they become totally ignored the minute they hit some sort of no-longer-a-newcomer class?

Newcomers as a group are probably the most resource intensive for the least benefit (until they graduate to being not a newcomer). Only concentrating on newcomer engagement seems like cutting off our nose despite the face as its essentially investing in the future and then cutting the investment loose when it hits maturity; investing all the work but reaping very little of the potential gains.

My underlying assumption (that might or might not stand a reality check) is that newcomers graduating as not a newcomer have established some connections that they didn't have when they started. If they have a patch waiting for reviewers, they are supposed to have a better idea about who to CC or what to do next.

Note also that our definition of newcomer is "uploaded a first patch less than year ago".

It would be even better if we could also have a list of open changesets of newcomers submitted during this quarter. Real-time might be too much, but perhaps renewed each month? With the same goal: identify which projects and reviewers could help, and keep newcomers engaged and learning.

As an aside, I once read an article (Which I unfortunately cannot find again) which argued that metrics should concentrate more on max time something is pending for rather then number of items pending. The logic was, people will try to game any metric you use, so if the metric is number of items, people will try and just do the easiest of the items, and since easy items get added as people create more patches, the easy items will always be done, and the hard items will totally be ignored. Where if the metric is max time something is pending, people can only improve the metric by dealing with all the issues. I'm not sure how applicable this is to us (Not everyone has the skill to review every patch), but something to think about.

In T173770#3683286, @Qgil wrote:

@Aklapper, is it possible to have that list of changesets published somewhere? Then we can see which projects are affected, and therefore which maintainers and other volunteers might be able to help, and how they prefer to do it.

It would be even better if we could also have a list of open changesets of newcomers submitted during this quarter. Real-time might be too much, but perhaps renewed each month? With the same goal: identify which projects and reviewers could help, and keep newcomers engaged and learning.

https://wikimedia.biterg.io/app/kibana#/dashboard/Gerrit-Backlog lists open (= unmerged and unabandoned and CR-2 or CR-1 or CR0 or CR+1) patchsets with URLs.

https://wikimedia.biterg.io/app/kibana#/dashboard/C_Gerrit_Demo lists the names of new Gerrit accounts with at least one patch. One can export those names (which also include staff and oldcomer accounts not yet merged with their main user accounts in the DB behind) and construct a filtered view (see the search field):
https://wikimedia.biterg.io/goto/193ff3bafe03d3599443ee3f04f49c95

That table initially did not show the corresponding repositories. Being a lonesome admin I fixed that by replacing the row "project" by "repository". This visualization customization will get overwritten by the next software update again.

Question: First, I would like to know if office hours could still be advertised to newcomers looking to get their patches reviewed? As someone recently asked a related question on Discourse.

Here are a few thoughts on this topic:

• As discussed above, I like the idea of adapting the format of Technical Advice IRC meeting for code review hours, sending regular emails, etc. Code review hour advertised well in advance, what patches need help, who can help, etc.
• For addressing volunteer patches, more specifically the ones by newcomers:
  • Per quarter, we get ~50 new volunteers. Manually watching their patches, and connecting them with mentors/reviewers might not be a lot of work. But, for this, we would need an improved version of Open changesets submitted by newbies stats that help support this effort. In its current form, it’s of no use (T186721). An advanced version of the same list could include recent changesets, along with the names of the reviewers, submitter, patch info, date and time, newbie tag, etc.
  • Besides monitoring patches, calling out bad practices, pointing reviewers to adhere to the guidelines and be kind to newcomers. Though we have guidelines in place, after viewing a few recent patches, it seems like not everyone follows them.
  • Recognizing / celebrating both reviewers and contributors through weekly email on Wikitech-I, list of patches submitted, merged, reviewed, etc.
    • Not in the scope of this task, but each of our projects should have a list of contributors and maintainers :-/
  • Educating newcomers on how to seek code review. We have good documentation, but apparently, it is not quite visible.
• IMO, code reviews are more helpful when done face-to-face. And, so I wonder for tedious code reviews and final deployments if it is easier to do/seek code review help over video calls. But, I am not sure if folks who do code reviews would be interested in experimenting with this approach and it would be a good use of their time.

In T173770#3963246, @srishakatux wrote:

Question: First, I would like to know if office hours could still be advertised to newcomers looking to get their patches reviewed?

Errm, what's the intention of advertising an event something that does not take place (according to the task description)? :)

Here are a few thoughts on this topic:

• As discussed above, I like the idea of adapting the format of Technical Advice IRC meeting for code review hours, sending regular emails, etc. Code review hour advertised well in advance, what patches need help, who can help, etc.

I'm fine sending regular emails again once we have a list of patches but I personally don't see much point in trying to encourage developers / maintainers (who might or might not be familiar with the code base of those patches) to be online at a certain time on a certain day to review them which might not be the timezone of the patch contributor. Trying to encourage a patch contributor to join could help creating personal bonds and get people deeper into the community but could also be seen as an additional burden in times of async communication...
I do see a point in encouraging developers / maintainers to make it part of their weekly routine and team culture to review contributed patches though, but that's a team practices thing IMO and nothing I can push for in teams.