Responding to Maintainer Feedback

You opened a clean, scoped, tested PR (see PR Quality). A maintainer reviewed it and left comments. How you respond over the next few days decides whether this PR merges in one more round or drags through five — and decides whether this maintainer wants to review your next PR. Review response is a skill, and it is one of the clearest signals maintainers use when forming an opinion of a contributor (see Maintainership).

This chapter is the mechanics and the etiquette of iterating on an OpenSearch PR. It pairs with Level 2 Lab 4: Review a PR — having reviewed someone else's PR makes you far better at receiving review on your own.

First: Read the Whole Review Before Touching Code

When a review lands, resist the reflex to start changing things. Read every comment first, then sort them:

GitHub's review states matter: "Comment" is advisory, "Approve" clears the path, and "Request changes" (often shown as "Changes requested") is a hard block — the PR will not merge until the reviewer who requested changes re-reviews and clears it. Treat "Changes requested" as a gate, not an insult.

The OpenSearch Iterate Cadence: Fixups, Then Squash, Force-Push

This is where OpenSearch differs sharply from Apache patch culture. In Apache projects you re-roll a numbered patch file (v1.patch, v2.patch). In OpenSearch you push to the same PR branch, and force-pushing that branch is normal and expected.

The recommended cadence during active review:

1. Make a fixup commit per addressed comment, so the reviewer can see exactly what changed since their last look:

   git commit -s --fixup=HEAD          # or --fixup=<sha> targeting a specific commit
   git push                            # adds a visible commit; no force needed yet
   

   Reviewers can use GitHub's "changes since last review" view against your new commits.
2. When the reviewer is satisfied, squash the fixups into clean logical commits and force-push:

   git rebase -i --autosquash main     # folds fixup! commits into their targets
   git push --force-with-lease          # rewrites the PR branch — normal here
   

   Always use --force-with-lease (not bare --force) so you do not clobber commits you didn't know about.

Note: git push --force-with-lease to your own PR branch is routine and welcomed in OpenSearch. It keeps history clean. The thing to avoid is force-pushing in a way that loses a reviewer's place mid-review without warning — hence the fixup-then-squash rhythm: fixups while review is active, squash when it's settling.

After any push that should be re-reviewed, re-request review explicitly (the circular-arrows icon next to the reviewer, or a short comment) — maintainers do not watch every branch for pushes.

Addressing vs. Discussing: Resolve Threads Honestly

Every review comment is a conversation thread. Close the loop on each one:

• If you made the change, reply "Done" (and link the commit if helpful), then let the reviewer resolve the thread — or resolve it yourself only when the project's convention allows.
• If you chose not to, do not silently ignore it. Reply with your reasoning. Silence reads as either "I missed it" or "I'm ignoring you," and both cost you.
• Do not resolve a thread you disagreed with by just hiding it. Resolving a thread signals "addressed"; resolving one you actually pushed back on, without agreement, looks evasive.

A reviewer scanning your PR should be able to see, thread by thread, that every comment was either fixed or answered. That completeness is what lets them approve without re-reading the whole diff.

When and How To Push Back — With Evidence

You will sometimes be right and the reviewer wrong, or the reviewer's suggestion will have a cost they didn't see. Pushing back is legitimate; arguing is not. The difference is evidence.

Good push-back is specific, evidence-backed, and offers a path forward:

"I looked at gating this on Version.V_2_18_0 as suggested, but the field is also written to the qa/ rolling-upgrade snapshot, and a 2.17 reader would then see an unknown field. The BWC test …RollingUpgradeIT fails with that gating (log attached). Gating on V_3_0_0 keeps the mixed-cluster test green. Would that work, or is there a reason to target 2.18?"

That comment cites the test, shows the failure, proposes an alternative, and ends with a question. It invites agreement instead of demanding it.

Bad push-back — even when you're correct — looks like:

"That's wrong, the version gating is fine as is."

It asserts, cites nothing, and leaves the reviewer no path but to dig in. You may win the technical point and still lose the relationship.

When the disagreement is genuine and unresolved, escalate the idea, not the tone: ask the maintainer to loop in another reviewer for the area (check MAINTAINERS.md/CODEOWNERS), or move the design question to the issue/RFC. Never let a PR thread turn into a multi-screen argument — that is where PRs go to die.

Handling "Changes Requested" and Re-Requesting Review

The workflow when a reviewer formally requests changes:

