Lab 2.3 — Fix It: `NullPointerException` in `TezTaskAttemptID.fromString`

Lab type: Fix-It — investigate real behavior → locate → write failing test → patch → verify → open PR Estimated time: 90–150 min Tez module: tez-common → org.apache.tez.dag.records.TezTaskAttemptID

Background

TezTaskAttemptID is the identifier that ties a task attempt to its task, vertex, DAG, and application. Its toString() produces a canonical form like:

attempt_1609459200000_0001_1_00_000000_0

and its static fromString(String) is supposed to parse that back. It is called all over the codebase — recovery, history events, the AM, the analyzers — usually followed immediately by .getId() or .getTaskID() on the result.

This lab is titled after a NullPointerException, but the first thing you will discover is that the real behavior is more interesting than the title suggests — and that discovery is the lab. You will investigate what the current code actually does with malformed input, contrast it with a sibling class that does it right, write a reproducing test, derive the fix, and take it through the full contribution flow from Lab 2.2.

This is a live bug, not a museum piece. Unlike many "fix-it" exercises, the defect you study here is present on current master. You will confirm that with git log/git blame, and the fix you write is one you could genuinely open as a PR.

Why This Lab Matters for Contributors

Parsing and validation bugs are the archetypal Minor first contribution: small, self-contained, testable, and genuinely valuable.
"Swallow the exception and return null" is one of the most common real-world antipatterns, and the damage it does (a NullPointerException far from the actual fault) is exactly what makes bugs expensive to diagnose. Seeing it in production code, in a library you use, is instructive.
The fix requires you to match an existing convention in the same package rather than invent your own — the single most important habit for getting a patch merged.

Prerequisites

Lab 2.1 and Lab 2.2 complete.
A TEZ-XXXX branch off current master on your fork.
You can run mvn test -pl tez-common.

Step-by-Step Tasks

Step 1: Locate the Real Source File

The class name says nothing about its module. Find it:

find . -name "TezTaskAttemptID.java" -path "*/main/*" -not -path "*/target/*"

Expected:

./tez-common/src/main/java/org/apache/tez/dag/records/TezTaskAttemptID.java

Note the package is org.apache.tez.dag.records, and the module is tez-common — not org.apache.tez.common, not tez-dag. Open it and read fromString in full.

Step 2: Investigate — What Does It Actually Do?

Read the fromString method carefully. On current master it has this shape (quoted from tez-common/.../records/TezTaskAttemptID.java; run grep -n "fromString" ... to find it):

public static TezTaskAttemptID fromString(String taIdStr) {
  try {
    int pos1 = taIdStr.indexOf(SEPARATOR);
    // ... more indexOf / substring / Integer.parseInt ...
    return TezTaskAttemptID.getInstance(/* ... */);
  } catch (Exception e) {
    e.printStackTrace();
  }
  return null;
}

Now reason about it before you run anything. Answer these:

#	Question
1	If `taIdStr` is `null`, which line throws, and what exception type?
2	That exception is inside the `try`. What does the `catch (Exception e)` block do with it?
3	So does `fromString(null)` throw to its caller, or return something? What?
4	Where does the stack trace go — a logger, or `System.err` via `e.printStackTrace()`?
5	For `"attempt_1609459200000_0001_1"` (too few parts), which call throws, and what does the caller receive?

The punchline: fromString does not throw a NullPointerException to its caller. It catches the NPE (and every other exception), prints a stack trace to stderr, and returns null. The real NullPointerException the title refers to happens later, in a caller that does fromString(x).getId() on the null return. That is far worse than an immediate exception: the failure surfaces away from its cause, with a misleading stack trace.

Step 3: See the Damage — Find the Callers

The return value is used unguarded across the codebase:

grep -rn "TezTaskAttemptID.fromString" --include=*.java . \
  | grep -v target | grep -v "records/TezTaskAttemptID.java" | head -20

You will find call sites in tez-dag (recovery events, TaskAttemptImpl), tez-runtime-internals (TezEventUtils), and the analyzers/history parsers in tez-plugins and tez-tools — many of which immediately dereference the result. A null from fromString becomes an NPE in each of them.

Step 4: Find the Class That Does It Right

The sibling TezDAGID, in the same package, is the model. Read its fromString:

grep -n -A22 "public static TezDAGID fromString" \
  tez-common/src/main/java/org/apache/tez/dag/records/TezDAGID.java

It validates and throws (quoted from tez-common/.../records/TezDAGID.java):

String[] split = dagId.split("_");
if (split.length != 4 || !dagId.startsWith(DAG + "_")) {
  throw new IllegalArgumentException("Invalid DAG Id format : " + dagId);
}
// ...
} catch (NumberFormatException e) {
  throw new IllegalArgumentException("Error while parsing App Id '" + split[2] + "' ...");
}

