Expedited review for trivial fixes

Stefan Johanssen, John Rose (April 2017)

Note: This is an outline of a proposal, not yet actionable except for discussion. Suggested venue is hotspot-dev.

Sometimes programmers hesitate to make small, low-risk changes because the review process outweight the change. As a result, there is a hazard that some kinds of change are not made as quickly as they should.

Defining a process for expedited review, and an expectation of what trivial fixes could be expedited, will address this problem. If we learn to use this distinction with care, our engineering process will go a little faster.

Being clear about these definitions will help the OpenJDK community adjust expectations about prior and post-hoc reviews, and maintain transparency about committer decisions.

What’s a trival fix?

A trivial fix is small, useful, and safe. That is, a trivial fix is a small change to a localized part of the source code, which is somehow obviously useful, and which has a negligible probability of causing a regression.

Here are some examples of trivial fixes:

But a fix is not trivial if it:

Programmers may consider rolling a trivial fix as an extra fix into some other body of non-trivial work. In that case, adding the extra fix add only trivial amounts of complexity to the main body of the work. Specifically:

Expedited review

The review process for a trivial fix may be expedited, at the discretion of the author and any reviewers. A change set with an expedited review may have a reduced number of reviewers. Or it may have a reduced requirement for advance notification, including a reduced waiting period.

Different projects have varying standards for review, and these standards may also vary over time. Project rules govern whether expedited reviews are allowed, and what review restrictions are relaxed. If a project does not explicitly allow expedited reviews, no reviews may be expedited.

Note that post-facto notification always occurs via the usual Mercurial post-commit notifications. Because of their volume, many people do not subscribe to these notifications, or they filter them.

In order to detect and extract trivial fixes for post-commit review, an expedited review must be marked <TBD>in the “Reviewed-by:” list, along with all human reviewers, by the case-sensitive pseudo-name “Expedited” </TBD>. The OpenJDK user ID “expedited” should be reserved to avoid confusion.

Suggested use by projects

If a project allows expedited review, it must define exactly which changes qualify for expedited review, and what are the (reduced) requirements for reviewing those changes.

Some projects require an e-mail notification followed by a 24-hour wait for people to see the e-mail. Such projects might allow a trivial fix to be committed immediately after review, without prior notification.

Sometimes review requirements are subject to temporary changes, such as near release milestones. In that case, the project must also make clear the rules for expedited review during those special times.

Here are suggested rules which might be adopted by projects, either wholly or in part:

  1. Trivial fixes may be rolled as extra fixes into other changesets.
  2. Trivial fixes, as defined in this document, have expedited review.
  3. Expedited reviews must be marked as such in the changeset comments.
  4. Expedited review requires one fewer reviewer, but at least one.
  5. Expedited changes do not require E-mail notifications or a waiting period.
  6. Expedited changes may be self-reviewed, if only one reviewer is required and the committer is also a reviewer.

Some thoughts on regulation

Adding expedited review introduces a small moral hazard. For example, a fix does not become trivial simply because the project is near a deadline or there is some other reason for haste, and yet someone might be tempted, just before a deadline, to use an expedited review for a change that really needs some scrutiny.

Avoiding such awkwardness requires self-regulation by authors and reviewers. Fortunately, this is not a new kind of regulation. Even full-scale review processes could be disregarded or “work around” by careless committers reviewers. Such faux pas happen occasionally, but social pressure in the community prevents it from happening frequently.

Usually the commit is called out and apologized for. Sometimes the complaint is made privately to the committer, whose behavior improves in the future. Occasionally the offending committer self-reports and apologizes publicly. Such matters are typically handled with grace and courtesy in our community. In any case, people are more careful afterwards. These natural social processes will still apply even in cases where review is expedited.

In particular, prior review of changes (with waiting periods) is neither necessary nor sufficient to enforce good behavior. The proper function of prior review is to reduce errors in risky changesets by allowing reviewers (in all time zones) to nominate themselves when they recognize a changeset that falls within their expertise. But trivial fixes are exactly those changes which fall within everyone’s expertise.