Lab 8.2 — Implement the Fix, Write the Test, Format the Patch

Lab type: Fix-It (real JIRA → merged PR) Estimated time: 2–6 hours

Background

You have a reproducer from Lab 8.1 that fails on master. This lab turns it into a merged Apache Tez pull request. That means more than "make the test pass": it means the minimal diff, the fails-then-passes test in the right harness, every local gate green, a PR titled TEZ-XXXX: <description>, the JIRA and PR cross-linked, and a review conversation you handle like a professional.

Tez is a JIRA + GitHub PR project. Verify the current workflow from the repository itself, not from memory:

cd ~/src/oss-repos/tez
git log --oneline -8

330fdc8f1 TEZ-4711: Normalize ASF license header (#488) (Raghav Aggarwal reviewed by Laszlo Bodor)
b6b5d42fe TEZ-4718: Modernize Jenkins and Yetus integration ... (#498) ...
c9459fe59 TEZ-4717: Modernize and Optimize Yetus Dockerfile ... (#494) ...

Every line is TEZ-XXXX: <summary> (#<PR>) (<Author> reviewed by <Reviewer>). That final commit is written by the committer when they squash-merge your PR. Your job is to produce a PR worth that line.

Why This Lab Matters for Contributors

• The reproducer proves the bug; the fix-with-test proves you fixed that bug and guarded it from regressing. Committers will not merge a fix without the test that fails without it.
• The build gates (Spotless, RAT, Checkstyle, SpotBugs) are not bureaucracy — they are the reason Tez merges cleanly across contributors. Running them locally is the difference between a green PR and a day of back-and-forth with a bot.
• Review etiquette — small diffs, honest answers, prompt turnarounds — is what turns a first-time contributor into someone whose next PR gets reviewed quickly.

Prerequisites

• Lab 8.1 complete: a repro that fails on master, master hash noted.
• A fork of github.com/apache/tez and a clone with your fork as a remote.

  cd ~/src/oss-repos/tez
  git remote -v            # origin should be apache/tez (upstream); add your fork as 'fork'
  git remote add fork git@github.com:<you>/tez.git
  

• mvn clean install -DskipTests -Dmaven.javadoc.skip=true green on master.

Step-by-Step Tasks

Step 1 — Branch

There is no rigid branch-name convention enforced by the project (the PR title carries the JIRA id), but name your branch for the issue so your own history is legible:

git checkout master && git pull origin master
git checkout -b TEZ-XXXX-short-description

Step 2 — Implement the minimum fix

Rules — internalize these, they are what reviewers enforce:

• Change only what is necessary to fix the bug.
• Do not reformat surrounding code. Spotless already governs format; a reformat buries your real change in noise.
• Do not bundle unrelated improvements. One JIRA, one PR.
• Add a comment only where it explains why the fix is correct, not what the code does.
• If the fix spans files, they still belong in one PR — but keep it as small as the bug allows.

Model to study: TEZ-4569. The fix could have been a one-line condition tweak. Instead the author extracted the tangled skip-init predicate into a named method — read it:

git show 44c4f1ec9 -- tez-dag/src/main/java/org/apache/tez/dag/app/dag/impl/VertexImpl.java

The old code inlined a four-clause boolean into assignVertexManager(). The fix pulled it into private boolean canSkipInitialization() with early returns, and added usesRootInputVertexManager(). The behavioral change is small; the readability change is what made it reviewable. That is the bar: a diff a reviewer can hold in their head.

Step 3 — Write the fails-then-passes test, in the right harness

Every Tez fix ships with a test that fails on the original code and passes on the patched code, in the harness the module already uses (chosen in Lab 8.1).

Model to study: TEZ-4699 — the fix and a brand-new TestCSVResult.java landed together:

git show bc265069f --stat
git show bc265069f -- tez-tools/analyzers/job-analyzer/src/test/java/org/apache/tez/analyzer/TestCSVResult.java

The test constructs the malicious input (a filename escaping the working dir) and asserts the new guard rejects it — the executable form of the reproducer.

Put your test in the same test class as existing tests for the class you changed (a VertexImpl fix → TestVertexImpl; a TaskAttemptImpl fix → TestTaskAttempt). Match the file's existing conventions — check them, do not guess:

grep -c "@Test(timeout"      tez-dag/src/test/java/org/apache/tez/dag/app/dag/impl/TestVertexImpl.java
grep -n  "DrainDispatcher"   tez-dag/src/test/java/org/apache/tez/dag/app/dag/impl/TestVertexImpl.java | head

Test-quality checklist (these are the exact things a reviewer flags):

• Test name states what it verifies (testRecoveryWithBroadcastInput, not testBug).
• @Test(timeout = ...) — TestVertexImpl uses timeouts on nearly every method; a hung test blocks the whole Yetus unit run (which is why the personality streams logs live to catch it).
• No Thread.sleep() — use DrainDispatcher.await() or a CountDownLatch.
• Assertions carry a message: assertEquals("vertex should be RUNNING", RUNNING, v.getState()).
• No hardcoded absolute paths or fixed ports.

Prove it fails without the fix. This is the step reviewers cannot see but always assume you did:

git stash                                            # remove the fix, keep the test
mvn test -pl <module> -Dtest=<YourTest>#<method>     # expect: FAIL (red)
git stash pop                                        # restore the fix
mvn test -pl <module> -Dtest=<YourTest>#<method>     # expect: PASS (green)

Step 4 — Run the build gates locally

Tez enforces four gates; running them locally is the whole difference between a green PR and a bot arguing with you. All are defined in the root pom.xml (see the index for the table).

Spotless — this one runs on every build whether you ask or not. spotless-check is bound to the validate phase, so a stray trailing space or a missing license header fails mvn clean install before compilation. Fix it in one command:

mvn spotless:apply          # rewrites imports (java,javax,org.apache,com,net,io), trims whitespace,
                            # ensures trailing newline, applies dev-support/spotless/license.java

Then the module build and your test:

mvn clean install -pl <module> -am -DskipTests -Dmaven.javadoc.skip=true
mvn test -pl <module> -Dtest=<YourTest>

Style and static analysis (Yetus runs these on the PR; run them now so there are no surprises):

mvn compile checkstyle:checkstyle -pl <module>     # rules: tez-build-tools/.../checkstyle/checkstyle.xml
mvn compile spotbugs:spotbugs -pl <module>         # excludes: <module>/findbugs-exclude.xml
mvn apache-rat:check                               # ASF license audit on any new files

Warning: New source files need the ASF license header. Spotless will inject the Java header from dev-support/spotless/license.java when you run spotless:apply, but confirm with mvn apache-rat:check — RAT is the independent auditor and a missing header is a hard fail.

Step 5 — Full-module test run

Yetus runs the entire unit suite at the project root (the personality forces MODULES=(.) for unit). You do not need to run everything locally, but run the whole module you touched — a fix can break a sibling test:

mvn test -pl <module> 2>&1 | tail -20

Step 6 — Commit and push

Write the commit as the committer will squash it. First line = the PR title = TEZ-XXXX: <summary>. The body explains why, and references the JIRA:

git add <the files you changed — check with git status>
git commit

TEZ-XXXX: <concise summary of the fix>

<What was wrong, in one or two sentences. Why the fix is correct.
 What the new test asserts.>

Fixes the hang/NPE/etc. described in TEZ-XXXX.

git push fork TEZ-XXXX-short-description

Step 7 — Open the PR

Open a PR from your fork's branch against apache/tez:master. The mechanics:

• Title: TEZ-XXXX: <description> — non-negotiable. The Yetus personality matches JIRA_ISSUE_RE='^TEZ-[0-9]+$', and humans triage by the id. (Read it: grep JIRA_ISSUE_RE dev-support/tez-personality.sh.)
• Body: what the bug was, how you reproduced it, what the fix does, and how the test proves it. Paste the one-command repro from Lab 8.1.
• Link the JIRA: put the PR URL in a JIRA comment and the JIRA URL in the PR body. This is the cross-link every reviewer expects; the two systems are not automatically joined.

Note: Do not attach a .patch file to the JIRA. That was the pre-2020 workflow and you will see its residue on old issues. The current process (verified from git log and the Jenkinsfile) is a GitHub PR.

Step 8 — Read the CI, respond to review

Two systems report on your PR (full detail in the index):

• GitHub Actions (.github/workflows/build.yml): matrix build across Java 21/25 and Ubuntu/macOS, mvn clean install -DskipTests. Failures here are usually compile or Spotless.
• Jenkins + Yetus (Jenkinsfile): the real precommit — it diffs your branch against origin/master, runs test-patch.sh in Docker, and posts a comment back to your PR with a per-check emoji vote (compile, javac, javadoc, checkstyle, spotbugs, unit, shellcheck, codespell). Read that comment top to bottom.

Handle review like the merged PRs did:

Push fixups to the same branch — the PR updates in place and re-triggers CI. Do not open a new PR per review round. Answer every comment, even if only to say "done in <commit-sha>".

Reading the Yetus comment. The bot posts a table, one row per check, each with a vote. A typical verdict looks like:

| vote |     subsystem     | comment                          |
|  +1  | mvninstall        | the patch passed                 |
|  +1  | compile           | the patch passed                 |
|  -1  | spotbugs          | tez-dag generated 1 new finding  |
|  +1  | checkstyle        | (reported, non-voting)           |
|  +1  | unit              | 1234 tests passed                |

A -1 on spotbugs, compile, javac, javadoc, or unit is a real block — fix it. checkstyle is reported but filtered from the vote (the Jenkinsfile passes --tests-filter=checkstyle), yet a reviewer will still ask; fix it too. If unit is -1, click through to the archived surefire-reports (the Jenkinsfile zips them as build artifacts) to find which test failed — it is frequently a sibling test your change perturbed, not yours.

Step 9 — The full pre-PR command sequence

Before you push, run the gates in the order CI will, so nothing surprises you:

mvn spotless:apply                                               # 1. format/license (validate phase)
mvn clean install -DskipTests -Dmaven.javadoc.skip=true          # 2. matches GitHub Actions
mvn test -pl <module> -Dtest=<YourTest>                          # 3. your fails-then-passes test
mvn test -pl <module>                                            # 4. the whole touched module
mvn compile checkstyle:checkstyle spotbugs:spotbugs -pl <module> # 5. Yetus's static checks
mvn apache-rat:check                                             # 6. license audit on new files

If all six are green, the Yetus comment on your PR will be too.

Step 10 — Keep the branch mergeable

While your PR is in review, master moves. If a reviewer or CI reports a conflict, rebase — do not merge master into your branch (that pollutes the squash and the history):

git fetch origin
git rebase origin/master           # replay your commit on top of current master
# resolve conflicts, then:
mvn spotless:apply && mvn clean install -pl <module> -am -DskipTests
git push --force-with-lease fork TEZ-XXXX-short-description

--force-with-lease (not --force) updates the PR branch safely — it refuses if someone else pushed in the meantime. The PR re-triggers CI automatically.

A worked review round

Reviewers on apache/tez tend to be terse and specific. A realistic exchange:

• Reviewer: "Can you add the vertexId here too? The vertexName alone isn't unique across DAG retries."
• You: amend the message, push a fixup to the same branch, reply "Added vertexId in <short hash> — good catch, it disambiguates across retries."
• Reviewer: "+1, will commit."

Note what the contributor did not do: argue, open a new PR, or leave the comment unanswered. Every comment gets a reply and a commit or a reasoned pushback. That responsiveness is what earns a fast review on your next PR — see responding to feedback.

Deliverables

• A branch with a minimal diff — only what the bug requires.
• A test in the correct harness that is red without the fix, green with it (show both).
• mvn spotless:apply run; mvn clean install -DskipTests green.
• mvn apache-rat:check, checkstyle:checkstyle, spotbugs:spotbugs clean for your change.
• The touched module's full test suite green.
• A commit whose first line is TEZ-XXXX: <summary>.
• A PR against apache/tez:master, titled TEZ-XXXX: ..., with the JIRA cross-linked.

Troubleshooting

Stretch Goals

1. Reproduce a full Yetus run locally. Install Apache Yetus rel/0.15.1 and run test-patch.sh with dev-support/tez-personality.sh against your branch diff — exactly what Jenkins does.
2. Take a merged small PR (e.g. TEZ-4308 or TEZ-4699), revert it locally, and confirm its shipped test goes red — then re-apply. You have just replayed the fix-with-test discipline end to end.
3. Write your PR body to the standard of a merged one: reproduce steps, root cause, fix, test, and the one-command verification. Compare it against a recent merged PR on github.com/apache/tez.

Validation

1. Show your test failing without the fix and passing with it. Which harness, and why that one?
2. Is your diff minimal — could a reviewer hold it in their head? What did you deliberately not change?
3. Did mvn spotless:apply + mvn clean install -DskipTests pass before you pushed?
4. Is your PR title exactly TEZ-XXXX: <summary>? Where did you cross-link the JIRA and PR?
5. When Yetus posts its comment, which checks does it vote on, and where is that defined?
6. If a reviewer says "this is too broad," what is your first move?
7. Who writes the final (Author reviewed by Reviewer) commit line, and when?

Cross-references: Lab 8.1: Reproduce an Issue, Lab 8.3: Improve Error Messages, patch quality, responding to feedback, Capstone step 8: Patch.