Lab 2.4 — Review It: Spot the Flaws in a Patch

Lab type: Review-It — read real merged PRs, then find every flaw in a constructed one Estimated time: 75–120 min Tez modules: tez-api (real PR), tez-tools (real PR), tez-dag (constructed exercise)

Background

Reviewing is not a committer-only skill you unlock later — it is how you become trusted enough to be a committer. On an Apache project, thoughtful non-binding review from a contributor is itself a contribution: it lightens the committers' load and demonstrates judgment. This lab teaches you to read a Tez change the way a committer does.

You will work in two parts. Part A reads two real, merged PRs from apache/tez history and extracts the checklist a reviewer actually applies. Part B hands you a constructed, deliberately flawed patch and asks you to find every problem and write a full model review of it. By the end you will have a repeatable review method and the vocabulary — binding vs non-binding, nit:, LGTM — to express it in the project's culture.

Why This Lab Matters for Contributors

The fastest way to learn what a good patch looks like is to critique a bad one against the bar a committer holds.
Reviewing real merged PRs teaches you the house conventions (test-with-fix, scope discipline, message wording) far better than any style guide.
Your own PRs improve the moment you can pre-review them with a committer's eye — you stop shipping the flaws you now know to look for.

Prerequisites

Lab 2.1–2.3 complete.
A checkout of apache/tez with full history (git log works).
Read patch-quality — the quality bar this lab operationalizes.

Part A — Read Two Real Merged PRs

Pick them out of history yourself so you trust they are real:

git log --oneline | grep -E "TEZ-4683|TEZ-4699"

A reviewer's checklist

For any change, a Tez reviewer works down roughly this list. Keep it beside you for Part B:

Dimension	The question
Correctness	Does the change do what the JIRA says, with no logic error?
Scope	Is every changed line necessary, or is there unrelated churn?
Test coverage	Is there a test that fails before, passes after? If none, is the omission justified?
Compatibility	Does it change a public API (`tez-api`), a config key, or a wire/protobuf format?
Style	Checkstyle-clean, spotless-clean, consistent with the surrounding code?
Diagnostics	On failure, does it produce a clear message, or a silent `null`/swallowed exception?
Resource safety	Streams/locks closed? Try-with-resources where appropriate?

