sijie commented on a change in pull request #7506:
URL: https://github.com/apache/pulsar/pull/7506#discussion_r453106816
##########
File path:
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerFactoryImpl.java
##########
@@ -320,18 +335,32 @@ public void asyncOpen(final String name, final
ManagedLedgerConfig config, final
// If the ledger state is bad, remove it from the map.
CompletableFuture<ManagedLedgerImpl> existingFuture =
ledgers.get(name);
- if (existingFuture != null && existingFuture.isDone()) {
- try {
- ManagedLedgerImpl l = existingFuture.get();
- if (l.getState().equals(State.Fenced.toString()) ||
l.getState().equals(State.Closed.toString())) {
- // Managed ledger is in unusable state. Recreate it.
- log.warn("[{}] Attempted to open ledger in {} state.
Removing from the map to recreate it", name,
+ if (existingFuture != null) {
+ if (existingFuture.isDone()) {
+ try {
+ ManagedLedgerImpl l = existingFuture.get();
+ if (l.getState().equals(State.Fenced.toString()) ||
l.getState().equals(State.Closed.toString())) {
+ // Managed ledger is in unusable state. Recreate it.
+ log.warn("[{}] Attempted to open ledger in {} state.
Removing from the map to recreate it", name,
l.getState());
- ledgers.remove(name, existingFuture);
+ ledgers.remove(name, existingFuture);
+ }
+ } catch (Exception e) {
+ // Unable to get the future
+ log.warn("[{}] Got exception while trying to retrieve
ledger", name, e);
}
- } catch (Exception e) {
- // Unable to get the future
- log.warn("[{}] Got exception while trying to retrieve ledger",
name, e);
+ } else {
+ PendingInitializeManagedLedger pendingLedger =
pendingInitializeLedgers.get(name);
Review comment:
We can do that. However, I didn't go down that route because of the
following reason:
we need to keep the ManageLedger reference and to close it to release
resources. Because the initialization involves a long pipeline including
opening managed ledger and cursors. The behavior we observed is that the
operation is stuck at opening cursors. So if we don't attempt to close the
ManagedLedger instance, it can result in resource leaking.
With that being said, we need to keep a reference to ManagedLedgerImpl along
with the CompletableFuture. Hence I chose the current implementation.
Another reason is to allow checking the timestamp proactively. I fixed a
couple of issues before that NPE is thrown between creating a Future and
registering error handling logic. I would like to have a mechanism in place
that can fix any potential bugs by proactively checking if a CompletableFuture
timed out.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