That is the contract library code should have: reject bad input loudly, at the boundary, with an IllegalArgumentException that names the offending string. Confirm that TezVertexID.fromString and TezTaskID.fromString share the same swallow-and-return-null bug as TezTaskAttemptID:

grep -n -A20 "fromString" tez-common/src/main/java/org/apache/tez/dag/records/TezVertexID.java
grep -n -A20 "fromString" tez-common/src/main/java/org/apache/tez/dag/records/TezTaskID.java

They do. That is a decision for your JIRA description: fix just TezTaskAttemptID, or fix all three consistently? (For a first PR, scope to one and file follow-ups; note the siblings in the JIRA.)

Step 5: Read the Existing Tests

find . -name "TestTezIds.java" -not -path "*/target/*"

Open tez-common/src/test/java/org/apache/tez/dag/records/TestTezIds.java. Study testInvalidDagIds() — it is your template:

try {
  dagId = TezDAGID.fromString("dag_111_11");
  Assert.fail("Expected failure for invalid dagId=" + dagIdStr);
} catch (IllegalArgumentException e) {
  Assert.assertTrue(e.getMessage().contains("Invalid DAG Id format"));
}

Answer:

#	Question
1	What test framework and assertion style does the file use (JUnit 4? `Assert.fail`/`try-catch`?)
2	Is there any test that feeds a malformed string to `TezTaskAttemptID.fromString`? (There is not.)
3	Why can't you write `@Test(expected = IllegalArgumentException.class)` against the current code?

The answer to (3): the current code does not throw — it returns null — so an expected= test would also fail. Your test must first drive the fix.

Step 6: Write the Reproducing Test (Red)

Add to TestTezIds, matching its existing style exactly:

@Test(timeout = 5000)
public void testInvalidTaskAttemptIds() {
  try {
    TezTaskAttemptID.fromString(null);
    Assert.fail("Expected IllegalArgumentException for null attempt id");
  } catch (IllegalArgumentException e) {
    // expected
  }

  String taIdStr = "attempt_1609459200000_0001_1";  // too few parts
  try {
    TezTaskAttemptID.fromString(taIdStr);
    Assert.fail("Expected failure for invalid attempt id=" + taIdStr);
  } catch (IllegalArgumentException e) {
    Assert.assertTrue(e.getMessage().contains("Invalid TaskAttemptId format"));
  }
}

Run it and watch it fail — because the current method returns null instead of throwing:

mvn test -pl tez-common -Dtest=TestTezIds#testInvalidTaskAttemptIds -q 2>&1 | tail -20

Record the actual failure (an AssertionError from your Assert.fail, since no exception was thrown). This "fails before the fix" test is what the Tez review process requires.

Step 7: Apply the Fix (Green)

Rewrite TezTaskAttemptID.fromString to validate and throw, mirroring TezDAGID's conventions. The attempt format has seven underscore-separated parts (attempt, rmId, appId, dagId, vId, taskId, id):

public static TezTaskAttemptID fromString(String taIdStr) {
  if (taIdStr == null) {
    throw new IllegalArgumentException("TaskAttemptId string cannot be null");
  }
  String[] split = taIdStr.split(String.valueOf(SEPARATOR));
  if (split.length != 7 || !taIdStr.startsWith(ATTEMPT + SEPARATOR)) {
    throw new IllegalArgumentException("Invalid TaskAttemptId format : " + taIdStr);
  }
  try {
    String rmId  = split[1];
    int appId    = Integer.parseInt(split[2]);
    int dagId    = Integer.parseInt(split[3]);
    int vId      = Integer.parseInt(split[4]);
    int taskId   = Integer.parseInt(split[5]);
    int id       = Integer.parseInt(split[6]);
    return TezTaskAttemptID.getInstance(
        TezTaskID.getInstance(
            TezVertexID.getInstance(
                TezDAGID.getInstance(rmId, appId, dagId), vId), taskId), id);
  } catch (NumberFormatException e) {
    throw new IllegalArgumentException("Error while parsing TaskAttemptId : " + taIdStr, e);
  }
}

Rules for a mergeable patch:

Do not keep the blanket catch (Exception e) { e.printStackTrace(); return null; } — deleting it is the fix. Never re-introduce printStackTrace() in library code (spotbugs/reviewers flag it).
Match TezDAGID's message wording ("Invalid ... format : ", "Error while parsing ... : ") so the package reads consistently.
Do not change the method signature, visibility, or the well-formed-input behavior — the round-trip fromString(x.toString()) must still return an equal ID. TestTezIds#testIdStringify guards that; keep it green.

Run your test again — it must now pass:

mvn test -pl tez-common -Dtest=TestTezIds -q 2>&1 | tail -20

Step 8: Verify — No Regressions, Clean Style

# Full module regression (many callers construct IDs via fromString in tests)
mvn test -pl tez-common -q 2>&1 | tail -20

