Lab 8.3 — Improve Error Messages for Failed DAGs

Lab type: Fix-It (diagnostics quality) Estimated time: 90 min – 2 hours

Background

When a Tez DAG fails, the diagnostics string it returns is the contract between the engine and the operator staring at a failed Hive query at 3 a.m. A diagnostic like "Vertex failed" or "Container exited" tells them almost nothing; one that names the vertex, the task, the exit code, and where the logs live turns a support escalation into a self-service fix. Improving these strings is one of the highest-value, lowest-risk contributions you can make — you change strings and the information gathered around them, not control flow, so the blast radius is tiny and reviewers love the PRs.

This lab is a focused contribution type: find a vague diagnostic in the Tez AM, make it actionable, and pin the new text with a test so it cannot silently regress. Precedent is real — git log --grep="TEZ-4308" shows exactly this kind of change merged.

Why This Lab Matters for Contributors

• Tez diagnostics propagate up a chain to the client. A message that names the failing entity is greppable in a log scraper; a generic one is invisible.
• The change is contained and the test is exact, so it is the ideal shape for a first or second PR.
• It forces you to read the DAG/Vertex/TaskAttempt state machines from the failure angle — the angle that matters most when you are on call.

Prerequisites

• Lab 8.2 — you know the PR mechanics (minimal diff, Spotless, RAT, the JIRA↔PR link, Yetus).
• A Tez checkout built green on master.
• Read this book's patch quality note — a diagnostics change is the canonical "small, high-signal patch."

Where Diagnostics Get Built — the getDiagnostics Chain

Failure text is accumulated in each state machine and joined on its way up to the client. Trace it in your checkout (grep -n, never trust a line number):

flowchart LR
  TA["TaskAttemptImpl<br/>addDiagnosticInfo()"] --> T[TaskImpl.getDiagnostics]
  T --> V["VertexImpl<br/>addDiagnostic()"]
  V --> D["DAGImpl<br/>addDiagnostic()"]
  D --> S["DAGStatusBuilder.setDiagnostics()"]
  S --> C["DAGStatus.getDiagnostics()<br/>(client sees this)"]

Two real sites worth reading first — they show the pattern and its gaps:

grep -n 'addDiagnostic("Vertex failed"' \
  tez-dag/src/main/java/org/apache/tez/dag/app/dag/impl/DAGImpl.java
grep -n 'addDiagnostic("Task failed"' \
  tez-dag/src/main/java/org/apache/tez/dag/app/dag/impl/VertexImpl.java

DAGImpl.vertexFailed(...) builds:

addDiagnostic("Vertex failed"
    + ", vertexName=" + vertex.getName()
    + ", vertexId=" + vertex.getVertexId()
    + ", diagnostics=" + vertex.getDiagnostics());

VertexImpl's taskFailed(...) builds the analogous "Task failed, taskId=..., diagnostics=...". These are decent — note they already name the entity. Your job is to find ones that are not, and bring them up to this standard.

The Anatomy of a Good Diagnostic

Before you write a single string, hold it to four properties:

1. What failed — the entity, named: Vertex, Task attempt, Container.
2. Where — the id, so it is greppable: vertexName=, taskAttemptId=, containerId=.
3. Why — the actual cause: the exit code, the exception, the limit exceeded.
4. What next — where to look: the log URL, the valid range, the config to change.

A diagnostic missing any of these forces the operator to guess. Grade real Tez strings against the four properties:

The right-hand columns are your target list: any row with ✗ marks is a candidate PR. The best ones to start with are the "improvable" rows — the entity is already named, so you are adding a field (safe for log scrapers, see below), not rewriting the whole message.

Step-by-Step Tasks

Step 1 — Hunt for weak diagnostics and vague throws

Search the AM for context-free failure text. Start with TezUncheckedException messages that name no id:

grep -rn 'throw new TezUncheckedException("' tez-dag/src/main/java tez-runtime-library/src/main/java \
  | awk -F'TezUncheckedException' 'length($2) < 45'

Real candidates this surfaces (verify them yourself):

• ClusterInfo.java — throw new TezUncheckedException("ClusterInfo not initialized yet"); (no context on what asked, or why it was not ready).
• VertexImpl.java — throw new TezUncheckedException("Unexpected VertexState: " + finalState); and "Unknown data movement type: " + ... (names the value but not the vertex).
• TaskCommunicatorManager.java — several "Registering task attempt: " + ... throws.
• Edge.java — "Unexpected null task." + ..., "Unhandled tez event type: " + ....

Also hunt the diagnostic-adding sites and log lines:

