Warmup Guide — Review Culture & Patch Evaluation

Zero-to-expert primer for Phase 06: how to review code the way committers do — the checklist and its ordering, the craft of writing review comments, defending and revising your own patches, and reading real patch history as training data.

Table of Contents

Chapter 1: What Review Is For (It Isn't Bug-Finding Alone)

Review serves four functions, and optimizing only the first produces bad reviewers:

  1. Defect detection — yes, but studies and experience agree most defects are caught by tests and tools; review catches the design-level ones they can't (Phase 05 Ch. 5).
  2. Knowledge transfer — after the review, two people understand the change; the bus factor of every merged line is ≥2. This is why "LGTM" without reading is a real harm even when the code was fine.
  3. Standard-setting — review is where a codebase's quality bar is actually enforced and taught; every comment is a tiny lecture other contributors read too.
  4. Trust calibration (the Apache-specific function) — review threads are where the PMC observes your judgment, both as reviewer and author. Phase 01's trust model is operationalized here; it's why this phase sits at the curriculum's center.

Chapter 2: The Review Checklist, Ordered by Blast Radius

Review the irreversible first. The ordering (memorize it; Lab 01 grades against it):

  1. Contract & compatibility: public API/config/wire/format changes? Annotations respected? japicmp/MiMa consulted? (A perfect implementation of a breaking change is still a -1.)
  2. Correctness: new states, error paths, boundary inputs, concurrency. Ask of every branch: what test would fail if this line were wrong — does it exist?
  3. Failure behavior: partial failure, retries, resource cleanup on the exception path. Most production incidents live in code that only runs when something else broke.
  4. Performance: only for hot paths, and then with evidence — demand the benchmark, review its design (Phase 08's JMH pitfalls are reviewer knowledge).
  5. Maintainability: naming, structure, comment quality, test readability — Phase 05's properties.
  6. Mechanics last: style nits the tools missed, typos. Prefix with "nit:" and never block on them.

The anti-pattern this ordering prevents: fifteen naming comments on a patch whose core design breaks rolling upgrades — attention spent inversely to consequence.

Chapter 3: Reading a Diff Effectively

The mechanics nobody teaches:

  • Two passes: first pass for shape — files touched, test-to-code ratio, anything unexpected in scope (a "fix NPE" patch touching 14 files is a scope conversation, not a line-comment conversation). Second pass for lines.
  • Start from the tests: they state what the author thinks the change does; diffs between that and what the code does are your highest-value findings.
  • Read around the diff: the bug pattern git diff hides is the interaction between changed lines and unchanged code — open the full file at the call sites; check other callers of a modified method (find-usages on every signature change).
  • Hunt the asymmetries: resources opened vs closed, locks taken vs released, state mutated vs restored on the exception path, new enum/case added vs every switch over it.
  • For concurrency: write down the guard story (which lock protects which field) as you read; if you can't, that's the comment.

Chapter 4: Writing Review Comments That Work

The craft (Lab 01's rubric, and interview-prep/04's scenarios):

  • Severity-label everything: blocking: / question: / nit: — the author must know what stands between them and merge. Unlabeled comments read as all-blocking and poison the thread.
  • Reasons, not verdicts: "this races when two batches flush concurrently — count is read outside the lock at line 84" is actionable; "this is thread-unsafe" is a homework assignment; "I don't like this" is noise.
  • Questions before accusations when you're <90% sure: "what prevents X here?" — half the time there's an invariant you missed, and the question surfaces it without cost; the other half, the author finds the bug themselves (better teaching).
  • Anchor in shared standards, not personal taste: cite the style guide, an existing pattern in the codebase, a measured number. Taste-comments are where threads go to die.
  • Acknowledge the good — one specific sentence ("the test matrix on the boundary conditions is exactly right"). Not politeness theater: it teaches what to keep doing, and it keeps newcomers (Phase 01's funnel) coming back.
  • Know your confidence: "I believe this breaks X, but verify — I may be missing context" is senior; false certainty in public threads compounds badly.

Chapter 5: Being Reviewed — Responding and Revising

The author-side discipline (Phase 12 Ch. 3 covers the full mechanics; the essentials):

  • Respond to every comment: fixed / pushed-back-with-reasons / clarifying question.
  • Batch revisions — one push addressing all comments; reviewers re-review revisions, not commits.
  • Take preferences cheaply (spend disagreement budget only on what matters); rebut blocking comments with evidence (a test, a benchmark, a spec citation) after restating the concern accurately.
  • When wrong: say so fast, fix, and convert the misunderstanding into a comment or test so the next reader doesn't repeat it.
  • Patch stalled? One polite ping after ~a week, then the project's escalation convention. Meanwhile review others' patches — the queue runs on reciprocity.

Chapter 6: Evaluating Design Tradeoffs in Review

Lab 02 presents competing implementations (token-bucket vs sliding-window rate limiter); the evaluation method generalizes:

  1. Make the axes explicit before judging: correctness under burst, memory per key, contention behavior, observability, complexity. Unstated axes are how design arguments go circular.
  2. Demand the workload: there is no "better rate limiter," only better-for- burst-tolerance or better-for-strict-ceilings. A reviewer's first design question is "what traffic shape and what failure cost?"
  3. Weigh evidence classes correctly: a benchmark beats an argument for the benchmarked case only (check its design — single-threaded JMH proves nothing about contention; Phase 08); an invariant argument beats a benchmark for correctness.
  4. Price the second implementation: every alternative kept alive doubles test surface and doc burden — "both, behind a flag" is sometimes right (workloads genuinely differ) and always expensive; make the cost part of the recommendation.
  5. Decide reversibility: config-defaulted choices are cheap to revisit; wire-format choices aren't (Phase 03 Ch. 5). Spend deliberation proportional to irreversibility.

Chapter 7: Patch Archaeology — Learning from Merged History

Lab 03's method — real merged patches (e.g., the bundled Kafka PR) as training data:

  • Reconstruct the timeline: rounds of review, what changed between revisions, time to merge. The delta between v1 and merged is a map of what this project's reviewers actually require — more honest than any contributing guide.
  • Classify every review comment with Chapter 2's checklist levels — you'll find the distribution skews far more toward contract/failure-mode than newcomers expect.
  • Note the social moves: how disagreement was de-escalated, how scope was cut ("file a follow-up JIRA" as the universal pressure valve), who tipped the decision and with what kind of evidence.
  • Steal templates: good JIRA descriptions, good commit messages, good test names — archaeology gives you the project's house style as worked examples.

Lab Walkthrough Guidance

Order: Lab 01 (flawed patch) → Lab 02 (design tradeoffs) → Lab 03 (archaeology).

  • Lab 01: review MessageBatcher with Chapter 2's ordering — find the concurrency and failure-path defects before polishing nits; write every comment to Chapter 4's rubric (severity label + reason + suggestion). Grade yourself: would your blocking comments survive the author's pushback?
  • Lab 02: write the axes table first (Chapter 6.1) for both rate limiters, run the provided benchmark, then write the recommendation for two different stated workloads — the answer should flip, and the writeup should show why.
  • Lab 03: apply Chapter 7's method to the bundled Kafka patch; the deliverable insight is the comment-classification histogram plus three templates you'd reuse.

Success Criteria

You are ready for Phase 07 when you can, from memory:

  1. Name the four functions of review and why "LGTM-without-reading" damages three of them.
  2. Recite the blast-radius checklist order and justify it with one sentence.
  3. Rewrite a verdict-comment ("this is wrong") into a working comment (reason + severity + suggestion) on demand.
  4. List four diff-reading asymmetries to hunt.
  5. Evaluate two competing designs by explicit axes + workload, and price the keep-both option.

Interview Q&A

Q: Review this PR live. (The meta-question: how do you start?) Narrate the two-pass method: "First, shape — what's touched, where are the tests, does scope match the title. Then contract: any public surface change? Then I'll read tests-first to learn intent, then the diff with attention on error paths and concurrency, then nits last, labeled." Interviewers grade the order more than the findings — it reveals whether you review by blast radius or by line number.

Q: A senior engineer's patch has a real flaw. How do you comment differently than on a newcomer's patch? The technical content is identical — the flaw, the evidence, the severity label; deference on correctness is how bad code ships. What changes is calibration: with a senior author I lead with the question form ("what prevents X?") because the prior that I'm missing context is higher, and I skip the teaching detail a newcomer needs. What never changes: public thread, evidence attached, respectful and direct.

Q: How do you keep review turnaround from becoming the team bottleneck? Make reviewability the author's job too: small PRs (norm, not nag), description states what+why+how-tested, CI green before human eyes. Reviewer side: SLA on first response (even "looking Thursday"), two-pass triage so trivial PRs clear in minutes, severity labels so authors parallelize fixes. And measure it — review latency is a team health metric, not a virtue contest.

References