Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved ACK handling can report success and advance LAC despite violating fault-domain placement, inline resend callbacks can access cleared or recycled state, and the regression test misses the triggering order.
3 open issues
What changed in this PR
This PR updates the BookKeeper client’s ensemble-change handling to reconcile stale pending adds and invalidate affected acknowledgments before resending writes or advancing completion callbacks.
Changes:
- Reconcile each pending add against the target ensemble.
- Prepare the entire pending queue before resending, then explicitly advance callbacks.
- Add regression tests for stale ensembles, acknowledgment cleanup, late responses, and queued failures.
| File | Description |
|---|---|
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/PendingAddOpTest.java |
Adds ensemble reconciliation and callback-ordering regression coverage. |
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingAddOp.java |
Separates ensemble preparation from resending and removes affected successful bookies. |
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java |
Coordinates queue-wide preparation, resends, and callback advancement. |
Review effort: Balanced
Comment on lines
+201
to
+207
| for (int index : resend) { | ||
| addEntrySuccessBookies.remove(previous.get(index)); | ||
| if (!ackSet.removeBookieAndCheck(index)) { | ||
| completed = false; | ||
| } | ||
| } | ||
| return resend; |
Comment on lines
+214
to
216
| for (int index : indices) { | ||
| sendWriteRequest(ensemble, index); | ||
| } |
| assertTrue(op.completed); | ||
| } | ||
| handle.changingEnsemble = false; | ||
| Set<Integer> changed = new LinkedHashSet<>(Arrays.asList(2, 0, 1)); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
An add can capture an ensemble before it is queued and miss an ensemble-change resend pass. A later no-op change, or a change affecting only another index, does not retry its missing writes when resend indices are derived only from ledger metadata. The stranded entry blocks subsequent completion callbacks and LAC advancement.
Processing changed indices individually can also invoke completion callbacks for a write-set-external index before ACKs for other changed indices, or later pending entries, have been invalidated.
Changes
Validation
mvn -pl bookkeeper-server -am -Dexec.skip=true -Dtest=PendingAddOpTest,LedgerHandleInlineReadTest -Dsurefire.failIfNoSpecifiedTests=false -DtestRetryCount=0 -Dspotbugs.skip testAll 12 selected tests passed on upstream master (7b66b5d).
Native build commands are skipped for this Java client change. Full-cluster and SpotBugs checks have not been run locally.