grep -rn 'addDiagnostic\|LOG.error\|LOG.warn' \
  tez-dag/src/main/java/org/apache/tez/dag/app/dag/impl/ | grep -viE '\+ vertex|\+ task|Id=' | head

And the container-completion log in AMContainerImpl — the classic "exited" message that omits the exit code:

grep -n 'exited with' tez-dag/src/main/java/org/apache/tez/dag/app/rm/container/AMContainerImpl.java

Step 2 — Pick one target

Choose a single site where an operator could hit the message with a real failure and where the current text fails one of the four properties. Good candidates:

• A message that says an entity "failed" without saying why or which.
• A TezUncheckedException that names a state/value but not the DAG/vertex/task it belongs to.
• A container-exit log that omits the exit code or diagnostics.

Keep the PR small — one message, one test. Diagnostics PRs earn their high acceptance rate by being small.

A second real hunting ground: the addDiagnostic sites in DAGImpl that already carry an e.getMessage() but drop the entity id. Read the DAG-init failure path:

grep -n 'addDiagnostic(e.getMessage()\|addDiagnostic(msg\|addDiagnostic("No vertices' \
  tez-dag/src/main/java/org/apache/tez/dag/app/dag/impl/DAGImpl.java

DAGImpl has an addDiagnostic("No vertices for dag") and several addDiagnostic(e.getMessage()) calls — the latter surface an exception message with no indication of which DAG, vertex, or phase raised it. Adding the DAG name and the failing phase to those is a textbook four-property improvement.

Step 3 — Understand the context

For your chosen site, answer:

1. Which class and method emits it? (from your grep)
2. What state transition or condition triggers it? (read the surrounding transition class)
3. What information is in scope at that point — method params, fields, the event — that could be added? (a containerId, a TezVertexID, an exitStatus, a cause Throwable)

You can only add what is reachable. Read the method; list the ids already in hand.

Step 4 — Improve the message (four properties)

Worked improvement — take the container-exit log in AMContainerImpl that currently drops the exit code, and make it actionable:

-        LOG.info("Container " + container.getContainerId()
-            + " exited with diagnostics set to " + diag);
+        LOG.info("Container " + container.getContainerId()
+            + " for vertex(es) " + container.getContainerId()  // whatever id is in scope
+            + " exited with exit status " + event.getExitStatus()
+            + " and diagnostics set to " + diag);

Or the classic bad→good, generalized:

// Before (fails "why" and "what next"):
diagnostics.add("Container " + containerId + " failed");

// After (all four properties):
diagnostics.add("Container " + containerId + " failed"
    + ", exitStatus=" + exitStatus
    + ", vertex=" + vertexName
    + ". See task logs for details.");

Note: Match the existing format in the class. DAGImpl/VertexImpl use ", key=" + value concatenation ("Vertex failed, vertexName=..., vertexId=..."). Follow that style so the diff is small and the log stays consistent with its siblings — do not invent a new String.format template in a file that uses concatenation everywhere else.

Guard any field you add against null the way the codebase already does. Read TaskAttemptImpl.addDiagnosticInfo(String diag) — it only records non-null, non-empty strings:

private void addDiagnosticInfo(String diag) {
  if (diag != null && !diag.equals("")) {
    diagnostics.add(diag);
  }
}

If the value you want to append can be null (a container id before assignment, a cause with no message), null-check it or SpotBugs and a reviewer will both flag a possible NullPointerException in the concatenation. The whole point of a diagnostic is to survive a failure — it must not throw on its way to being reported.

Step 5 — Write a test that pins the message

The message is now part of the contract, so a test must assert it. Assert the load-bearing substrings (the id, the exit code, the cause) — never the whole string, which makes the test brittle and invites reviewers to ask you to relax it.

Put the test in the state machine's existing test class (TestVertexImpl, TestTaskAttempt, or the relevant one). Follow that file's conventions — @Test(timeout = ...), DrainDispatcher, no Thread.sleep (see Lab 8.2):

@Test(timeout = 5000)
public void testFailureDiagnosticsIncludeExitStatus() {
  // ... drive the state machine to the failure via the harness ...
  List<String> diags = vertex.getDiagnostics();   // or taskAttempt.getDiagnostics()
  assertTrue("diagnostics should name the container",
      diags.stream().anyMatch(d -> d.contains("Container container_")));
  assertTrue("diagnostics should include the exit status",
      diags.stream().anyMatch(d -> d.contains("exitStatus=")));
}

Prove it fails without the fix:

git stash                                                  # remove the message change
mvn test -pl tez-dag -Dtest=TestVertexImpl#testFailureDiagnosticsIncludeExitStatus  # FAIL
git stash pop
mvn test -pl tez-dag -Dtest=TestVertexImpl#testFailureDiagnosticsIncludeExitStatus  # PASS

