You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/08/10 08:37:06 UTC
[GitHub] [pulsar] gaozhangmin opened a new pull request #11621: fix getPreviousPosition npe
gaozhangmin opened a new pull request #11621:
URL: https://github.com/apache/pulsar/pull/11621
Fixes #11619
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #11621: fix getPreviousPosition npe
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #11621:
URL: https://github.com/apache/pulsar/pull/11621#discussion_r686120797
##########
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:
LGTM.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a change in pull request #11621: fix getPreviousPosition npe
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] hangc0276 commented on a change in pull request #11621: fix getPreviousPosition npe
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on a change in pull request #11621:
URL: https://github.com/apache/pulsar/pull/11621#discussion_r686088081
##########
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:
+1, there are too expensive to copy the whole ledgers map.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] hangc0276 commented on pull request #11621: fix getPreviousPosition npe
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on pull request #11621:
URL: https://github.com/apache/pulsar/pull/11621#issuecomment-896092475
move to 2.8.2
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] hangc0276 merged pull request #11621: fix getPreviousPosition npe
Posted by GitBox <gi...@apache.org>.
hangc0276 merged pull request #11621:
URL: https://github.com/apache/pulsar/pull/11621
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a change in pull request #11621: fix getPreviousPosition npe
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #11621:
URL: https://github.com/apache/pulsar/pull/11621#discussion_r685887182
##########
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 ConcurrentNavigableMap<Long, LedgerInfo> ledgersCopied = new ConcurrentSkipListMap<>(ledgers);
Review comment:
Since `ledgersCopied` is only a local variable, it's not necessary to use a concurrent data structure.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] hangc0276 commented on pull request #11621: fix getPreviousPosition npe
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on pull request #11621:
URL: https://github.com/apache/pulsar/pull/11621#issuecomment-896422260
move back to 2.8.1
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #11621: fix getPreviousPosition npe
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #11621:
URL: https://github.com/apache/pulsar/pull/11621#discussion_r686085678
##########
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:
We may have many ledgers if users keep long-term data, copy here might be expensive, as you can see we have many places that uses the `getPreviousPosition` method, can we catch the NPE and retry the execution?
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #11621: fix getPreviousPosition npe
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #11621:
URL: https://github.com/apache/pulsar/pull/11621#discussion_r686085678
##########
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:
We may have many ledgers if users keep long-term data, copy here might be expensive here, as you can see we have many places that uses the `getPreviousPosition` method, can we catch the NPE and retry the execution?
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org