lhotari commented on code in PR #24722:
URL: https://github.com/apache/pulsar/pull/24722#discussion_r2342289026
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -1878,6 +1878,18 @@ protected synchronized void
updateLedgersIdsComplete(@Nullable LedgerHandle orig
}
}
+ void addEntryFailedDueToConcurrentlyModified(final LedgerHandle
currentLedger, int rc) {
+ log.error("[{}] Fencing the topic to ensure durability and
consistency(the current ledger was concurrent"
+ + " modified by a other bookie client, which is not expcted)."
Review Comment:
nit
```suggestion
log.error("[{}] Fencing the topic to ensure durability and
consistency (the current ledger was concurrent"
+ " modified by a other bookie client, which is not
expected)."
```
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpAddEntry.java:
##########
@@ -361,7 +361,16 @@ void handleAddFailure(final LedgerHandle lh) {
finalMl.getExecutor().execute(() -> {
// Force the creation of a new ledger. Doing it in a background
thread to avoid acquiring ML lock
// from a BK callback.
+ // If we received a "MetadataVersionException" or a
"LedgerFencedException", we should tell the ML that
+ // the ledger has been closed by others, and the entries count in
the ledger may is not correct. The ML
+ // will handle it.
finalMl.ledgerClosed(lh);
+ if (rc != null && (rc.intValue() ==
BKException.Code.MetadataVersionException
+ || rc.intValue() ==
BKException.Code.LedgerFencedException)) {
+ finalMl.addEntryFailedDueToConcurrentlyModified(lh, rc);
+ } else {
+ finalMl.ledgerClosed(lh);
+ }
Review Comment:
There's now 2 calls to `finalMl.ledgerClosed(lh);` it seems that the
intention has been to remove the one on line 367.
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -1878,6 +1878,18 @@ protected synchronized void
updateLedgersIdsComplete(@Nullable LedgerHandle orig
}
}
+ void addEntryFailedDueToConcurrentlyModified(final LedgerHandle
currentLedger, int rc) {
+ log.error("[{}] Fencing the topic to ensure durability and
consistency(the current ledger was concurrent"
+ + " modified by a other bookie client, which is not expcted)."
+ + " Current ledger: {}, lastAddConfirmed: {} (the value stored
may be larger), error coder: {}.",
+ name, currentLedger.getId(),
currentLedger.getLastAddConfirmed(), rc);
+ // Stop switching ledger and write topic metadata, to avoid messages
lost. The doc of
+ // LedgerHandle also mentioned this:
https://github.com/apache/bookkeeper/blob/release-4.17.2/
+ //
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java#L2047-L2048.
+ handleBadVersion(new BadVersionException("Failed opened ledger {} to
check the last add confirmed"
+ + " position when the ledger was concurrent modified."));
Review Comment:
The exception message isn't very clear. Could it be improve? It also
contains `{}` without a meaning.
--
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]
