Submit a pull request

Original source: The CL author’s guide to getting through code review

Acronym: CL - changelist

Writing good CL description

• Record what change is being made and why it was made

First line

• Short summary of what is being done
• Complete sentence, written as though it was an order
• Follow by empty line.
• Try to keep your first line short, focused, and to the point. The clarity and utility to the reader should be the top concern.

Body is informative

• The rest of the description should be informative
• It might include a brief description of the problem that’s being solved, and why this is the best approach.
• If relevant, include background information such as bug numbers, benchmark results, and links to design documents.

Good CL description

Functionality change

Example:

rpc: remove size limit on RPC server message freelist.

Servers like FizzBuzz have very large messages and would benefit from reuse. Make the freelist larger, and add a goroutine that frees the freelist entries slowly over time, so that idle servers eventually release all freelist entries.

Refactoring

Construct a Task with a TimeKeeper to use its TimeStr and Now methods.

Add a Now method to Task, so the borglet() getter method can be removed (which was only used by OOMCandidate to call borglet’s Now method). This replaces the methods on Borglet that delegate to a TimeKeeper.

Allowing Tasks to supply Now is a step toward eliminating the dependency on Borglet. Eventually, collaborators that depend on getting Now from the Task should be changed to use a TimeKeeper directly, but this has been an accommodation to refactoring in small steps.

Continuing the long-range goal of refactoring the Borglet Hierarchy.

Small CL that needs some context

Review the description before submitting the CL

CLs can undergo significant change during review. It can be worthwhile to review a CL description before submitting the CL, to ensure that the description still reflects what the CL does.

Why Write Small CLs?

• Reviewed more quickly.
• Reviewed more thoroughly.
• Less likely to introduce bugs.
• Less wasted work if they are rejected.
• Easier to merge.
• Easier to design well.

Handle reviewer comments

• Don't take it personally: the goal of review is to maintain the quality of our codebase and our products. When a reviewer provides a critique of your code, thinks of it as their attempt to help you
• Never respond in anger to code review comment: If you are too angry or annoyed to respond kindly, then walk away from your computer for a while, or work on something else until you feel calm enough to reply politely.

Fix the Code

• If a reviewer says that they don’t understand something in your code, your first response should be to clarify the code itself.

Think for yourself

• It’s often really satisfying to finally send one out for review, feel like it’s done, and be pretty sure that no further work is needed