57 stories
·
2 followers

Gerrit now automatically adds reviewers

1 Comment

Finding reviewers for a change is often a challenge, especially for a newcomer or folks proposing changes to projects they are not familiar with. Since January 16th, 2019, Gerrit automatically adds reviewers on your behalf based on who last changed the code you are affecting.

Antoine "@hashar" Musso exposes what lead us to enable that feature and how to configure it to fit your project. He will offers tip as to how to seek more reviewers based on years of experience.


When uploading a new patch, reviewers should be added automatically, that is the subject of the task T91190 opened almost four years ago (March 2015). I declined the task since we already have the Reviewer bot (see section below), @Tgr found a plugin for Gerrit which analyzes the code history with git blame and uses that to determine potential reviewers for a change. It took us a while to add that particular Gerrit plugin and the first version we installed was not compatible with our Gerrit version. The plugin was upgraded yesterday (Jan 16th) and is working fine (T101131).

Let's have a look at the functionality the plugin provides, and how it can be configured per repository. I will then offer a refresher of how one can search for reviewers based on git history.

Reviewers by blame plugin

The Gerrit plugin looks at affected code using git blame, it extracts the top three past authors which are then added as reviewers to the change on your behalf. Added reviewers will thus receive a notification showing you have asked them for code review.

The configuration is done on a per project basis and inherits from the parent project. Without any tweaks, your project inherits the configuration from All-Projects. If you are a project owner, you can adjust the configuration. As an example the configuration for operations/mediawiki-config which shows inherited values and an exception to not process a file named InitialiseSettings.php:

The three settings are described in the documentation for the plugin:

plugin.reviewers-by-blame.maxReviewers
The maximum number of reviewers that should be added to a change by this plugin.
By default 3.

plugin.reviewers-by-blame.ignoreFileRegEx
Ignore files where the filename matches the given regular expression when computing the reviewers. If empty or not set, no files are ignored.
By default not set.

plugin.reviewers-by-blame.ignoreSubjectRegEx
Ignore commits where the subject of the commit messages matches the given regular expression. If empty or not set, no commits are ignored.
By default not set.

By making past authors aware of a change to code they previously altered, I believe you will get more reviews and hopefully get your changes approved faster.

Previously we had other methods to add reviewers, one opt-in based and the others being cumbersome manual steps. They should be used to compliment the Gerrit reviewers by blame plugin, and I am giving an overview of each of them in the following sections.

Gerrit watchlist

The original system from Gerrit lets you watch projects, similar to a user watch list on MediaWiki. In Gerrit preferences, one can get notified for new changes, patchsets, comments... Simply indicate a repository, optionally a search query and you will receive email notifications for matching events.

The attached image is my watched projects configuration, I thus receive notifications for any changes made to the integration/config config as well as for changes in mediawiki/core which affect either composer.json or one of the Wikimedia deployment branches for that repo.

One drawback is that we can not watch a whole hierarchy of projects such as mediawiki and all its descendants, which would be helpful to watch our deployment branch. It is still useful when you are the primary maintainer of a repository since you can keep track of all activity for the repository.

Reviewer bot

