Page MenuHomePhabricator

Goal: Organize a Gerrit Cleanup Day on September 23, 2015
Closed, ResolvedPublic

Tokens
"Mountain of Wealth" token, awarded by RandomDSdevel."Like" token, awarded by D3r1ck01."Like" token, awarded by Fhocutt."Love" token, awarded by Qgil."Like" token, awarded by polybuildr."The World Burns" token, awarded by Addshore."Love" token, awarded by Rdicerb."Mountain of Wealth" token, awarded by Florian."Love" token, awarded by TTO."Love" token, awarded by Aklapper."Yellow Medal" token, awarded by Ricordisamoa."Manufacturing Defect?" token, awarded by Reedy."Like" token, awarded by jayvdb."Like" token, awarded by fbstj.
Assigned To
Authored By
Qgil, Feb 4 2015

Description

Gerrit Cleanup Day on Wednesday September 23, 2015

A full day committed to reviewing proposed code changes in Gerrit.

Result

Partially completed

All WMF developer teams using Gerrit participated, although with different degrees of engagement.

The queue of changesets without any review was reduced by 18% (total) and 25% (last 3 months).

BUT 752 changesets were still unreviewed (was 910), 314 from the past 3 months (was 406).

  • We committed to the goal of 100% without a calculator; we are still happy about the 18%-24%.
  • Lessons learned are on their way, but first impression is that the experience was positive and it helps sensibilizing our teams and our technical community about improving our code review practices.

Situation and aim

Health check survey results/FY2014-15 Q3 indicates an overall notable trend on "Persistent concerns about code review".
There are more than 1200 changesets waiting for review and the age of open changesets is increasing.
Cleaning up Gerrit alone will not solve this, but will help raising the awareness of this problem and help evaluating approaches to improve our code review practices.
Overall aim: Reduce queue length and median age of unreviewed changesets with a focus on volunteer contributions.

Involved parties

All WMF Engineering and Technology teams join the Day. @Aklapper from the Developer-Advocacy Team is the main organizer of the Day. @MBinder_WMF helps with related Team-Practices. Release-Engineering-Team is available in #wikimedia-releng in case of problems with Jenkins/CI.

Your steps to follow

  • Join the #wikimedia-dev channel on Freenode IRC for communication and assistance.
  • Query Gerrit for unreviewed changesets in your code area, submitted in the last three months. Prioritize those changesets authored by volunteers and review them.
    • See section below for example queries per team/area! Corrections welcome!
  • Query Gerrit for any further unreviewed changesets in your code area. Prioritize those changesets authored by volunteers and review them.
  • Once your area is "clean", or if your area has less patches to review:
  • General:
    • Mark WIP changesets by adding a "[WIP]" prefix
    • Work-In-Progress changesets: Try to exclude patchsets with "[WIP]" in title.
    • Merging and Quality: The goal is not to merge as many changesets as possible: We are not after reducing the quality of our codebase but after cleaning our backlog and give feedback to contributors. When merging changesets, mergers are supposed to test on Beta Cluster afterwards. Also, changesets without tests in areas where it is possible to write tests should not be merged.
    • Dead code repositories: List unmaintained/deprecated/inactive code repositories in the Etherpad so these repositories can be marked afterwards (cf. T102920), to improve the correctness of our statistics.

Goals

  1. All open changesets submitted by new volunteers in the last three months have at least one review.
  2. (Extended goal) All patches submitted by volunteers (non-WMF/non-WMDE/etc) have at least one review.
  3. (Extended goal) Review/Update further older patches by anybody and examine CR-1 and CR+1 changesets.

Gerrit queries per team/area

Your corrections and additions of repositories to the queries are welcome! Please edit!

Gerrit queries for unreviewed changesets per team/area, submitted in the last three months:

