🛸 Hitchhiker's Guide — Phase 06: Review Culture and Patch Workflow

Read this if: You've gotten a -1 review and didn't understand why, or you've reviewed a PR and left only style comments while the real bug shipped anyway. By the end you should be able to produce a review that a senior Apache committer would recognize as the work of someone who has read the codebase and understands the invariants — not just someone who ran the linter.

0. The 30-second mental model

A code review has three questions:

  1. Is it correct?
     — edge cases, concurrency, null safety, exception contract
     — what tests are missing?

  2. Is the API contract preserved?
     — binary compatibility (existing compiled callers still work)
     — source compatibility (existing source callers still compile)
     — Javadoc accuracy (is the documented contract still true?)

  3. Can the next person maintain it?
     — naming, comments that explain WHY (not WHAT), no dead code
     — no "trust me" constants without citations

Automated tools (Checkstyle, SpotBugs) answer parts of question 3.
Only humans can answer 1 and 2.

1. Anatomy of a Good Review Comment

1.1 The five-part structure

Every substantive review comment should have five parts:

[SEVERITY] Location: One-sentence statement of the problem.

Why it matters: What runtime failure or contract violation does this cause?
Under what inputs or conditions does it trigger?

Evidence: Quote the relevant code (2–5 lines).

Suggested fix: A concrete alternative (code, not just "fix this").

Label: BLOCKER / MAJOR / MINOR / NIT / QUESTION

1.2 Real examples

Good:

[BLOCKER] MessageBatcher.add(): The backing ArrayList is accessed from multiple
threads without synchronization.

Thread A can be in the middle of ArrayList.add() (which grows the internal array
when capacity is exceeded) while Thread B calls add() simultaneously. The result
is either a lost update (one message silently dropped) or an ArrayIndexOutOfBoundsException
depending on the timing of the capacity resize.

    private List<String> batch = new ArrayList<>();  // ← no synchronization

Fix: use CopyOnWriteArrayList, or wrap add/flush in synchronized(this), or use
a ConcurrentLinkedQueue and drain it in flush().

Bad:

This is not thread-safe. Please fix.

The bad version is not actionable — the author doesn't know which line is the problem, what failure mode to worry about, or what to do instead.

1.3 Severity labels in practice

A review that is all NITs and no BLOCKERs means either the code is excellent or the reviewer wasn't looking hard enough.

2. What Tests Don't Prove

Tests prove that specific inputs produce specific outputs. They don't prove:

• Absence of concurrency bugs — a test that passes on your laptop may fail under load when two threads hit the same code path simultaneously.
• Correctness of the API contract — a test can assert that flush() returns a list, but not that the return type change is binary-compatible with callers compiled against the old version.
• Completeness — a 100% line-coverage test suite can still miss edge cases if the tests only exercise the happy path with valid inputs.

When reviewing tests, ask:

• Is there a test for null inputs?
• Is there a test for the boundary condition (empty, exactly-max-size, max-size+1)?
• Is there a test for the failure path (closed batcher, zero-capacity bucket)?
• Is there a concurrent test for any code that will be called from multiple threads?

3. API Contract Regression

3.1 Binary compatibility

Two versions are binary compatible if class files compiled against version N work without recompilation against version N+1.

A method signature change is a binary compatibility break:

// v1 — original
public void flush() { ... }

// v2 — changed return type
public List<String> flush() { ... }
// ← Any caller compiled against v1 that calls flush() will get:
//   java.lang.NoSuchMethodError: flush()V
//   (The JVM looks for flush with void return "V"; can't find it)

3.2 Source compatibility

Two versions are source compatible if source code that compiled against version N still compiles against version N+1.

// Caller source (compiled against v1):
batcher.flush();  // void return, discard result — compiles against v1

// Against v2:
batcher.flush();  // List<String> return, result discarded — STILL COMPILES
                  // (Java allows discarding a non-void return value)

This example is source-compatible but binary-incompatible. This is the subtle case that reviewers miss: "it compiles" does not mean "it's compatible."

3.3 When does this matter in Apache projects?

Apache projects that publish library JARs (Kafka, Spark, Commons) have strict compat policies. The rule:

• Minor version (1.x → 1.y): must be binary compatible
• Major version (1.x → 2.0): may break binary compat, but must document all breaks

If a PR changes a public method signature without a major version bump, it gets a -1.

4. Design Tradeoffs: How to Argue for an Approach

When two approaches are both correct, the right way to advocate for one is:

1. State the tradeoff precisely — not "approach A is better" but "approach A has O(1) acquire() at the cost of O(k) space (k = bucket capacity), while approach B has O(log w) acquire() but O(w) space (w = window width in requests)"
2. Provide evidence — benchmark numbers, not intuition
3. State the use case — for a rate limiter where bursty traffic is acceptable and p99 latency matters, token bucket. For strict "no more than N requests in any window" enforcement, sliding window.
4. End with a concrete recommendation — "I recommend token bucket for this use case because X. If the team prefers strict fairness, sliding window is acceptable with the caveat that it uses O(limit) memory."

This is the structure of a "+1 with conditions" review: you're in favor but with documented constraints.

5. Reading a Patch: git diff Format

A .patch file is a git diff with mail headers. The diff format:

--- a/src/main/java/org/example/NetworkClient.java   ← old file
+++ b/src/main/java/org/example/NetworkClient.java   ← new file

@@ -42,7 +42,12 @@ public class NetworkClient {
                                  ↑    ↑
                               old  new  (start line, count)

 public void connect(String host) {  ← context (unchanged)
-    this.socket = new Socket();     ← removed (red in GitHub)
+    if (host == null) {             ← added (green in GitHub)
+        throw new IllegalArgumentException("host must not be null");
+    }
+    this.socket = new Socket();
 }

When reading a patch:

1. Read the @@ header to understand what method/class you're in
2. Read the - lines (removed) to understand what the code used to do
3. Read the + lines (added) to understand what it does now
4. Check the context lines (no prefix) to understand the surrounding invariants

Common things to look for in the diff:

• A removed null check (regression)
• A new throws clause added without updating the interface
• A method renamed (binary break)
• A synchronized block removed (thread safety regression)

6. Interview Questions — Concepts You Must Explain Out Loud

7. References

• Apache Software Foundation — How the ASF Works
• Apache Kafka Code Review Guidelines
• How to Review Pull Requests — Apache Spark Wiki
• Binary Compatibility — Java SE Specification §13
• OpenJDK Review Process
• Token Bucket vs. Sliding Window — Cloudflare Blog