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.
----
<strong>(Highlight)</strong>I think you are right, there has another way to
fix it:
- remove the current ledger from variable `ledgers` only
- update ledger info to ZK
- get ledgerId by variable `currentLedger`, and delete it from BK
Already edited this code, please take a look
--
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]
