mattisonchao commented on code in PR #16420:
URL: https://github.com/apache/pulsar/pull/16420#discussion_r915530980
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -1482,18 +1482,19 @@ public synchronized void createComplete(int rc, final
LedgerHandle lh, Object ct
lastLedgerCreationFailureTimestamp = clock.millis();
} else {
log.info("[{}] Created new ledger {}", name, lh.getId());
- ledgers.put(lh.getId(),
LedgerInfo.newBuilder().setLedgerId(lh.getId()).setTimestamp(0).build());
- currentLedger = lh;
- currentLedgerEntries = 0;
- currentLedgerSize = 0;
-
+ NavigableMap<Long, LedgerInfo> ledgersTmp = new
ConcurrentSkipListMap<>(ledgers);
Review Comment:
Does the local variable need to use `Concurrent`? Maybe some immutable
container is better.
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -1482,18 +1482,19 @@ public synchronized void createComplete(int rc, final
LedgerHandle lh, Object ct
lastLedgerCreationFailureTimestamp = clock.millis();
} else {
log.info("[{}] Created new ledger {}", name, lh.getId());
- ledgers.put(lh.getId(),
LedgerInfo.newBuilder().setLedgerId(lh.getId()).setTimestamp(0).build());
- currentLedger = lh;
- currentLedgerEntries = 0;
- currentLedgerSize = 0;
-
+ NavigableMap<Long, LedgerInfo> ledgersTmp = new
ConcurrentSkipListMap<>(ledgers);
+ ledgersTmp.put(lh.getId(),
LedgerInfo.newBuilder().setLedgerId(lh.getId()).setTimestamp(0).build());
Review Comment:
Cleanup: Maybe we can avoid building `schemainfo` twice? another is line
1494.
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -1545,21 +1546,21 @@ public void operationFailed(MetaStoreException e) {
}
};
- updateLedgersListAfterRollover(cb);
+ updateLedgersListAfterRollover(cb,
getManagedLedgerInfo(ledgersTmp));
}
}
-
- private void updateLedgersListAfterRollover(MetaStoreCallback<Void>
callback) {
+ private void updateLedgersListAfterRollover(MetaStoreCallback<Void>
callback, ManagedLedgerInfo mlInfo) {
if (!metadataMutex.tryLock()) {
// Defer update for later
- scheduledExecutor.schedule(() ->
updateLedgersListAfterRollover(callback), 100, TimeUnit.MILLISECONDS);
+ scheduledExecutor.schedule(() ->
updateLedgersListAfterRollover(callback, mlInfo),
Review Comment:
I think we are not handling this condition well:
If the process runs at this line, we continue to retry later. But at the
same time, the `ledgers` is changed. However, we still use old data to update.
I'm not sure if it is a big problem. But I think we have to clarify it.
I think I have to give a change request. @lordcheng10 Could you help to
confirm it?
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -3569,6 +3570,10 @@ private ManagedLedgerInfo getManagedLedgerInfo() {
return buildManagedLedgerInfo(ledgers);
}
+ private ManagedLedgerInfo getManagedLedgerInfo(NavigableMap<Long,
LedgerInfo> ledgersTmp) {
+ return buildManagedLedgerInfo(ledgersTmp);
Review Comment:
Why do we need to create a new method?
--
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]