PR 1 — `TEZ-4683: Fix tez framework mode config name` (#455)

A one-line change. Read it:

git show TEZ-4683 -- tez-api/src/main/java/org/apache/tez/dag/api/TezConfiguration.java

The entire diff (quoted from tez-api/.../TezConfiguration.java):

-  public static final String TEZ_FRAMEWORK_MODE = TEZ_PREFIX + ".framework.mode";
+  public static final String TEZ_FRAMEWORK_MODE = TEZ_PREFIX + "framework.mode";

Now review it as a committer would. First, find the bug — it is not obvious from the diff alone:

grep -n "TEZ_PREFIX =" tez-api/src/main/java/org/apache/tez/dag/api/TezConfiguration.java

TEZ_PREFIX is "tez." — it already ends in a dot. So the old code produced the key "tez..framework.mode" (a double dot); the fix produces "tez.framework.mode". This is a correctness bug in a public config key.

Reviewer reasoning to internalize:

Correctness: confirmed by expanding the constant, not by reading the diff. Always resolve the constant.
Compatibility: the broken key was almost certainly never usable, so fixing it is safe — but a careful reviewer asks "was tez..framework.mode ever documented or shipped in a release?" before approving, because renaming a working config key would break users.
Scope: minimal — exactly one line. Good.
Test: none, and that is defensible — it is a string constant with no behavior to assert. The reviewer instead verifies by grepping sibling keys for the same TEZ_PREFIX + "." antipattern.
Diagnostics: n/a.

That single line earned a review, a reviewed by Laszlo Bodor attribution, and a squash-merge. Small does not mean unreviewed.

PR 2 — `TEZ-4699: Add Validation/Canonical checks to avoid path exploitation in CSVResult.java` (#471)

A larger, well-formed change. Read it:

git show TEZ-4699 --stat
git show TEZ-4699 -- tez-tools/analyzers/job-analyzer/src/main/java/org/apache/tez/analyzer/CSVResult.java

Stat: CSVResult.java +53/−28, and a new TestCSVResult.java (136 lines). The core of the fix adds a validateOutputFile guard (quoted from tez-tools/.../analyzer/CSVResult.java):

Path baseDir    = Paths.get(System.getProperty("user.dir")).toAbsolutePath().normalize();
Path targetPath = Paths.get(fileName).toAbsolutePath().normalize();
if (!targetPath.startsWith(baseDir)) {
  throw new IOException("Path escapes the allowed base directory. Path: " + targetPath + ...);
}

and rewrites dumpToFile to use try-with-resources over Files.newOutputStream(..., CREATE_NEW, WRITE) with StandardCharsets.UTF_8.

Review reasoning:

Correctness / security: the guard normalize()s both paths and rejects any target that does not start with the base directory — the standard defense against ../../etc/passwd traversal. A reviewer checks the normalize() is applied before the startsWith, which it is.
Test coverage: a whole new TestCSVResult accompanies the fix — this is the pattern Tez wants. The reviewer opens it and confirms it exercises the traversal case, not just the happy path.
Resource safety: try-with-resources replaces the old manual bw.flush(); bw.close(); — an improvement the reviewer would call out approvingly.
Scope: the PR also does incidental style cleanup (@Override moved to its own line, spacing in @SuppressWarnings). A reviewer may allow this because it is in the same file and improves consistency, or may nit: that it muddies the security-focused diff. Both are legitimate.
Behavioral edge: CREATE_NEW throws if the file already exists — is that the intended contract, or a behavior change from the old FileOutputStream (which truncated)? A sharp reviewer raises exactly this question on the PR.

Write, in your notebook, the two-sentence approval-or-question you would post on each PR.

Part B — The Constructed Flawed Patch

The rest of this lab is an exercise. The following diff is constructed for teaching — it does not exist in history. A contributor claims it fixes a bug where TaskImpl.getCounters() returns null before any attempt has completed. There are exactly five flaws. Find them all before reading the answer key.

diff --git a/tez-dag/src/main/java/org/apache/tez/dag/app/dag/impl/TaskImpl.java b/tez-dag/.../TaskImpl.java
--- a/tez-dag/src/main/java/org/apache/tez/dag/app/dag/impl/TaskImpl.java
+++ b/tez-dag/src/main/java/org/apache/tez/dag/app/dag/impl/TaskImpl.java
@@ class TaskImpl implements Task, EventHandler<TaskEvent> {

+  import org.apache.tez.common.counters.TezCounters;
+
   public synchronized TezCounters getCounters() {
     TezCounters counters = null;
     if (successfulAttempt != null) {
@@
       counters = successfulAttempt.getCounters();
     } else {
       counters = attemptList.stream()
-          .filter(a -> a.getState() == TaskAttemptState.SUCCEEDED)
+          .filter(a -> a.getState() == TaskAttemptState.RUNNING)
           .findFirst()
           .map(TaskAttemptImpl::getCounters)
           .orElse(null);
@@
     return counters;
   }

+  /**
+   * Returns the counter for this task, or a new empty TezCounters object
+   * if no counters are available yet.
+   *
+   * @return counters, never null
+   */
+  public synchronized TezCounters getCountersOrEmpty() {
+    TezCounters c = getCounters();
+    return c == null ? new TezCounters() : c;
+  }
+
diff --git a/tez-dag/src/test/java/org/apache/tez/dag/app/dag/impl/TestTaskImpl.java b/.../TestTaskImpl.java
--- a/tez-dag/src/test/java/org/apache/tez/dag/app/dag/impl/TestTaskImpl.java
+++ b/tez-dag/src/test/java/org/apache/tez/dag/app/dag/impl/TestTaskImpl.java
@@ class TestTaskImpl {

+  @Test
+  public void testGetCountersBeforeAnyAttempt() {
+    // No attempts started; counters should not be null
+    initTask();
+    TezCounters result = task.getCounters();
+    assertNotNull("getCounters() must not return null", result);
+  }
+
+  @Test
+  public void testGetCountersOrEmptyReturnsSameObjectEachTime() {
+    initTask();
+    TezCounters first  = task.getCountersOrEmpty();
+    TezCounters second = task.getCountersOrEmpty();
+    assertSame("Must return same instance", first, second);
+  }
+

Your task

Fill this table before revealing the answers:

#	File	Hunk	Flaw	Why it matters	Suggested fix
1
2
3
4
5

Guided questions

Q1 — Import placement. The import was added inside the class body, after the opening brace. Is that a legal location for a Java import? What happens at compile time? And does TaskImpl already import TezCounters? (grep "import.*TezCounters" tez-dag/src/main/java/org/apache/tez/dag/app/dag/impl/TaskImpl.java.)

Q2 — The filter predicate. The JIRA says counters are null before any attempt completes. The patch changes the fallback filter from SUCCEEDED to RUNNING. Does reading counters from a running attempt fix the stated bug? Are a running attempt's counters final and reliable?

Q3 — testGetCountersBeforeAnyAttempt. It asserts assertNotNull on getCounters(). But the patch never makes getCounters() non-null — it adds a separate getCountersOrEmpty(). When successfulAttempt is null and attemptList is empty, what does getCounters() return? Will this test pass against the patched code?

Q4 — testGetCountersOrEmptyReturnsSameObjectEachTime. getCountersOrEmpty() does return c == null ? new TezCounters() : c; — a new object each null call. Does assertSame (reference identity) match that? Is assertSame even testing the documented contract ("never null")?

Q5 — Does it fix the bug at all? The root cause is that getCounters() returns null. The patch leaves getCounters() returning null and adds a new method nobody calls. How many existing callers of getCounters() are there, and are they now safe?

grep -rn "\.getCounters()" tez-dag/src/main/ | grep -v "//" | wc -l

Model Review (read after filling the table)

Reveal the five flaws

#	Flaw	Impact	Fix
1	`import` placed inside the class body (after `{`)	Compile error — Java imports must precede the type declaration. And `TezCounters` is already imported at the top of `TaskImpl`, so it is also redundant.	Delete the added import line entirely.
2	Filter changed `SUCCEEDED` → `RUNNING`	Returns a running attempt's counters, which are partial and change as the task runs — wrong/unstable data. It also does not address the stated bug (no completed attempt yet).	Keep `SUCCEEDED`; handle the "no successful attempt" case explicitly (return null or an empty `TezCounters` by contract).
3	`testGetCountersBeforeAnyAttempt` asserts `assertNotNull` on `getCounters()`	The patch never makes `getCounters()` non-null, so the test fails against the patched code — it does not correspond to any behavior the patch delivers.	Either test `getCountersOrEmpty()`, or change `getCounters()` itself to honor a non-null contract and keep the assertion.
4	`assertSame` on `getCountersOrEmpty()`	Each null-path call returns a new object, so `assertSame` always fails. It also over-specifies — identity is not the documented contract.	Use `assertNotNull`; do not assert reference identity.
5	Adds `getCountersOrEmpty()` but leaves `getCounters()` returning null	The root cause is unfixed. Every existing caller of `getCounters()` (there are several in `tez-dag`) still gets null; the counter-aggregation loops that do `counters.incrAllCounters(...)` still NPE.	Fix `getCounters()` itself to return an empty `TezCounters` (a documented non-null contract), or guard every caller — and delete the unused new method.

The written review a committer would post

Study the tone — specific, line-anchored, constructive, and it separates blocking issues from nits:

Thanks for the patch, and for adding tests — good instinct. A few blockers before this can go in:

TaskImpl.java, the added import: this is inside the class body, so it won't compile; also TezCounters is already imported at the top. Please drop this line.

The RUNNING filter: counters from a running attempt aren't final and will fluctuate — I don't think we want to surface those. The JIRA is about the no completed attempt yet case, so the SUCCEEDED filter should stay and we should decide the contract for "nothing succeeded yet."

Root cause: getCountersOrEmpty() is new API that no caller uses, while getCounters() still returns null — so the callers described in the JIRA are still exposed. Can we instead make getCounters() return an empty TezCounters and document that? That fixes every caller at once. If we add a new method, we need a JIRA discussion about the API surface.

Tests: testGetCountersBeforeAnyAttempt will fail against this patch (getCounters() is still null), and testGetCountersOrEmptyReturnsSameObjectEachTime uses assertSame where the impl creates a fresh object each call — that will always fail. Please assert assertNotNull and target the method whose contract you actually changed.

nit: the Javadoc says "the counter" (singular) — "the counters" reads better.

Happy to re-review once the contract on getCounters() is settled. -1 for now (non-binding).

Apache Review Etiquette

The culture matters as much as the findings. Internalize:

Convention	Meaning
`+1` / `0` / `-1`	Approve / neutral / object. On a PR these appear as review approvals or comments; Yetus even uses emoji votes for its automated checks.
Binding vs non-binding	A committer/PMC member's `+1` is binding (it can gate a merge or a release vote); a contributor's `+1` is non-binding but still valued. Always label yours: "+1 (non-binding)".
`nit:`	A prefix for a non-blocking, take-it-or-leave-it suggestion (style, wording). Signals "don't hold the PR for this."
`LGTM`	"Looks good to me" — an informal approval, usually from someone whose `+1` may be non-binding.
Blocking vs nit separation	Always tell the author which comments must be addressed and which are optional. Burying a compile error under five style nits is poor review.
Tone	Critique the patch, never the person. "This filter returns unstable data" — not "you don't understand counters."

Review is meritocratic currency: sustained, high-quality review is one of the things that earns committership (see committer-mindset and code-style-trust). You do not need to be a committer to review — start now.

Deliverables

Your filled reviewer-checklist notes for both real PRs (TEZ-4683, TEZ-4699), including the double-dot bug you had to expand the constant to see, and the CREATE_NEW edge-case question.
The completed five-flaw table for the constructed patch, matching the answer key.
A written model review of the constructed patch in the committer voice above, separating blockers from nits and ending with a labeled vote.
A one-paragraph note on when an "additive workaround" (new method beside the broken one) is acceptable vs when it is wrong.

Troubleshooting

Symptom	Cause	Fix
You found fewer than 5 flaws	You reviewed the diff without resolving symbols/behavior	Resolve constants, check the JIRA intent, and mentally run each test against the patched code.
You flagged the style cleanup in PR 2 as a blocker	Confused nit with blocker	In-file, consistency-improving cleanup is a `nit:`, not a `-1`.
You marked `getCountersOrEmpty()` as "fine, it's new API"	Missed that it doesn't fix the root cause	New API that no caller uses does not fix a bug in existing callers.
Your review reads as harsh	Critiqued the author, not the code	Anchor every comment to a line and a consequence; drop second-person judgments.

Stretch Goals

Review a live PR. Open a currently-open apache/tez PR on GitHub, run the checklist, and draft a non-binding review comment. Do not post unless it adds real value.
Find a real "test with the fix" thread. In merged history, find a PR where the reviewer asked for a test to be added or changed (git log --grep="test" is a start). Quote what they said.
Re-review your own Lab 2.3 patch. Apply the full checklist to your own fromString fix. Did you leave any printStackTrace, any scope creep, any missing edge case?
Audit for the counter-null pattern. The constructed bug is plausible — grep tez-dag for getCounters() callers that don't null-check, and see whether any are genuinely exposed.

Validation / Self-check

In PR 1, what made the one-line change a real bug, and how did you have to look beyond the diff to see it?
In PR 2, name two things a reviewer approves of and one behavioral edge-case worth questioning.
In the constructed patch, which single flaw is a hard compile error, and why?
Why does changing the filter to RUNNING fail to fix the stated bug and introduce a new one?
Why do both new tests in the constructed patch fail against the patched code?
What is the difference between a binding and a non-binding +1, and how should you label yours as a contributor?

You have completed Level 2. You can navigate the repository, prepare a properly-formatted PR, fix a real bug with a reproducing test, and review a change like a committer. Next: Level 3.

Open-Source Engineer & Contributor