How To Be A Bad-Ass Code Reviewer

Code review is really important and REALLY hard.

Transcript

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!