poorbarcode commented on code in PR #19444:
URL: https://github.com/apache/pulsar/pull/19444#discussion_r1098365963
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -1706,19 +1710,10 @@ synchronized void ledgerClosed(final LedgerHandle lh) {
if (log.isDebugEnabled()) {
log.debug("[{}] Ledger has been closed id={} entries={}", name,
lh.getId(), entriesInLedger);
}
- if (entriesInLedger > 0) {
- LedgerInfo info =
LedgerInfo.newBuilder().setLedgerId(lh.getId()).setEntries(entriesInLedger)
-
.setSize(lh.getLength()).setTimestamp(clock.millis()).build();
- ledgers.put(lh.getId(), info);
- } else {
- // The last ledger was empty, so we can discard it
- ledgers.remove(lh.getId());
- mbean.startDataLedgerDeleteOp();
- bookKeeper.asyncDeleteLedger(lh.getId(), (rc, ctx) -> {
- mbean.endDataLedgerDeleteOp();
- log.info("[{}] Delete complete for empty ledger {}. rc={}",
name, lh.getId(), rc);
- }, null);
Review Comment:
> Why we need remove these lines?
This method is part of the ledger switch flow, the whole ledger switch flow
is like this:
1. close the current ledger (Here it is).
2. create a new ledger
3. update ledgers to ZK
If we delete `current ledger` from BK here, and update ledgers to ZK fail,
it will cause an issue: this ledger is still used but has been deleted, so we
can not delete this ledger here.
if we remove the current ledger from variable `ledgers` only, then after
updating ledger info to ZK, we can not get the ledgerId which should be deleted.
### new implementation
- remove the current ledger from variable `ledgers` only
- updating ledger info to ZK
- get ledgerId by variable `currentLedger`, and delete it from BK
--
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]
