BewareMyPower commented on a change in pull request #11621:
URL: https://github.com/apache/pulsar/pull/11621#discussion_r686102144
##########
File path:
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -3202,8 +3203,9 @@ public PositionImpl getPreviousPosition(PositionImpl
position) {
return PositionImpl.get(position.getLedgerId(),
position.getEntryId() - 1);
}
+ final NavigableMap<Long, LedgerInfo> ledgersCopied = new
TreeMap<>(ledgers);
// The previous position will be the last position of an earlier
ledgers
- NavigableMap<Long, LedgerInfo> headMap =
ledgers.headMap(position.getLedgerId(), false);
+ NavigableMap<Long, LedgerInfo> headMap =
ledgersCopied.headMap(position.getLedgerId(), false);
Review comment:
The point here is even if we catch the NPE, there's still a chance that
`ledgers` might be modified during the iteration.
A better way to avoid coping the whole map might be adding all null checks
for each access, here's my suggested change:
```diff
diff --git
a/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
b/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
index 5ea1346ca89..77b3984f614 100644
---
a/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
+++
b/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
@@ -3205,7 +3205,8 @@ public class ManagedLedgerImpl implements
ManagedLedger, CreateCallback {
// The previous position will be the last position of an earlier
ledgers
NavigableMap<Long, LedgerInfo> headMap =
ledgers.headMap(position.getLedgerId(), false);
- if (headMap.isEmpty()) {
+ final Map.Entry<Long, LedgerInfo> firstEntry = headMap.firstEntry();
+ if (firstEntry == null) {
// There is no previous ledger, return an invalid position in
the current ledger
return PositionImpl.get(position.getLedgerId(), -1);
}
@@ -3213,13 +3214,13 @@ public class ManagedLedgerImpl implements
ManagedLedger, CreateCallback {
// We need to find the most recent non-empty ledger
for (long ledgerId : headMap.descendingKeySet()) {
LedgerInfo li = headMap.get(ledgerId);
- if (li.getEntries() > 0) {
+ if (li != null && li.getEntries() > 0) {
return PositionImpl.get(li.getLedgerId(), li.getEntries() -
1);
}
}
// in case there are only empty ledgers, we return a position in
the first one
- return PositionImpl.get(headMap.firstEntry().getKey(), -1);
+ return PositionImpl.get(firstEntry.getKey(), -1);
}
/**
```
What do you think? @codelipenghui @hangc0276 @gaozhangmin
--
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: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
