This is an automated email from the ASF dual-hosted git repository.
sijie pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/bookkeeper.git
The following commit(s) were added to refs/heads/master by this push:
new df857dd Fix selectFromNetworkLocation in
RackawareEnsemblePlacementPolicyImpl
df857dd is described below
commit df857dd678810e396ba44531e4c4546e67a0eb7a
Author: Charan Reddy Guttapalem <[email protected]>
AuthorDate: Thu Dec 6 05:16:50 2018 -0800
Fix selectFromNetworkLocation in RackawareEnsemblePlacementPolicyImpl
Descriptions of the changes in this PR:
Since beginning, selectFromNetworkLocation(excludeRacks, excludeBookies,..)
method kind of ignores predicate/ensemble passed to that method
https://github.com/apache/bookkeeper/blob/branch-4.7/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java#L694.
This was kind of ok, because so far effectively this method is called from
only
one place -
https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java#L612,
which passes TruePredicate.INSTANCE and
EnsembleForReplacementWithNoConstraints.INSTANCE.
But it is not ideal to ignore those parameters in
selectFromNetworkLocation(excludeRacks, excludeBookies,..),
from future usage perspective. So passing the received predicate and
ensemble to the underlying calls.
Reviewers: Sijie Guo <[email protected]>, Samuel Just <[email protected]>
This closes #1862 from reddycharan/fixrackaware
---
.../RackawareEnsemblePlacementPolicyImpl.java | 3 +-
.../TestRackawareEnsemblePlacementPolicy.java | 100 +++++++++++++++++++++
2 files changed, 101 insertions(+), 2 deletions(-)
diff --git
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java
index 155229a..578d49e 100644
---
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java
+++
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java
@@ -722,8 +722,7 @@ public class RackawareEnsemblePlacementPolicyImpl extends
TopologyAwareEnsembleP
}
try {
- return selectRandomInternal(knownNodes, 1,
fullExclusionBookiesList, TruePredicate.INSTANCE,
- EnsembleForReplacementWithNoConstraints.INSTANCE).get(0);
+ return selectRandomInternal(knownNodes, 1,
fullExclusionBookiesList, predicate, ensemble).get(0);
} catch (BKNotEnoughBookiesException e) {
if (!fallbackToRandom) {
LOG.error(
diff --git
a/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestRackawareEnsemblePlacementPolicy.java
b/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestRackawareEnsemblePlacementPolicy.java
index d801044..e75e93f 100644
---
a/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestRackawareEnsemblePlacementPolicy.java
+++
b/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestRackawareEnsemblePlacementPolicy.java
@@ -41,6 +41,7 @@ import junit.framework.TestCase;
import org.apache.bookkeeper.client.BKException.BKNotEnoughBookiesException;
import org.apache.bookkeeper.client.BookieInfoReader.BookieInfo;
+import
org.apache.bookkeeper.client.ITopologyAwareEnsemblePlacementPolicy.Ensemble;
import
org.apache.bookkeeper.client.TopologyAwareEnsemblePlacementPolicy.BookieNode;
import
org.apache.bookkeeper.client.TopologyAwareEnsemblePlacementPolicy.EnsembleForReplacementWithNoConstraints;
import
org.apache.bookkeeper.client.TopologyAwareEnsemblePlacementPolicy.TruePredicate;
@@ -1177,6 +1178,105 @@ public class TestRackawareEnsemblePlacementPolicy
extends TestCase {
}
@Test
+ public void testSelectBookieByExcludingRacksAndBookies() throws Exception {
+ repp.uninitalize();
+
+ int minNumRacksPerWriteQuorum = 4;
+ ClientConfiguration clientConf = new ClientConfiguration(conf);
+ clientConf.setMinNumRacksPerWriteQuorum(minNumRacksPerWriteQuorum);
+ // set enforceMinNumRacksPerWriteQuorum
+ clientConf.setEnforceMinNumRacksPerWriteQuorum(true);
+ /*
+ * Durability is enforced
+ *
+ * When durability is being enforced; we must not violate the predicate
+ * even when selecting a random bookie; as durability guarantee is not
+ * best effort; correctness is implied by it
+ */
+ repp = new RackawareEnsemblePlacementPolicy(true);
+ repp.initialize(clientConf, Optional.<DNSToSwitchMapping> empty(),
timer, DISABLE_ALL,
+ NullStatsLogger.INSTANCE);
+ repp.withDefaultRack(NetworkTopology.DEFAULT_REGION_AND_RACK);
+
+ int numOfRacks = 3;
+ int numOfBookiesPerRack = 5;
+ String[] rackLocationNames = new String[numOfRacks];
+ List<BookieSocketAddress> bookieSocketAddresses = new
ArrayList<BookieSocketAddress>();
+ Map<BookieSocketAddress, String> bookieRackMap = new
HashMap<BookieSocketAddress, String>();
+ BookieSocketAddress bookieAddress;
+
+ for (int i = 0; i < numOfRacks; i++) {
+ rackLocationNames[i] = "/default-region/r" + i;
+ for (int j = 0; j < numOfBookiesPerRack; j++) {
+ int index = i * numOfBookiesPerRack + j;
+ bookieAddress = new BookieSocketAddress("128.0.0." + index,
3181);
+ StaticDNSResolver.addNodeToRack(bookieAddress.getHostName(),
rackLocationNames[i]);
+ bookieSocketAddresses.add(bookieAddress);
+ bookieRackMap.put(bookieAddress, rackLocationNames[i]);
+ }
+ }
+
+ repp.onClusterChanged(new
HashSet<BookieSocketAddress>(bookieSocketAddresses),
+ new HashSet<BookieSocketAddress>());
+
+ Set<BookieSocketAddress> excludeBookiesOfRackR0 = new
HashSet<BookieSocketAddress>();
+ for (int i = 0; i < numOfBookiesPerRack; i++) {
+ excludeBookiesOfRackR0.add(bookieSocketAddresses.get(i));
+ }
+
+ Set<Node> excludeBookieNodesOfRackR0 =
repp.convertBookiesToNodes(excludeBookiesOfRackR0);
+
+ Set<String> excludeRackR1 = new HashSet<String>();
+ excludeRackR1.add(rackLocationNames[1]);
+
+ BookieNode nodeSelected;
+ nodeSelected = repp.selectFromNetworkLocation(excludeRackR1,
excludeBookieNodesOfRackR0, TruePredicate.INSTANCE,
+ EnsembleForReplacementWithNoConstraints.INSTANCE, false);
+ assertEquals("BookieNode should be from Rack2", rackLocationNames[2],
nodeSelected.getNetworkLocation());
+
+ try {
+ /*
+ * durability is enforced, so false predicate will reject all
+ * bookies.
+ */
+ repp.selectFromNetworkLocation(excludeRackR1,
excludeBookieNodesOfRackR0, (candidate, chosenBookies) -> {
+ return false;
+ }, EnsembleForReplacementWithNoConstraints.INSTANCE, false);
+ fail("Should get not enough bookies exception since we are using
false predicate");
+ } catch (BKNotEnoughBookiesException bnebe) {
+ // this is expected
+ }
+
+ try {
+ /*
+ * Using ensemble which rejects all the nodes.
+ */
+ repp.selectFromNetworkLocation(excludeRackR1,
excludeBookieNodesOfRackR0, TruePredicate.INSTANCE,
+ new Ensemble<BookieNode>() {
+
+ @Override
+ public boolean addNode(BookieNode node) {
+ return false;
+ }
+
+ @Override
+ public List<BookieSocketAddress> toList() {
+ return null;
+ }
+
+ @Override
+ public boolean validate() {
+ return false;
+ }
+
+ }, false);
+ fail("Should get not enough bookies exception since ensemble
rejects all the nodes");
+ } catch (BKNotEnoughBookiesException bnebe) {
+ // this is expected
+ }
+ }
+
+ @Test
public void testNewEnsembleWithMultipleRacks() throws Exception {
BookieSocketAddress addr1 = new BookieSocketAddress("127.0.0.1", 3181);
BookieSocketAddress addr2 = new BookieSocketAddress("127.0.0.2", 3181);
