Issue #1970: Ensure getStickyReadBookieIndex returns valid bookie index
Descriptions of the changes in this PR:
Master Issue: #1970
Related Issues: apache/pulsar#3715 apache/pulsar#4526
*Motivation*
Fixes #1970
By default bookie uses a random generator to generate a bookie index as the sticky
read bookie index. However the random generator will generate negative numbers. Hence
it will generate negative bookie indexes in write set and cause ArrayIndexOutOfBoundException
when bookkeeper attempts to read entries.
*Modifications*
Make sure getStickyReadBookieIndex not return negative number.
*Verify this change*
This problem introduced by a random generator. It is very hard to write a unit test to reproduce this issue.
Existing StickyRead tests are good to cover this code change.
Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Jia Zhai <zhaijia@apache.org>, Yong Zhang <zhangyong1025.zy@gmail.com>
This closes #2111 from sijie/fix_sticky_read, closes #1970
diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/EnsemblePlacementPolicy.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/EnsemblePlacementPolicy.java
index 64a4b91..6249ec5 100644
--- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/EnsemblePlacementPolicy.java
+++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/EnsemblePlacementPolicy.java
@@ -30,6 +30,7 @@
import org.apache.bookkeeper.client.api.LedgerMetadata;
import org.apache.bookkeeper.common.annotation.InterfaceAudience;
import org.apache.bookkeeper.common.annotation.InterfaceStability;
+import org.apache.bookkeeper.common.util.MathUtils;
import org.apache.bookkeeper.conf.ClientConfiguration;
import org.apache.bookkeeper.feature.FeatureProvider;
import org.apache.bookkeeper.net.BookieSocketAddress;
@@ -384,12 +385,12 @@
if (!currentStickyBookieIndex.isPresent()) {
// Pick one bookie randomly from the current ensemble as the initial
// "sticky bookie"
- return ThreadLocalRandom.current().nextInt() % metadata.getEnsembleSize();
+ return ThreadLocalRandom.current().nextInt(metadata.getEnsembleSize());
} else {
// When choosing a new sticky bookie index (eg: after the current
// one has read failures), by default we pick the next one in the
// ensemble, to avoid picking up the same one again.
- return (currentStickyBookieIndex.get() + 1) % metadata.getEnsembleSize();
+ return MathUtils.signSafeMod(currentStickyBookieIndex.get() + 1, metadata.getEnsembleSize());
}
}
diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java
index 7e21f97..f3abef8 100644
--- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java
+++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java
@@ -95,6 +95,8 @@
public class LedgerHandle implements WriteHandle {
static final Logger LOG = LoggerFactory.getLogger(LedgerHandle.class);
+ private static final int STICKY_READ_BOOKIE_INDEX_UNSET = -1;
+
final ClientContext clientCtx;
final byte[] ledgerKey;
@@ -199,7 +201,7 @@
&& getLedgerMetadata().getEnsembleSize() == getLedgerMetadata().getWriteQuorumSize()) {
stickyBookieIndex = clientCtx.getPlacementPolicy().getStickyReadBookieIndex(metadata, Optional.empty());
} else {
- stickyBookieIndex = -1;
+ stickyBookieIndex = STICKY_READ_BOOKIE_INDEX_UNSET;
}
if (clientCtx.getConf().throttleValue > 0) {
@@ -265,7 +267,7 @@
// If sticky bookie reads are enabled, switch the sticky bookie to the
// next bookie in the ensemble so that we avoid to keep reading from the
// same failed bookie
- if (stickyBookieIndex != -1) {
+ if (stickyBookieIndex != STICKY_READ_BOOKIE_INDEX_UNSET) {
// This will be idempotent when we have multiple read errors on the
// same bookie. The net result is that we just go to the next bookie
stickyBookieIndex = clientCtx.getPlacementPolicy().getStickyReadBookieIndex(getLedgerMetadata(),
@@ -2020,7 +2022,7 @@
* This will include all bookies that are cotna
*/
WriteSet getWriteSetForReadOperation(long entryId) {
- if (stickyBookieIndex != -1) {
+ if (stickyBookieIndex != STICKY_READ_BOOKIE_INDEX_UNSET) {
// When sticky reads are enabled we want to make sure to take
// advantage of read-ahead (or, anyway, from efficiencies in
// reading sequential data from disk through the page cache).
