Phase 06 — Review Culture and Patch Workflow
Prerequisite: Phase 05 (you know what Checkstyle and SpotBugs catch; now you learn what humans catch)
Outcome: You can read any PR as a committer would — produce a structured, actionable review; argue for a design with evidence; and read a raw git diff to find problems before they hit main.
Why This Phase Matters
Getting +1 from a committer means your code survived three filters: CI (automated), style review (Checkstyle/SpotBugs), and human review. The human review is the only filter that can catch wrong abstractions, subtle concurrency bugs, API contract regressions, and missing test coverage.
Apache committers spend 30–50% of their time reviewing code, not writing it. If you want to become a committer, you need to demonstrate that you can protect the codebase — not just contribute to it.
Labs
| Lab | Artifact | What You Learn |
|---|---|---|
| Lab 01 — FlawedPatch | MessageBatcher.java with 6 planted review issues | Structured review comments, severity labeling, finding what tests miss |
| Lab 02 — Design Tradeoffs | Two RateLimiter implementations + benchmark | How to argue for a design with code and data; the "+1 with conditions" review pattern |
| Lab 03 — Patch Archaeology | Synthetic Kafka-style .patch file | Reading raw git diffs; retrospective review; identifying what slipped through |
Core Concepts
| Concept | One-line definition |
|---|---|
| Binding vs non-binding vote | A committer's vote counts toward the required 3 +1s; a contributor's vote is informational |
| API contract regression | A change that breaks callers compiled against the previous version (binary compat) or breaks callers that recompile (source compat) |
| Design vote email | A message to dev@ presenting two approaches with tradeoffs, asking for community preference before writing code |
| Patch archaeology | Reading a merged commit retrospectively to understand what the reviewer should have caught and why it wasn't |
-1 with veto | A PMC member can block a release or major API change with a -1; the veto must include a technical reason and can be overridden only by a majority PMC vote |
Deliverables Checklist
-
MessageBatcher.java— all 6 issues found, 4 correctness bugs fixed, all tests pass -
ApproachComparison.md— written vote email comparingTokenBucketRateLimitervsSlidingWindowRateLimiter, citing benchmark output -
kafka-pr-17873-review.md— retrospective review of the synthetic patch with severity labels for each issue found -
mvn testgreen in all 3 lab modules
Interview Relevance
| Question | Lab |
|---|---|
| "What's the difference between source and binary compatibility?" | Lab 01 (Issue #2) |
| "How would you review a concurrent data structure change?" | Lab 01 (Issue #1) |
| "How do you choose between a token bucket and sliding window rate limiter?" | Lab 02 |
| "Walk me through how you'd review a non-trivial pull request" | Lab 01 + Lab 03 |
| "What should a -1 review comment include?" | All labs |