The reviewer bot has been written by Merlijn van Deen (@valhallasw), it is similar to the Gerrit watched projects feature with some major benefits:

  • watcher is added as a reviewer, the author thus knows you were notified
  • it supports watching a hierarchy of projects (eg: mediawiki/*)
  • the file/branch filtering might be easier to gasp compared to Gerrit search queries
  • the watchers are stored at a central place which is public to anyone, making it easy to add others as reviewers.

One registers reviewers on a single wiki page: https://www.mediawiki.org/wiki/Git/Reviewers.

Each repository filter is a wikitext section (eg: === mediawiki/core ===) followed by a wikitext template and a file filter using using python fnmatch. Some examples:

Listen to any changes that touch i18n:

== Listen to repository groups ==
=== * ===
* {{Gerrit-reviewer|JohnDoe|file_regexp=<nowiki>i18n</nowiki>}}

Listen to MediaWiki core search related code:

=== mediawiki/core ===
* {{Gerrit-reviewer|JaneDoe|file_regexp=<nowiki>^includes/search/</nowiki>

The system works great, given maintainers remember to register on the page and that the files are not moved around. The bot is not that well known though and most repositories do not have any reviewers listed.

Inspecting git history

A source of reviewers is the git history, one can easily retrieve a list of past authors which should be good candidates to review code. I typically use git shortlog --summary --no-merges for that (--no-merges filters out merge commit crafted by Gerrit when a change is submitted). Example for MediaWiki Job queue system:

$ git shortlog --no-merges --summary --since "one year ago" includes/jobqueue/|sort -n|tail -n4
     3	Petr Pchelko
     4	Brad Jorsch
     4	Umherirrender
    16	Aaron Schulz

Which gives me four candidates that acted on that directory over a year.

Past reviewers from git notes

When a patch is merged, Gerrit records in git trace votes and the canonical URL of the change. They are available in git notes under /refs/notes/review, once notes are fetched, they can be show in git show or git log by passing --show-notes=review, for each commit, after the commit messages, the notes get displayed and show votes among other metadata:

$ git fetch refs/notes/review:refs/notes/review
$ git log --no-merges --show-notes=review -n1
commit e1d2c92ac69b6537866c742d8e9006f98d0e82e8
Author: Gergő Tisza <tgr.huwiki@gmail.com>
Date:   Wed Jan 16 18:14:52 2019 -0800

    Fix error reporting in MovePage
    
    Bug: T210739
    Change-Id: I8f6c9647ee949b33fd4daeae6aed6b94bb1988aa

Notes (review):
    Code-Review+2: Jforrester <jforrester@wikimedia.org>
    Verified+2: jenkins-bot
    Submitted-by: jenkins-bot
    Submitted-at: Thu, 17 Jan 2019 05:02:23 +0000
    Reviewed-on: https://gerrit.wikimedia.org/r/484825
    Project: mediawiki/core
    Branch: refs/heads/master

And I can then get the list of authors that previously voted Code-Review +2 for a given path. Using the previous example of includes/jobqueue/ over a year, the list is slightly different:

$ git log --show-notes=review --since "1 year ago" includes/jobqueue/|grep 'Code-Review+2:'|sort|uniq -c|sort -n|tail -n5
      2     Code-Review+2: Umherirrender <umherirrender_de.wp@web.de>
      3     Code-Review+2: Jforrester <jforrester@wikimedia.org>
      3     Code-Review+2: Mobrovac <mobrovac@wikimedia.org>
      9     Code-Review+2: Aaron Schulz <aschulz@wikimedia.org>
     18     Code-Review+2: Krinkle <krinklemail@gmail.com>

User Krinkle has approved a lot of patches, even if he doesn't show in the list of authors obtained by the previous mean (inspecting git history).

Conclusion

The Gerrit reviewers by blame plugin acts automatically which offers a good chance your newly uploaded patch will get reviewers added out of the box. For finer tweaking one should register as a reviewer on https://www.mediawiki.org/wiki/Git/Reviewers which benefits everyone. The last course of action is meant to compliment the git log history.

For any remarks, support, concerns, reach out on IRC freenode channel #wikimedia-releng or fill a task in Phabricator.

Thank you @thcipriani for the proof reading and english fixes.

Read the whole story
thcipriani
6 days ago
reply
\o/ Happy this is live now
Share this story
Delete

Long Beach, California, USA

1 Comment

In December 2018 I’ve visited beatiful Long Beach (near Los Angeles) for business. The event was at Queen Mary hotel, a retired British ocean liner. I’ve never been in a similar hotel 🚢. There wasn’t a lot of time for sightseeing, but we’ve visited Aquarium of the Pacific 🐡 and USS Iowa ⚓️.

Read the whole story
thcipriani
6 days ago
reply
good times
Share this story
Delete

Software Archaeology

1 Share

In a previous article I described how I discovered that the utility I needed was already available in my bin, because I had written in twelve years previously, but then forgotten.

Another episode in this series: I save screen and monitor configurations in files under ~/.screenlayout; each is a shell script which, when run, resets the display to use its particular layout. So for example home.sh is for the two-monitor setup I use at home and work.sh is for the two-monitor setup I use at work, where the second monitor is vertically oriented and the primary monitor is farther from my eyes.

Yesterday I wanted a home setup where both monitors had the same resolution and the same display. I opened arandr and set it up the way I wanted, and then prepared to save it to .screenlayout/home-merged.sh.

Except that file already existed, and guess what was in it?

Read the whole story
thcipriani
31 days ago
reply
Share this story
Delete

Sunday, November 11 - initial notes on the intel nuc (NUC6i7KYK)

1 Share

Sunday, November 11

initial notes on the intel nuc (NUC6i7KYK)

Previously: initial notes on the dell xps 13 developer edition (9360).

Rationale: I’ve been using laptops as my primary working machines since I quit my last in-office job in November of 2014, and wanting to go back to a robust desktop setup for almost as long. Laptop ergonomics are a disaster, and while low-level pain from typing too much has been part of my life since I was a teenager, it’s been quite a bit worse lately. I finally decided to do something about this last weekend.

My ideal here would be to take an old case I’ve had since the first time I did a custom PC build in 2001 and jam it full of stuff. I may still do that eventually, but this time around I spent a couple of evenings looking at workstation build lists and manufacturers before throwing up my hands and ordering an Intel NUC, the one with the stupid gamer-aesthetic-looking skull graphic on top of it, at around 3 in the morning while drunk and high.

I’m still mad at Intel for a wide range of pretty mind-boggling security fuckups over the last couple of years (or decades) and I really don’t like giving them money, but here we are.

What I bought:

  • Intel NUC Skull Canyon NUC6i7KYK Kit - Newegg, $528.00
  • Crucial 32GB Kit (16GBx2) DDR4 2133 MT/s (PC4-17000) DR x8 SODIMM 260-Pin Memory, CT2K16G4SFD8213 - Amazon, $289.00
  • Samsung 860 EVO 1TB M.2 SATA Internal SSD (MZ-N6E1T0BW) - Amazon, $162.99
  • The 250 gig version of the above, from Best Buy, after I realized that the 1 TB one wasn’t going to get here in time to do setup this weekend.

(Here’s a pro forma apology for giving Amazon money. The NUC was cheaper on Newegg, and I’d intended to get storage and RAM from them as well, but they were wildly more expensive there and, having reached the point of making an actual decision about what to buy, I knew that further hesitation might derail me for months, so I just pulled the trigger.)

The NUC is a “kit” in the sense that you have to open the case and plug in memory and storage. For those not familiar with these things, they pretty much read as laptop hardware in a little box, sans keyboard and display. They’re basically single-board computers with “real” specs. This one has 4 USB ports, a USB C port, and one each of HDMI and DisplayPort, SD card slot, and headphone/speaker jack. There’s also builtin ethernet and wifi, an optical port of some kind, and what I assume is an IR sensor. It’s not the plethora of legacy ports I really want, but neither is it nearly as lame as the typical modern laptop profile. I think it might be possible to break out some additional interfaces - there’re some connectors on the board under the lid.

Setup: This thing is rumored to run Debian pretty much ok, as long as you update to the latest BIOS first.

So far I’ve:

  • Installed the latest BIOS from Intel by downloading the “Recovery BIOS update” to a USB drive and pressing F7 at start. You then get a menu that lets you select the update to install.
  • Installed Debian Stretch 9.6.0 from the netinst copied to a different USB drive. (I used pv debian-9.6.0-amd64-netinst.iso | sudo dd of=/dev/sda to image the drive.)
  • Copied firmware-iwlwifi_20161130-4_all.deb onto the drive from step one and inserted it when prompted during the Debian install. I’ve had all sorts of trouble with installing proprietary drivers this way in the past, but this time it was fairly painless. I am, as usual, irritate that proprietary drivers are a thing. The computer hardware industry sucks.

Next I’ll spend some time figuring out how to share the vast majority of my home directory between this box and the laptop I still use when I leave the house, while keeping separate configuration for stuff that touches screen resolution and so forth. I’m planning to use relatively low-resolution monitors so that I don’t have to fight with how many of my tools are still terrible at high-DPI environments and how bad my eyes are at reading tiny, tiny fonts.

Read the whole story
thcipriani
68 days ago
reply
Share this story
Delete

I'm a Woman and I Want to Be an Eagle Scout

1 Share

I am a 17-year-old woman from New York who has been lobbying to join the Boy Scouts of America for seven years. I have been told that my efforts are partly responsible for the most historic changes in the organization's 108-year history—to allow girls to join the Boy Scouts, as well as to change its name to gender-neutral Scouts BSA

I did all that because I want to achieve the rank of Eagle Scout and I support other young women to do the same. But because Scouts has imposed a February 2019 start date for its official acceptance of female Scouts, I will age out of the program before I am eligible. I am calling on the organization to accept female members immediately, so that I and other activists won’t be frozen out of benefiting from the changes we helped to bring about. 

Last month, I spent two weeks at Camp Keowa, a Boy Scout camp on a river about 100 miles north of New York City. It was the first time I’d been camping with my troop since I was elected Senior Patrol Leader (unofficially, of course). We hiked, fished, tackled the one-mile swim, and shot arrows, rifles, and shotguns. Afterwards, I was invited to stay on as a volunteer staff member. 

Before I left for camp, I did what thousands of Scouts have done before me—I submitted my project proposal for the Eagle rank. That service project is the culmination of a Scout’s career, bringing together everything we’ve learned in aid of our local community. It's the last hurdle to clear before earning the organization’s highest rank. For my project, I’m proposing an event that will connect veterans with dogs who need adoption, along with volunteers who can help train service and therapy animals. The only problem is that the BSA will not consider that proposal because I am a girl. 

I believe that I am the first girl trying to earn the Eagle rank, just as I was one of the first girls elected to the position of Senior Patrol Leader. I have advanced through the ranks of Scouting with only unofficial acceptance and done that with no guarantee that my years of work would ever culminate in the Eagle Scout award. 

I have shown through my persistence, honorable work, and service to others that I, and my Eagle project, deserve to be recognized and acknowledged. Not only will my project work to benefit our most honored citizens—a key mission of the BSA—but given the BSA’s decision to admit young women, my hard work should no longer be rejected, nor my rank postponed simply based on my gender. 

If the BSA wants to welcome female Scouts, it needs to start now—not next February. Our work to advocate for female admission has created unprecedented change in the organization, but also new challenges. I and the other activists who brought about that change are uniquely positioned to help tackle those challenges. As a change-maker for the BSA, I am confident that I will be supported as a loyal and brave advocate for positive change. 

That’s why I’m calling on the BSA to immediately begin allowing young women to join. All I want is to be able to continue to help make Scouting the best leadership program for all our youth.  

Read the whole story
thcipriani
116 days ago
reply
Share this story
Delete

Production Excellence: September 2018

1 Share

How’d we do in our strive for operational excellence last month? Read on to find out!

Month in numbers

  • 1 documented incident since August 9. [1]
  • 113 Wikimedia-prod-error tasks closed since August 9. [2]
  • 99 Wikimedia-prod-error tasks created since August 9. [3]

Current problems

Frequent:

  • [MediaWiki-Logging] Exception from Special:Log (public GET). – T201411
  • [Graph] Warning "data error" from ApiGraph in gzdecode. – T184128
  • [RemexHtml] Exception "backtrack_limit exhausted" from search index jobs. – T201184

Other:

  • [MediaWiki-Redirects] Exception from NS_MEDIA redirect (public GET). – T203942

This is an oldie: (Well..., it's an oldie where I come from... 🎸)

  • [FlaggedRevs] Exception from Special:ProblemChanges (since 2011). – T176232

Terminology:

  • An Exception (or fatal error) causes user actions to be aborted. For example, a page would display "Exception: Unable to render page", instead the article content.
  • A Warning (or non-fatal error) can produce page views that are technically unaware of a problem, but may show corrupt or incomplete information. For example, an article would display the word "null" instead of the actual content. Or, a user may be told "You have null new messages."

The combined volume of infrequent non-fatal errors is high. This limits our ability to automatically detect whether a deployment caused problems. The “public GET” risks in particular can (and have) caused alerts to fire that notify Operations of wikis potentially being down. Such exceptions must not be publicly exposed.

With that behind us... Let’s celebrate this month’s highlights!

*️⃣ Quiz defect – "0" is not nothing!

Tyler Cipriani (Release Engineering) reported an error in Quiz. Wikiversity uses Quiz for interactive learning. Editors define quizzes in the source text (wikitext). The Quiz program processes this text, creates checkboxes with labels, and sends it to a user. When the sending part failed, "Error: Undefined index" appeared in the logs. @Umherirrender investigated.

A line in the source text can: define a question, or an answer, or nothing at all. The code that creates checkboxes needs to decide between "something" and "nothing". The code utilised the PHP "if" statement for this, which compares a value to True and False. The answers to a quiz can be any text, which means PHP first transforms the text to one of True or False. In doing so, values like "0" became False. This meant the code thought "0" was not an answer. The code responsible for sending checkboxes did not have this problem. When the code tried to access the checkbox to send, it did not exist. Hence, "Error: Undefined index".

Umherirrender fixed the problem by using a strict comparison. A strict comparison doesn't transform a value first, it only compares.

https://phabricator.wikimedia.org/T196684

*️⃣ PageTriage enters JobQueue for better performance

Kosta Harlan (from Audiences's Growth team) investigated a warning for PageTriage. This extension provides the New Pages Feed tool on the English Wikipedia. Each page in the feed has metadata, usually calculated when an editor creates a page. Sometimes, this is not available. Then, it must be calculated on-demand, when a user triages pages. So far, so good. The information was then saved to the database for re-use by other triagers. This last part caused the serious performance warning: "Unexpected database writes".

Database changes must not happen on page views. The database has many replicas for reading, but only one "master" for all writing. We avoid using the master during page views to make our systems independent. This is a key design principle for MediaWiki performance. [5] It lets a secondary data centre build pages without connecting to the primary (which can be far away).

Kosta addressed the warning by improving the code that saves the calculated information. Instead of saving it immediately, an instruction is now sent via a job queue, after the page view is ready. This job queue then calculates and saves the information to the master database. The master synchronises it to replicas, and then page views can use it.

https://phabricator.wikimedia.org/T199699 / https://gerrit.wikimedia.org/r/455870

*️⃣ Tomorrow, may be sooner than you think

After developers submit code to Gerrit, they eagerly await the result from Jenkins, an automated test runner. It sometimes incorrectly reported a problem with the MergeHistory feature. The code assumed that the tests would finish by "tomorrow".

It might be safe to assume our tests will not take one day to finish. Unfortunately, the programming utility "strtotime", does not interpret "tomorrow" as "this time tomorrow". Instead, it means "the start of tomorrow". In other words, the next strike of midnight! The tests use UTC as the neutral timezone.

Every day in the 15 minutes before 5 PM in San Francisco (which is midnight UTC), code submitted to Code Review, could have mysteriously failing tests.

– Continue at https://gerrit.wikimedia.org/r/452873

*️⃣ Continuous Whac-A-Mole

In August, developers started to notice rare and mysterious failures from Jenkins. No obvious cause or solution was known at that time.

Later that month, Dan Duvall (Release Engineering team) started exploring ways to run our tests faster. Before, we had many small virtual servers, where each server runs only one test at a time. The idea: Have a smaller group of much larger virtual servers where each server could run many tests at the same time. We hope that during busier times this will better share the resources between tests. And, during less busy times, allow a single test to use more resources.

As implementation of this idea began, the mysterious test failures became commonplace. "No space left on device", was a common error. The test servers had their hard disk full. This was surprising. The new (larger) servers seemed to have enough space to accommodate the number of tests it ran at the same time. Together with Antoine Musso and Tyler Cipriani, they identified and resolved two problems:

  1. Some automated tests did not clean up after themselves.
  2. The test-templates were stored on the "root disk" (the hard drive for the operating system), instead of the hard drive with space reserved for tests. This root disk is quite small, and is the same size on small servers and large servers.

https://phabricator.wikimedia.org/T202160 / https://phabricator.wikimedia.org/T202457

🎉 Thanks!

Thank you to everyone who has helped report, investigate, or resolve production errors past month. Including:

Tpt
Ankry
Daimona
Legoktm
Volker_E
Pchelolo
Dan Duvall
Gilles Dubuc
Daniel Kinzler
Umherirrender
Greg Grossmeier
Gergő Tisza (Tgr)
Sam Reed (Reedy)
Giuseppe Lavagetto
Brad Jorsch (Anomie)
Tim Starling (tstarling)
Kosta Harlan (kostajh)
Jaime Crespo (jcrespo)
Antoine Musso (hashar)
Roan Kattouw (Catrope)
Adam WMDE (Addshore)
Stephane Bisson (SBisson)
Niklas Laxström (Nikerabbit)
Thiemo Kreuz (thiemowmde)
Subramanya Sastry (ssastry)
This, that and the other (TTO)
Manuel Aróstegui (Marostegui)
Bartosz Dziewoński (matmarex)
James D. Forrester (Jdforrester-WMF)

Thanks!

Until next time,

– Timo Tijhof


Further reading:

Footnotes:

[1] Incidents. – https://wikitech.wikimedia.org/wiki/Special:AllPages?from=Incident+documentation%2F20180809&to=Incident+documentation%2F20180922&namespace=0
[2] Tasks closed. – https://phabricator.wikimedia.org/maniphest/query/wOuWkMNsZheu/#R
[3] Tasks opened. – https://phabricator.wikimedia.org/maniphest/query/6HpdI76rfuDg/#R
[4] Quiz on Wikiversity. – https://en.wikiversity.org/wiki/How_things_work_college_course/Conceptual_physics_wikiquizzes/Velocity_and_acceleration
[5] Operate multiple datacenters. – https://www.mediawiki.org/wiki/Requests_for_comment/Master-slave_datacenter_strategy_for_MediaWiki

Read the whole story
thcipriani
119 days ago
reply
Share this story
Delete
Next Page of Stories