Copilot commented on code in PR #25795:
URL: https://github.com/apache/pulsar/pull/25795#discussion_r3352153916
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -1749,8 +1767,11 @@ public void operationComplete(Void v, Stat stat) {
synchronized (ManagedLedgerImpl.this) {
try {
State state =
STATE_UPDATER.get(ManagedLedgerImpl.this);
- if (state == State.Closed || state.isFenced()) {
- log.debug().log("skip ledger update after
create complete ledger is closed or fenced");
+ if (state == State.Closed || state ==
State.Terminated || state.isFenced()) {
+ log.debug().attr("state", state)
+ .log("skip ledger update after create
complete ledger is not writable");
+ // TODO: if this path is hit after the new
ledger was already written into metadata,
+ // delete the unused ledger together with
removing it from the metadata.
Review Comment:
This TODO comment doesn’t reference a GitHub issue. In this codebase, TODOs
are expected to include a link (e.g.,
https://github.com/apache/pulsar/issues/XXXX) so they’re trackable and don’t
get lost.
##########
managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerTerminationTest.java:
##########
@@ -53,6 +74,251 @@ public void terminateSimple() throws Exception {
}
}
+ @Test(timeOut = 30000)
+ public void terminateDuringLedgerSwitchKeepsTerminatedState() throws
Exception {
+ // Regression for terminate racing with a ledger rollover. The new
ledger create callback is held until after
+ // terminate wins. The late create callback must not reopen the
managed ledger, and the late-created ledger
+ // must be closed/deleted because terminate will not use a future
ledger for pending writes.
+ BookKeeper spyBookKeeper = spy(bkc);
+ ManagedLedgerConfig config = new ManagedLedgerConfig();
+ initManagedLedgerConfig(config);
+ config.setMaxEntriesPerLedger(1);
+
+ AtomicBoolean holdNextCreate = new AtomicBoolean(false);
+ CountDownLatch createRequested = new CountDownLatch(1);
+ AtomicReference<CreateCallback> createCallback = new
AtomicReference<>();
+ AtomicReference<Object> createCtx = new AtomicReference<>();
+ AtomicReference<LedgerHandle> createdLedger = new AtomicReference<>();
+
+ doAnswer(invocation -> {
+ if (holdNextCreate.compareAndSet(true, false)) {
+ LedgerHandle lh = bkc.createLedger(invocation.getArgument(0),
invocation.getArgument(1),
+ invocation.getArgument(2), invocation.getArgument(3),
invocation.getArgument(4));
+ createdLedger.set(lh);
+ createCallback.set(invocation.getArgument(5));
+ createCtx.set(invocation.getArgument(6));
+ createRequested.countDown();
+ return null;
+ }
+ return invocation.callRealMethod();
+ }).when(spyBookKeeper).asyncCreateLedger(anyInt(), anyInt(), anyInt(),
any(), any(), any(), any(), any());
+
+ ManagedLedgerFactoryImpl localFactory = new
ManagedLedgerFactoryImpl(metadataStore, spyBookKeeper);
+ try {
+ ManagedLedgerImpl ledger = (ManagedLedgerImpl)
localFactory.open("terminate_during_ledger_switch", config);
+ holdNextCreate.set(true);
+
+ Position p0 = ledger.addEntry("entry-0".getBytes());
+ assertTrue(createRequested.await(5, TimeUnit.SECONDS));
+ assertEquals(ledger.getState(),
ManagedLedgerImpl.State.CreatingLedger);
+
+ CountDownLatch addFailed = new CountDownLatch(1);
Review Comment:
The CountDownLatch name `addFailed` is misleading here since it’s used to
signal completion for both success and failure paths (both callbacks call
`countDown()`). Rename it to reflect completion to avoid confusing future
readers and test maintenance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