Step 6 — Gates, commit, PR

Exactly as Lab 8.2:

mvn spotless:apply
mvn clean install -pl tez-dag -am -DskipTests -Dmaven.javadoc.skip=true
mvn test -pl tez-dag -Dtest=TestVertexImpl
mvn apache-rat:check
git commit -m "TEZ-XXXX: Include exit status in container-failure diagnostics"
git push fork TEZ-XXXX-diag

PR title: TEZ-XXXX: <specific failure scenario> diagnostics include <the new info>. Cross-link the JIRA. In the body, quote the before and after message so the reviewer sees the improvement in two lines.

Step 7 — Confirm it reaches the client

A diagnostic is only useful if it propagates to what the operator actually sees. Follow your added text up the chain (the diagram at the top of this lab) and confirm it survives the join into DAGStatus. DAGImpl.getDAGStatus(...) ends with status.setDiagnostics(diagnostics), and DAGStatus.getDiagnostics() (module tez-api) returns that list to the client. A vertex- or task-level string only surfaces to the client if a higher layer folds it in — VertexImpl folds task diagnostics into "Task failed, ..., diagnostics=", and DAGImpl.vertexFailed(...) folds vertex diagnostics into "Vertex failed, ..., diagnostics=". If your improved message lives on TaskAttemptImpl but nothing above it includes it in a failure path, the operator never sees it — that gap is itself a worthwhile fix, and a MiniTezCluster test asserting through DAGStatus.getDiagnostics() proves the whole chain.

grep -n 'setDiagnostics\|getDiagnostics' \
  tez-dag/src/main/java/org/apache/tez/dag/app/dag/impl/DAGImpl.java \
  tez-api/src/main/java/org/apache/tez/dag/api/client/DAGStatus.java

How Maintainers Evaluate Error-Message Churn

Diagnostics changes are welcome but not free — reviewers weigh two real concerns. Anticipate them in your PR:

• Log-scraper compatibility. Downstream tools (Hive, Ambari, log dashboards) grep these strings. Adding fields to a message is safe; reordering or removing existing tokens can break a scraper that matched on them. Prefer append-only changes, and say in the PR that you preserved the existing tokens.
• Churn vs. value. A reviewer will not merge a reword that adds no information. Every change must add one of the four properties. "I made it prettier" is not a reason; "I added the exit status an operator needs to diagnose the failure" is.
• No internal leakage. Do not dump internal class names or full stack frames into operator-facing diagnostics — those belong in the AM logs, not the DAGStatus a client parses. TEZ-4336 (git show 6863a2d99) is a good counter-model to study: it deliberately threaded the original cause Throwable into the shuffle failure so the real exception surfaces — that is adding signal, not noise.

Tip: TEZ-4308 (git show 5eeccf0e3) is the smallest possible instance of this lab: it added a space and brackets to a shuffle error so progress!failureCounts= became progress: [failureCounts=. One file, six lines, merged. Read it — that is a legitimate first PR.

Deliverables

• One located vague diagnostic with the current text captured.
• A rewritten message satisfying the four properties (what / where / why / what-next), in the class's existing format.
• A unit test asserting the load-bearing substrings — red without the fix, green with it.
• Gates green: spotless:apply, clean install -DskipTests, module tests, apache-rat:check.
• A PR titled TEZ-XXXX: ... with before/after quoted, JIRA cross-linked.

Troubleshooting

Stretch Goals

1. Thread a cause through, TEZ-4336-style: find a diagnostic that discards the underlying Throwable and carry it into the reported exception (new IOException(msg, cause)), with a test asserting the cause survives.
2. Improve the DAGStatus client-facing view: confirm your added text actually reaches DAGStatus.getDiagnostics() by asserting through the full chain in a MiniTezCluster test.
3. Audit ClusterInfo, Edge, and TaskCommunicatorManager for the vaguest TezUncheckedException messages and file a JIRA proposing a batch of small, append-only improvements (one PR each).

Validation

1. Quote the message before and after. Which of the four properties did it gain?
2. Does your test pin the load-bearing substrings, not the whole string? Why does that matter?
3. Is the test red without the fix and green with it? Show both.
4. Which layer of the getDiagnostics chain did you change, and does the new text reach DAGStatus?
5. Is your change append-only with respect to existing tokens? Why does a log scraper care?
6. Did you keep internal class/stack detail out of operator-facing diagnostics?
7. Find a real TEZ JIRA whose only change was a log/diagnostic message (start with TEZ-4308). Was it accepted, and how large was the diff?

Cross-references: Lab 8.2: Implement the Fix, Lab 8.1: Reproduce an Issue, patch quality, JIRA review, Capstone overview.