This is an automated email from the ASF dual-hosted git repository.
sijie pushed a commit to branch branch-4.9
in repository https://gitbox.apache.org/repos/asf/bookkeeper.git
The following commit(s) were added to refs/heads/branch-4.9 by this push:
new a35e6c1 Issue #1970: Ensure getStickyReadBookieIndex returns valid
bookie index
a35e6c1 is described below
commit a35e6c157745a86da6e71da8b2711995a322f09b
Author: Sijie Guo <[email protected]>
AuthorDate: Thu Jun 20 20:19:53 2019 -0700
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 <[email protected]>, Jia Zhai
<[email protected]>, Yong Zhang <[email protected]>
This closes #2111 from sijie/fix_sticky_read, closes #1970
---
.../org/apache/bookkeeper/client/EnsemblePlacementPolicy.java | 5 +++--
.../src/main/java/org/apache/bookkeeper/client/LedgerHandle.java | 8 +++++---
2 files changed, 8 insertions(+), 5 deletions(-)
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 fad3f92..7bdfb3e 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.DistributionSchedule.WriteSet;
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;
@@ -378,12 +379,12 @@ public interface EnsemblePlacementPolicy {
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 @@ import org.slf4j.LoggerFactory;
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 @@ public class LedgerHandle implements WriteHandle {
&& 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 @@ public class LedgerHandle implements WriteHandle {
// 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 @@ public class LedgerHandle implements WriteHandle {
* 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).