flowchart LR
    A[Review: Changes requested] --> B[Read all comments, bucket them]
    B --> C[Push fixup commits<br/>+ reply per thread]
    C --> D[Keep CI green]
    D --> E[Re-request review]
    E --> F{Reviewer satisfied?}
    F -- No --> B
    F -- Yes --> G[Squash fixups,<br/>force-push clean history]
    G --> H[Approve + merge]

The two things contributors most often forget: re-requesting review (the reviewer is not notified by a push alone) and keeping CI green between rounds.

Keep CI Green Between Rounds

Every push reruns CI (gradle-check). A reviewer will not invest in a PR with red CI, because the red might be your change. Before each push:

cd ~/os-src
./gradlew spotlessApply
./gradlew precommit
./gradlew :server:test --tests "org.opensearch.your.ChangedTests"

If CI fails on a flaky test unrelated to your change, say so explicitly with the failing test name and a link if it's a known flaky-test — do not just push again silently and hope. If main moved and you have conflicts, rebase and force-push:

git fetch origin
git rebase origin/main
git push --force-with-lease

A PR that is green, rebased, and has every thread answered is one a maintainer can approve in five minutes. That is the goal of every round.

After Merge: The Backport Workflow

Your PR merged into main. If it carries a backport <branch> label (e.g. backport 2.x), a backport bot automatically opens a backport PR against that release branch. Your job is not done:

1. Watch for the bot's backport PR (it @-mentions you).
2. If the cherry-pick applied cleanly, just make sure its CI is green.
3. If the bot reports a conflict, it cannot auto-backport — you cherry-pick by hand:

   git fetch origin
   git checkout -b backport/2.x/my-fix origin/2.x
   git cherry-pick -x <merge-or-commit-sha>   # -x records the source commit
   # resolve conflicts, keep the change minimal and version-appropriate
   git push origin backport/2.x/my-fix
   # open a PR against 2.x, link the original PR
   

4. The backport still needs the same quality bar: DCO sign-off, a CHANGELOG entry under the 2.x [Unreleased] section, and green CI. See PR Quality and compatibility — a backport must be safe on the older line, which can differ from main.

A Worked Review Thread: Good vs. Bad Responses

Reviewer comment on your PR:

"This new field in FooRequest.writeTo isn't Version-gated. Won't a 2.x node fail to deserialize a request from a 3.0 node during a rolling upgrade?"

Bad response (do not do this):

"It works in my tests."

It dismisses a real compatibility concern, cites nothing, and ignores that the maintainer's worry is the mixed-cluster path, which your single-version tests never exercised.

Good response:

"Good catch — you're right, the single-version FooRequestTests round-trip doesn't cover the mixed-version case. I've gated the field write/read on Version.V_3_0_0 (commit abc1234) and added a …BwcSerializationTests case asserting a 2.x stream omits it. I also added a rolling-upgrade case under qa/ (commit def5678). Both green in CI. Re-requesting review — does the gate version look right to you, or should it target the next 2.x minor instead?"

It concedes the valid point, names exactly what changed and where, points to the new tests that prove the fix, keeps CI green, and ends with a question that invites approval. This is the response pattern that turns reviewers into advocates.

Warning: Compatibility comments like the one above are the most common blocking feedback on OpenSearch PRs. If you do not yet know why a non-gated field breaks a rolling upgrade, read Compatibility, Stability, and Performance and the serialization-BWC deep dive before you respond — a confident wrong answer here is worse than asking.

Validation: Prove You Understand This

1. Given a review with five comments of different types, bucket each (clear fix / style / question / disagreement / blocking) and state your response to each.
2. Show the exact commands for the fixup-then-squash cadence, including --autosquash and --force-with-lease, and explain why force-pushing your PR branch is normal here.
3. Write an evidence-backed push-back to a reviewer suggestion you believe is wrong, citing a test or measurement and ending with a question.
4. Describe the steps after a backport 2.x bot reports a conflict, including cherry-pick -x and the 2.x CHANGELOG entry.
5. Rewrite a bad ("it works on my machine") response to a rolling-upgrade concern into a good one that names the fix, the proving test, and re-requests review.

When your review rounds are short, your threads are all answered, and your CI is always green, maintainers start trusting your PRs by default. The next chapter — Compatibility, Stability, and Performance — is the knowledge behind the most common blocking feedback you will receive.