CR=0, last 3mCR=0, allCR-1CR+1Main Contact + IRC name on wikimedia-dev
Community Tech
MW Core (everybody)MW Core (everybody)MW Core (everybody)MW Core (everybody)Editing:Parsing can help with patches affecting includes/parser/*
DiscoveryDiscoveryDiscoveryDiscovery@EBernhardson (ebernhardson on IRC); team plans
Editing:CollaborationEditing:CollaborationEditing:CollaborationEditing:CollaborationTBD on #wikimedia-collaboration
Editing:LanguageEditing:LanguageEditing:LanguageEditing:LanguageTBD on #mediawiki-i18n
Editing:MultimediaEditing:MultimediaEditing:MultimediaEditing:Multimedia@matmarex (and @MarkTraceur) on #wikimedia-multimedia
Editing:ParsingEditing:ParsingEditing:ParsingEditing:Parsing@ssastry (subbu on IRC, in #mediawiki-parsoid if not in #wikimedia-dev)
Editing:VisualEditorEditing:VisualEditorEditing:VisualEditorEditing:VisualEditorJames_F, krenair, edsanders on #mediawiki-visualeditor
Fundraising TechFundraising TechFundraising TechFundraising TechTBD on #wikimedia-fundraising
AnalyticsAnalyticsAnalyticsAnalytics@Nuria (nuria on IRC, in #wikimedia-analytics)
Infrastructure:Release Eng.Infrastructure:Release Eng.Infrastructure:Release Eng.Infrastructure:Release Eng.Deployment systems related: Tyler C ('thcipriani'), CI/QA: Antoine ('hashar') and Dan ('marxarelli'), Site Requests: Chad H; #wikimedia-releng
Infrastructure:ServicesInfrastructure:ServicesInfrastructure:ServicesInfrastructure:Services@mobrovac (mobrovac on #wikimedia-services)
Infrastructure:TechOps/LabsInfrastructure:TechOps/LabsInfrastructure:TechOps/LabsInfrastructure:TechOps/LabsTBD on #wikimedia-operations and #wikimedia-labs; Yuvipanda prepared+internally posted a list of patch URLs
Reading / MobileReading/MobileReading/MobileReading/MobileTBD on #wikimedia-reading and #wikimedia-mobile
PerformancePerformancePerformancePerformanceTBD (team meeting on Mon 21st)
ResearchResearchResearchResearch#wikimedia-research, but nothing to review in Gerrit and not really using it according to Dario
PywikibotPywikibotPywikibotPywikibotask in #pywikibot
WikidataWikidataWikidataWikidataask in #wikidata
CR=0, last 3mCR=0, allCR-1CR+1Main Contact + IRC name on wikimedia-dev

Measurement of success / Evaluation

  • 👎 All WMF developer teams join the Day.
  • 👎 All patches contributed by volunteers and all patches from the past 3 months have at least one review.
  • Compare number of patchsets in queue before and after. (T110947)
  • Compare median age of patchsets in queue before and after. (T110947)
  • Total number of changesets waiting for a reviewer action and Unknown affiliation: 628 (was 751 ~ -17%)
  • Total number of changesets waiting for a reviewer action and Independent affiliation: 124 (was 159 ~ -23,1%)
  • Total number of changesets waiting for a reviewer action, Unknown affiliation and open in the last three months: 261 (was 335 ~ -22%)
  • Total number of changesets waiting for a reviewer action, Independent affiliation and open in the last three months: 53 (was 71 ~ -25,4%

(Data gathered at 00:00 UTC / 17:00 PST hence might be cut-off at the end)

See T113378: Lessons learned from the first Gerrit Cleanup Day for more information / discussion.

Afterwards / Related links

Related Objects

StatusAssignedTask
DuplicateQgil
ResolvedQgil
ResolvedQgil
InvalidNone
InvalidNone
ResolvedQgil
ResolvedDicortazar
ResolvedAklapper
OpenNone
DeclinedNone
OpenNone
ResolvedQgil
ResolvedQgil
ResolvedQgil
ResolvedQgil
ResolvedAklapper
ResolvedNone
ResolvedAklapper
ResolvedAklapper
ResolvedAklapper
ResolvedDicortazar
ResolvedDicortazar
ResolvedAcs
ResolvedDicortazar
ResolvedDicortazar
ResolvedDicortazar
ResolvedDicortazar
ResolvedDicortazar
ResolvedDicortazar
InvalidDicortazar
ResolvedDicortazar
ResolvedDicortazar
ResolvedAklapper
ResolvedDicortazar
ResolvedDicortazar
DuplicateNone
ResolvedDicortazar

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Aklapper updated the task description. (Show Details)Sep 3 2015, 5:04 PM
Aklapper updated the task description. (Show Details)Sep 3 2015, 10:15 PM
Aklapper updated the task description. (Show Details)Sep 4 2015, 9:19 AM
Aklapper updated the task description. (Show Details)Sep 11 2015, 2:13 PM
Aklapper updated the task description. (Show Details)Sep 11 2015, 2:43 PM
Aklapper updated the task description. (Show Details)Sep 14 2015, 12:57 PM
Aklapper updated the task description. (Show Details)Sep 14 2015, 6:08 PM
Aklapper updated the task description. (Show Details)Sep 14 2015, 6:55 PM
Aklapper updated the task description. (Show Details)Sep 15 2015, 6:25 PM
Aklapper updated the task description. (Show Details)Sep 16 2015, 2:09 PM

A per-day code-review dashboard would be helpful (Korma?)

A per-day code-review dashboard would be helpful (Korma?)

Feel free to elaborate in a dedicated task with your idea(s). Thanks!

Aklapper updated the task description. (Show Details)Sep 17 2015, 2:11 PM
Aklapper updated the task description. (Show Details)Sep 17 2015, 3:43 PM

The TechCom didn't talk about this when we planned for our regularly scheduled E67. Should we be trying to reschedule that, or continue to have it on 2015-09-23?

I've edited the ops links to not include mediawiki-config - IMO that isn't really ops responsibility - most of us have never even sync'd it. Not sure whose responsibility it is, however.

Krenair added a comment.EditedSep 18 2015, 12:57 AM

I know that DB ops sync files in there (in fact, to be able to actually sync certain files in that repo, you have to be ops because the rest of us wouldn't be able to merge changes to them on tin).
That said, if the wmf-deployment gerrit group actually acted as a real group of people rather than a simple ACL group, it'd be ours (obviously, this doesn't map to a real team at a Wikimedia organisation, but that doesn't matter). Given that it's deployment... Maybe RelEng? @greg?

greg added a comment.Sep 18 2015, 1:30 AM

sure, put it in releng's. we'll play along nicely :)

I'm going to try to go through patches from non-opsen first. It is only one page long though, and most of it is marked WIP

cscott updated the task description. (Show Details)Sep 18 2015, 3:03 PM

The queries with CR=0 don't exclude V-1. Should patches with V-1 instead be included in the CR-1 queries?

I think we should review CR+1 before CR-1 otherwise CR+1 is in effect a negative vote.

greg updated the task description. (Show Details)Sep 18 2015, 4:56 PM
cscott updated the task description. (Show Details)Sep 18 2015, 5:15 PM
cscott added a subscriber: ssastry.
cscott updated the task description. (Show Details)Sep 18 2015, 6:26 PM
cscott updated the task description. (Show Details)
He7d3r removed a subscriber: He7d3r.Sep 19 2015, 1:24 PM
XZise added a comment.Sep 19 2015, 1:42 PM

Would it make sense for repositories which actually use Jenkins to prioritize those which aren't V+2 as that indicates (at least for Pywikibot) that the user is not white-listed which probably provide one-time patches which are targeted by this task.

Aklapper updated the task description. (Show Details)Sep 19 2015, 1:47 PM

The TechCom didn't talk about this when we planned for our regularly scheduled E67. Should we be trying to reschedule that, or continue to have it on 2015-09-23?

@RobLa-WMF: That's unfortunate. I'd say that rescheduling will create too much work for involved parties (finding new slots when people have time).
A lesson to learn for next time: How could you have gotten aware of this earlier / better?
Notifications were sent to wikitech-l, the (WMF-internal) engineering mailing list and an entry in the (internal) "WMF Engineering" calendar got created. Where else would you have expected a notification so we can make sure next time?

Aklapper updated the task description. (Show Details)Sep 21 2015, 11:58 AM
Qgil awarded a token.Sep 21 2015, 12:17 PM
Aklapper updated the task description. (Show Details)Sep 21 2015, 12:28 PM
Andrew added a subscriber: Andrew.

an entry in the (internal) "WMF Engineering" calendar

That's one of the reasons why I personally missed it. E67 has been on the Phab calendar for a while, whereas Gerrit Cleanup Day isn't on there. Additionally, the RFC meetings are (and have been) on the internal WMF Engineering calendar for some time.

None of the other attendees of the TechCom meeting highlighted this as a potential conflict. While I'm kicking myself for not bringing it up, it's unfortunate that no one highlighted this potential conflict to me. The TechCom meetings have been happening on Wednesday UTC for a very long time now.

Tgr added a comment.Sep 21 2015, 9:41 PM

It's too late now but if there is another code review day in the future, it would be nice to get T63463 done before that. Searching for open changes in a specific area of MW core is not really possible as of now.

On advertising this ...

  • Can we add some information to the Phabricator front page?
  • A site banner on mediawiki.org?
  • Can Gerrit include a banner?

On advertising this ...

  • Can we add some information to the Phabricator front page?

Phabricator does not allow global banners like Bugzilla did. I've added a section to the "News" box on the Phabricator frontpage.

  • A site banner on mediawiki.org?

Personally I have no idea how to do this. Help welcome. (And noted for next time.)

  • Can Gerrit include a banner?

I don't think so, at least I didn't find any steps / documentation for that.

Tgr added a comment.Sep 22 2015, 9:09 AM

For Gerrit a theme with GerritSiteHeader.html seems like an easy way to do it.

Qgil added a comment.Sep 22 2015, 9:35 AM

Banners in mediawiki.org are organized via https://meta.wikimedia.org/wiki/CentralNotice but I think it is too late now to request a banner.

greg updated the task description. (Show Details)Sep 22 2015, 3:50 PM
Aklapper updated the task description. (Show Details)Sep 22 2015, 3:58 PM
Aklapper updated the task description. (Show Details)Sep 22 2015, 4:04 PM
Aklapper added a subscriber: mobrovac.
ssastry updated the task description. (Show Details)Sep 22 2015, 4:09 PM
Aklapper updated the task description. (Show Details)Sep 22 2015, 5:42 PM
Aklapper updated the task description. (Show Details)Sep 22 2015, 7:40 PM
Aklapper updated the task description. (Show Details)Sep 23 2015, 10:30 AM
Aklapper updated the task description. (Show Details)Sep 24 2015, 12:28 PM
Aklapper closed this task as Resolved.Sep 24 2015, 12:30 PM

Thanks to everybody who joined the Cleanup Day yesterday!

  • Followup discussion and evaluation in T113378: Lessons learned from the first Gerrit Cleanup Day - Please join and share your impressions and thoughts: Shall this be repeated? What to change?
  • Updated task summary with results.
  • Closing this task as "resolved" as the first Wikimedia Gerrit Cleanup Day took place.
Aklapper updated the task description. (Show Details)Sep 25 2015, 9:55 AM
Qgil updated the task description. (Show Details)Oct 1 2015, 1:46 PM
Tgr added a comment.Oct 13 2015, 10:59 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.