Upgrade to Pro — share decks privately, control downloads, hide ads and more …

How To Be A Bad-Ass Code Reviewer

569f10721398d92f5033097ac6d9132c?s=47 Tim Hockin
November 18, 2019

How To Be A Bad-Ass Code Reviewer

Code review is really important and REALLY hard.


Tim Hockin

November 18, 2019


  1. How To Be A Bad-Ass Code Reviewer KubeCon Contributor Summit

    Nov, 2019 Tim Hockin <thockin@google.com> Software Engineer, Google @thockin (c) Google LLC
  2. Kubernetes has become a phenomenon • One of the most

    active OSS projects EVAR! • Hundreds of repos on github • Contributors from every timezone and almost every continent (Antarcticans - if you’re out there, holla!) We receive scores to hundreds of Pull Requests per day Every single PR needs someone to review it! Why do I care?
  3. Good code review is HARD

  4. Make time^H^H^H^H^H enough time • Doing a full pass in

    a single sitting is best Get into the mindset • Close email, chat, Slack, IRC, Zoom, etc. • Headphones or similar environmental neutralizers Read the associated bugs • There ARE bugs linked, right? Understand WHY the change is being made Before starting
  5. Read and understand the change Understand the context: the surrounding

    code and adjacent features matter a lot Think about: • Test cases • APIs • Comments and docs Start the hard work
  6. Every change makes the code either better or worse

  7. Let’s talk about how to do stronger code reviews

  8. Is the bug cited? Is the bug real and worth

    fixing? Does this actually fix it? Does this prove that it fixes it (tests, benchmarks)? Is this the “right” fix? If not - it’s OK to push back Motivation: Bug fix
  9. Do we really want the feature? How does it interact

    with other features? Is the community (SIGs) OK with the design? Where’s the KEP? Motivation: Feature
  10. Style

  11. Is it the simplest way to express the solution? •

    Can it be broken down further? Are functions, types, variables named clearly? • Go style encourages short names for locals, but sometimes “frobber” really is better than “f” • Code should “read well” Whitespace can be as valuable as code to clarity Clarity
  12. Do the changes make sense in the places they are

    being made? Do they fit the mission of the type or function? • It’s OK to add new types or functions • Don’t force-fit! Cohesion
  13. Can the change be broken down any further? • Smaller,

    focused PRs are better Is it complete? • Opposite of above - don’t commit half-cooked work Is it exposing details that callers don’t need to know? • Can it be pre-factored to reduce scope? Scope
  14. The more deeply embedded, the less the API matters •

    E.g. breaking out a function for readability vs exporting The more widely visible, the more the API and generality matters • Hyrum’s law - all exposed surface will be used • Special cases for things like k/utils Generality
  15. Substance

  16. If code does ANYTHING non-obvious, it needs to explain WHY

    • Almost never “what” or “how”, unless obscure • If you did some analysis to make a decision, say what • If you chose something at random, say that • If you think you will need to revisit some decision later and you are leaving room for that, say so If the next reader’s instinct will be to undo some of this, make sure to leave a note Comments
  17. Does the code handle errors? • Return, log, retry -

    DO NOT IGNORE (or comment why) • Wrap errors when context is needed Think about corner-cases and “pathological” situations Does it fail gracefully? • Will it retry? • Is this clear in the code and comments? • How will the user know something failed? Errors and failure
  18. Does it log significant actions and errors? • Are the

    logs useful and/or actionable? • Are they spammy? • Do they have enough data to comprehend what actually happened? Does it have metrics, when appropriate? • Do they tell us something we actually want to know? Observability
  19. If the PR involves config fields, APIs, CLIs, or flags:

    • Think about how they will be used: do they feel natural? • Are they consistent with other instances? • Would they impede or confuse other planned work? • Principle Of Least Surprise • If we were starting it all over, what we would want? UX
  20. Almost all code changes require test changes • A code

    PR without tests is an immediate smell • Even if tests didn’t exist before, do better Warning signs • Do the tests require twiddling some global variable or flag? • Do the tests anticipate the most obvious ways for future me to break your feature? • Are the tests at the smallest scope possible? • Do the tests use time.Sleep? Testing
  21. Some PRs are about performance, lock contention, resource usage, scalability,

    etc. These sorts of changes must be well commented These PRs really should come with a benchmark • To demonstrate efficacy • To prevent regressions Performance and scale
  22. Is the PR complete? • API definitions and implementation •

    Client and server • All the functionality for a whole change We don’t want to merge half-complete features • It’s too easy to miss a release cutoff • OK to seek review before complete, but not merge Completeness
  23. Many PRs require knowledge of the context • What has

    been done in the past • Why things were done the way they were • What performance metrics matter or have been a problem in the past • How other features will interact with this change • What we want to do in the future Domain specific knowledge
  24. If you need context you do not have: GO GET

    IT! • Go exploring - software archaeology is fun • git grep is your friend • Do experiments or patch in the change and try it • Ask for a consult • Ask PR author to document the results in comments • links to previous issues / changes / discussions are VERY helpful for future others I’ve seen too many PRs approved without sufficient context that need to be fixed later • I have been guilty of this, too Domain specific knowledge
  25. If you don’t know how a change interacts with other

    code or features, or how it fits into the subsystem: DO NOT APPROVE! Domain specific knowledge
  26. Commits matter • They are the permanent record of what

    happened and why Organize large PRs into intentional, discrete steps • Easier up-front, but possible ex post facto Commits should almost always stand on their own OK to have “fix feedback” commits during review • Need to fold those back to real commits BEFORE merging • Yes it is tedious, sorry (not sorry) Commit hygiene
  27. PRs are not really represented in git history PR comment

    should summarize, explain what the PR is about and why, link to relevant docs (e.g. KEPs) and reference bugs Commit comments should be thorough and detailed • Enough for git log to be useful • WHY > HOW PR and commit comments
  28. Like refactoring, but done BEFORE making changes • Arranges code

    to better accommodate a future change • Reduces the size and scope of the real change • Should be no-op code changes, plus tests Prefactoring is not just acceptable - encourage it! • Any significant change will almost certainly require it • Can be separate PRs or commits in a larger PR Prefactoring
  29. Process

  30. DO: Ask questions and seek to understand • “Am I

    following this correctly?” • “Why was this choice made?” • If you send every comment you write, you are probably doing it wrong DO: Explain your thinking • “If X is true, then Y can’t be true, but this doesn’t handle that case” Communication
  31. DO: Be clear about nits vs. requirements vs. questions •

    Nit: A small thing that could be improved • Required: “I will not /lgtm until you fix this” DON’T: Pick a bunch of nits if the real problem is that the PR has the wrong approach • Architectural guidance should be first priority Requesting changes
  32. DO: Indicate progress • Be clear about whether you’re finished

    reviewing or not, or what state you think the PR is in overall • It’s frustrating to get batch after batch of comments when you thought you were done DON’T: Approve because of deadlines or guilt • There’s always another release • Again: There’s. Always. Another. Release. Progress
  33. DON’T: Be timid about asking for changes • You are

    the defenders of this codebase • THIS IS YOUR JOB (for the moment) DO: Be reasonable • They won’t do everything exactly how you would have DO: Be willing to be wrong; trust, but verify DO: Be willing to acquiesce on reasonable debate Discussion
  34. Critique CODE not PEOPLE

  35. Strategic tech-debt should be rare but not unheard of Discuss

    debt and tradeoffs with your SIGs and other reviewers, if needed This project is massively distributed - we HAVE TO trust each other to make good decisions Tradeoffs
  36. Code review is community service, and must not be taken

    lightly • Make time, and don't half-ass it • We have to live in the codebase we create • Trust your instincts: if it doesn’t smell right, investigate • You’re guaranteed to find either a bug in the code or a bug in your instincts • Do not approve things you don’t understand fully At the end of the day...
  37. DO NOT BE A RUBBER STAMP I’d rather we slow

    the whole project down than merge garbage
  38. TO PR AUTHORS Code review time is exponential to the

    PR size Smaller PRs have many more chances to schedule!
  39. None