# The gates precommit will run for a changed .java file
mvn spotless:apply   -pl tez-common
mvn checkstyle:check -pl tez-common
mvn apache-rat:check -pl tez-common
mvn compile spotbugs:spotbugs -Pspotbugs -pl tez-common

All must be BUILD SUCCESS. If TestTezIds#testIdStringify breaks, your parser changed the well-formed path — revisit Step 7.

Note on scope: downstream callers currently swallow the symptom (a later NPE). Your fix converts that into a clear IllegalArgumentException at the parse boundary. Do not also try to "fix" every caller in this PR — that is scope creep. If a caller genuinely needs to tolerate bad input, that is a separate JIRA.

Step 9: Confirm the Archaeology

Before you write the JIRA, prove to yourself this was never fixed:

git log --oneline --follow -- \
  tez-common/src/main/java/org/apache/tez/dag/records/TezTaskAttemptID.java | head
git blame -L /fromString/,+6 \
  tez-common/src/main/java/org/apache/tez/dag/records/TezTaskAttemptID.java

You will see the swallow-and-return-null pattern predates most of the project and survived even refactors like TEZ-4227: Introduce convenient methods in TezID subclasses. That history goes in your JIRA as evidence the bug is real and long-standing.

Step 10: Write the JIRA and Open the PR

Draft the JIRA (Minor, component Tez):

Summary: TezTaskAttemptID.fromString swallows exceptions and returns null on
         malformed input instead of throwing IllegalArgumentException

Description:
  fromString wraps parsing in catch(Exception){ e.printStackTrace(); } and
  returns null for any malformed input (null, too few parts, non-numeric
  fields). Callers such as TaskAttemptImpl and TezEventUtils dereference the
  result, producing a NullPointerException far from the real fault, plus an
  unhelpful stack trace on stderr.

  The sibling TezDAGID.fromString already validates input and throws
  IllegalArgumentException with a descriptive message; this change brings
  TezTaskAttemptID in line. (TezVertexID / TezTaskID share the same defect —
  see follow-up.)

  Test: TestTezIds#testInvalidTaskAttemptIds (fails before, passes after).

Then take it through Lab 2.2: commit TEZ-XXXX: TezTaskAttemptID.fromString should throw IllegalArgumentException on invalid input, push, open the PR against master, and respond to Yetus.

Deliverables

The investigation notes for Step 2 — proof, in your own words, that fromString returns null rather than throwing NPE, and where the real NPE surfaces.
The reproducing test testInvalidTaskAttemptIds in TestTezIds, failing on unpatched code.
The fix in TezTaskAttemptID.java that removes the swallow and throws IllegalArgumentException, with testIdStringify still green.
Clean mvn checkstyle:check, apache-rat:check, spotbugs, and full tez-common tests.
A JIRA description citing the archaeology and the TezDAGID precedent.
An opened PR titled TEZ-XXXX: ….

Troubleshooting

Symptom	Cause	Fix
Your reproducing test passes on unpatched code	You wrote `@Test(expected=...)` — but no exception is thrown	Use `Assert.fail` inside a `try`, as `testInvalidDagIds` does.
`testIdStringify` now fails	Your `split`-based parser mishandles the well-formed path	Verify all seven parts map correctly; the format is `attempt_rm_app_dag_v_task_id`.
Checkstyle `-1` on `LineLength`	A chained `getInstance(...)` exceeds 120 chars	Break the expression across lines (max is 120 in the checkstyle config).
SpotBugs flags `printStackTrace`	You left the old catch block	Remove it entirely — throwing replaces it.
Callers' tests break	They fed malformed input and relied on `null`	That reliance was the latent bug; fix the test or file a caller JIRA — note it, don't hide it.

Stretch Goals

Fix the siblings. File follow-up JIRAs and PRs for TezVertexID.fromString and TezTaskID.fromString, which share the defect. Reuse the TestTezIds pattern.
Property round-trip test. Add a test that generates random valid IDs, asserts fromString(id.toString()).equals(id), and asserts that truncating any separator throws IllegalArgumentException.
Compare with Hadoop. Read Hadoop's TaskAttemptID.forName; note how it validates and what exception it throws. Argue in one paragraph why Tez should match that contract.
Grep for the antipattern project-wide. grep -rn "e.printStackTrace()" --include=*.java tez-* | grep -v test — how many other swallow sites exist? Each is a potential Minor JIRA.

Validation / Self-check

Does TezTaskAttemptID.fromString(null) throw or return? What line is responsible, and what does the catch block do?
Where does the NullPointerException in this lab's title actually occur, and why is that worse than an immediate exception at the parse site?
Which sibling class is the model for the fix, and what exception type and message convention does it use?
Why must your reproducing test use Assert.fail inside a try rather than @Test(expected=...) against the unpatched code?
Which existing test guards the well-formed round-trip, and why must it stay green?
Why is fixing all three sibling classes in one PR the wrong move for a first contribution?

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

Open-Source Engineer & Contributor