Responding to Feedback

Your patch is attached. A committer comments. What happens next is the most underrated skill in open source: turning review comments into a committed patch without burning the reviewer's patience or your own. This chapter is the playbook.

The Asynchronous Reality

Apache review is asynchronous and bursty. The committer who reviews your patch may:

• Be in a different time zone (most likely)
• Be reviewing on weekends or commute time
• Have other patches queued
• Be the only person in the world who deeply knows the file you touched

Practical consequences:

Set the expectation early — both for yourself and for reviewers — that a non-trivial patch takes 3–6 weeks from first attach to commit. Optimise for round-trip count, not round-trip duration.

Address Each Comment Explicitly

Reviewers leave per-line comments on a patch (on JIRA in older Tez, on a PR in newer). Each comment needs an explicit response. Not implicit. The committer should not have to diff your old and new patches to figure out which feedback you took.

The pattern:

Reviewer:  L243: This catches Exception too broadly. Tighten to IOException.

You (in JIRA comment when attaching .002):

  Addressed in .002:
    - L243: tightened to IOException; rethrowing wrapped TezException as before.
    - L301: added the missing null check you mentioned.
    - L427: pushed back; see explanation below.

This three-line response is more valuable than a perfect patch with no commentary. It shows you read every comment and decided about each one.

Don't Argue Without Evidence

When a committer says "this is wrong" and you disagree, the natural reflex is to defend. The Apache-effective reflex is to provide evidence.

Bad:

I don't think changing this would help.

Good:

I tried the suggested approach in a local branch. It causes TestVertexImpl#testRecover to fail because REASON. Output:

java.lang.AssertionError: expected 3 attempts, got 2
  at ...

Suggesting we keep the current approach with the additional comment you also asked for.

Three rules for pushback:

1. Always try the alternative first. Often the committer is right and you didn't see it.
2. Quote the failing test or benchmark. Numbers and stack traces close arguments.
3. Offer the smallest possible compromise. "Keep current behavior but add the comment you asked for" is much easier to accept than "no."

When to Push Back

You should push back when:

• The committer's suggestion would break a documented behavior of a @Public API.
• The committer's suggestion contradicts another committer's suggestion (cite the other).
• The committer's suggestion expands scope beyond the JIRA (offer to file a follow-up).
• You have a measurement (perf, memory) that contradicts the suggestion.

You should not push back when:

• It's a style preference and you don't strongly care. Take it; save your capital.
• It's a test-coverage ask. Add the test.
• It's a "split this into two patches" ask. Split it.
• It's "rename this method." Rename it.

The principle: defend the substance of the patch, never the shape.

When to Abandon

Most patches that get abandoned should not have been opened in the first place. But some get abandoned mid-review and that's the right call. Signals:

Abandoning is a respectable outcome. Ghosting a patch is not. If you can't continue, say so on the JIRA in one sentence:

Stepping away from this; my time has been redirected. Unassigning so someone else can pick it up. Latest patch (.003) is a good starting point but needs the test reviewer @NAME asked for.

Post a New Patch with a Clear Delta

When you upload TEZ-NNNN.002.patch, leave a JIRA comment that lists the deltas from .001:

Posted .002. Delta from .001:

- L243: tightened catch to IOException, per @<reviewer>.
- L301: added null check, per @<reviewer>.
- L427: kept current logic; rationale above.
- Added testRecoverNoInputs in TestVertexImpl.

mvn install -DskipTests, mvn checkstyle:check, mvn test -pl tez-dag all pass.

Why this matters:

• Reviewer can re-review by diffing the delta, not the full patch.
• Future readers of the JIRA see the iteration history at the JIRA level, not just in git.
• It demonstrates the patch had real iteration, not a vibes-based "I changed some stuff."

Diff your own patches locally:

diff -u TEZ-NNNN.001.patch TEZ-NNNN.002.patch | less

Thank the Reviewer

After commit, comment on the JIRA:

Thanks @COMMITTER for the review and commit. Thanks @OTHER-REVIEWERS for the feedback.

This is not perfunctory. Apache is a long game. The committer who reviewed your first patch is likely to review your tenth. They are humans investing volunteer attention.

Acknowledgement also matters at the project level — it shows other onlookers that the project's reviewers are responsive, which makes the next contributor more likely to attempt a patch.

The Shepherd Committer

For non-trivial JIRAs, especially design-heavy ones, one committer often becomes the "shepherd" — the de facto reviewer and merge-committer. The relationship:

Spotting a shepherd: after 2–3 review rounds with the same committer, they're shepherding. Direct future questions on the JIRA to them ("@COMMITTER, would you prefer A or B for the rename?"). Don't ping multiple committers in parallel; that fragments attention.

When to Ping

JIRA pings have a half-life. Use them sparingly.

What kills a patch dead: pinging weekly or daily. After two such pings, reviewers deprioritise the patch out of self-defence. Don't.

Worked Example — A Full Round-Trip

JIRA: TEZ-4321, "Fix NPE in VertexImpl.recover when no inputs."

Day 0:  You attach TEZ-4321.001.patch, set status to Patch Available.
Day 4:  Committer @gunther comments:
          L88: prefer Collections.emptyList() over new ArrayList<>()
          L92: add test for the no-inputs case
          L94: should we also handle no-outputs symmetrically?

Day 5:  You reply on JIRA:
          - L88: agreed, will fix.
          - L92: agreed, adding TestVertexImpl#testRecoverNoInputs.
          - L94: noticed but out of scope for this JIRA. Filed TEZ-4329 for follow-up.

Day 5:  You attach TEZ-4321.002.patch and a delta-summary comment.

Day 9:  @gunther comments: "+1 LGTM"

Day 10: @gunther commits as
          "TEZ-4321: Fix NPE in VertexImpl.recover when no inputs. (Jane Doe via gunther)"
        and sets status to Resolved.

Day 10: You comment:
          "Thanks @gunther. Working on TEZ-4329 next."

10 days, 2 patch rounds, 1 follow-up JIRA filed, 0 arguments. That is a healthy review.

When Feedback Comes from a Non-Committer

Non-committers can review too. Their +1 is non-binding (only committers' votes count for commit), but their feedback is often substantively excellent — they may know the area better than the committer who eventually commits.

Treat non-committer feedback exactly like committer feedback: address each comment, explain, iterate. Two non-binding +1s also signal to a committer that the patch is ready to consider, accelerating attention.

Validation Artifacts

After this chapter you should have:

1. A ~/tez-notes/in-flight.md listing any JIRA you currently have a patch on, with the date of last activity.
2. A template for the "delta from previous patch" comment, saved as ~/tez-notes/delta-template.md.
3. Internalised the four-tier ping schedule.
4. The reflex to thank the committer after merge.

The next chapter — Compatibility — is the technical knowledge you need so reviewers don't have to teach you compatibility rules during review.