Skip to content

Commit

Permalink
Addressed most review feedback on BindException fixes.
Browse files Browse the repository at this point in the history 
.classpath
- fixed error I introduced.

*RebalanceTest.java
- refactored to reduce cut&paste code
- added comments and one TODO about attempting a proper fix wrt BindExceptions later.

ServerTestUtils.java
- renamed method back to startVoldemortServer
- added 'throws BindException'

test/unit/voldemort/scheduled/StreamingSlopPusherTest.java
- added comment clarifying why this test is hard to harden against BindException issue
  • Loading branch information
@jayjwylie
jayjwylie committed Jan 26, 2013
1 parent 340ea39 commit 6e28bb7
Show file tree
Hide file tree
Showing 9 changed files with 89 additions and 128 deletions.
2 changes: 1 addition & 1 deletion .classpath
View file
Expand Up @@ -21,7 +21,7 @@
<classpathentry kind="lib" path="lib/colt-1.2.0.jar"/>
<classpathentry kind="lib" path="contrib/hadoop-store-builder/lib/commons-cli-2.0-SNAPSHOT.jar"/>
<classpathentry kind="lib" path="contrib/hadoop-store-builder/lib/commons-configuration-1.6.jar"/>
<classpathentry kind="lib" path="/home/jwylie/projects/v/voldemort-jayjwylie/contrib/hadoop-store-builder/lib/hadoop-core-1.0.4-p2.jar"/>
<classpathentry kind="lib" path="contrib/hadoop-store-builder/lib/hadoop-core-1.0.4-p2.jar"/>
<classpathentry kind="lib" path="lib/junit-4.6.jar"/>
<classpathentry kind="lib" path="lib/log4j-1.2.15.jar"/>
<classpathentry kind="lib" path="lib/jetty-6.1.18.jar"/>
Expand Down
7 changes: 7 additions & 0 deletions contrib/ec2-testing/test/voldemort/utils/Ec2RebalanceTest.java
View file
Expand Up @@ -41,6 +41,8 @@
*/
public class Ec2RebalanceTest extends AbstractRebalanceTest {

private static int NUM_KEYS;

private static final Logger logger = Logger.getLogger(Ec2RebalanceTest.class);
private static Ec2RebalanceTestConfig ec2RebalanceTestConfig;
private static List<HostNamePair> hostNamePairs;
Expand Down Expand Up @@ -68,6 +70,11 @@ public static void ec2TearDown() throws Exception {
destroyInstances(hostNames, ec2RebalanceTestConfig);
}

@Override
protected int getNumKeys() {
return NUM_KEYS;
}

@Override
protected Cluster getCurrentCluster(int nodeId) {
String hostName = nodeIdsInv.get(nodeId);
Expand Down
47 changes: 31 additions & 16 deletions test/common/voldemort/ServerTestUtils.java
View file
Expand Up @@ -674,20 +674,24 @@ public static void stopVoldemortServer(VoldemortServer server) throws IOExceptio
}

/**
* Starts a Voldemort server for testing purposes.
*
* Unless the ports passed in via cluster are guaranteed to be available,
* this method is susceptible to BindExceptions in VoldemortServer.start().
* (And, there is no good way of guaranteeing that ports will be available,
* so...)
*
* Tests that directly call this method ought to catch BindException and
* retry or otherwise mask this issue.
* The method {@link ServerTestUtils#startVoldemortCluster} should be used
* in preference to this method.}
*
* @param socketStoreFactory
* @param config
* @param cluster
* @return
*/
public static VoldemortServer startVoldemortServerInMannerThatMayResultInBindException(SocketStoreFactory socketStoreFactory,
VoldemortConfig config,
Cluster cluster) {
public static VoldemortServer startVoldemortServer(SocketStoreFactory socketStoreFactory,
VoldemortConfig config,
Cluster cluster) throws BindException {

// TODO: Some tests that use this method fail intermittently with the
// following output:
Expand All @@ -701,10 +705,19 @@ public static VoldemortServer startVoldemortServerInMannerThatMayResultInBindExc
// config, Cluster cluster) to understand how this error is possible,
// and why it only happens intermittently.
VoldemortServer server = new VoldemortServer(config, cluster);
server.start();
try {
server.start();
} catch(VoldemortException ve) {
if(ve.getCause() instanceof BindException) {
ve.printStackTrace();
throw new BindException(ve.getMessage());
} else {
throw ve;
}
}

ServerTestUtils.waitForServerStart(socketStoreFactory, server.getIdentityNode());
// wait till server start or throw exception
// wait till server starts or throw exception
return server;
}

Expand Down Expand Up @@ -758,15 +771,15 @@ protected static Cluster internalStartVoldemortCluster(int numServers,
throws IOException {
Cluster cluster = ServerTestUtils.getLocalCluster(numServers, partitionMap);
for(int i = 0; i < numServers; i++) {
voldemortServers[i] = ServerTestUtils.startVoldemortServerInMannerThatMayResultInBindException(socketStoreFactory,
ServerTestUtils.createServerConfig(useNio,
i,
TestUtils.createTempDir()
.getAbsolutePath(),
clusterFile,
storeFile,
properties),
cluster);
voldemortServers[i] = ServerTestUtils.startVoldemortServer(socketStoreFactory,
ServerTestUtils.createServerConfig(useNio,
i,
TestUtils.createTempDir()
.getAbsolutePath(),
clusterFile,
storeFile,
properties),
cluster);
}
return cluster;
}
Expand All @@ -792,6 +805,8 @@ protected static Cluster internalStartVoldemortCluster(int numServers,
* servers.
* @throws IOException
*/
// TODO: numServers is likely not needed. If this method is refactored in
// the future, then try and drop the numServers argument.
public static Cluster startVoldemortCluster(int numServers,
VoldemortServer[] voldemortServers,
int[][] partitionMap,
Expand Down
97 changes: 5 additions & 92 deletions test/long/voldemort/client/rebalance/RebalanceLongTest.java
View file
@@ -1,44 +1,23 @@
package voldemort.client.rebalance;

import java.io.IOException;
import java.util.Arrays;
import java.util.Collection;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Properties;

import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.junit.runners.Parameterized.Parameters;

import voldemort.ServerTestUtils;
import voldemort.TestUtils;
import voldemort.VoldemortException;
import voldemort.cluster.Cluster;
import voldemort.server.VoldemortConfig;
import voldemort.server.VoldemortServer;
import voldemort.store.metadata.MetadataStore.VoldemortState;

/**
* Start VoldemortServer locally using ServerTestUtils and run rebalancing
* tests.
*
* Run a version of RebalanceTests with a lot more keys.
*
*/
@RunWith(Parameterized.class)
public class RebalanceLongTest extends AbstractRebalanceTest {
public class RebalanceLongTest extends RebalanceTest {

Map<Integer, VoldemortServer> serverMap;
private final boolean useNio;
private final boolean useDonorBased;
protected static int NUM_MANY_KEYS = 10100;
private final int NUM_KEYS = 10100;

public RebalanceLongTest(boolean useNio, boolean useDonorBased) {
this.useNio = useNio;
this.useDonorBased = useDonorBased;
this.serverMap = new HashMap<Integer, VoldemortServer>();
super(useNio, useDonorBased);
}

@Parameters
Expand All @@ -49,73 +28,7 @@ public static Collection<Object[]> configs() {

@Override
protected int getNumKeys() {
return NUM_MANY_KEYS;
}

@Override
protected VoldemortState getCurrentState(int nodeId) {
VoldemortServer server = serverMap.get(nodeId);
if(server == null) {
throw new VoldemortException("Node id " + nodeId + " does not exist");
} else {
return server.getMetadataStore().getServerState();
}
return NUM_KEYS;
}

@Override
protected Cluster getCurrentCluster(int nodeId) {
VoldemortServer server = serverMap.get(nodeId);
if(server == null) {
throw new VoldemortException("Node id " + nodeId + " does not exist");
} else {
return server.getMetadataStore().getCluster();
}
}

@Override
protected Cluster startServers(Cluster cluster,
String storeXmlFile,
List<Integer> nodeToStart,
Map<String, String> configProps) throws IOException {
for(int node: nodeToStart) {
Properties properties = new Properties();
if(null != configProps) {
for(Entry<String, String> property: configProps.entrySet()) {
properties.put(property.getKey(), property.getValue());
}
}

VoldemortConfig config = ServerTestUtils.createServerConfig(useNio,
node,
TestUtils.createTempDir()
.getAbsolutePath(),
null,
storeXmlFile,
properties);

VoldemortServer server = ServerTestUtils.startVoldemortServerInMannerThatMayResultInBindException(socketStoreFactory,
config,
cluster);
serverMap.put(node, server);
}

return cluster;
}

@Override
protected void stopServer(List<Integer> nodesToStop) throws IOException {
for(int node: nodesToStop) {
try {
ServerTestUtils.stopVoldemortServer(serverMap.get(node));
} catch(VoldemortException e) {
// ignore these at stop time
}
}
serverMap = null;
}

@Override
protected boolean useDonorBased() {
return this.useDonorBased;
}
}
11 changes: 7 additions & 4 deletions test/unit/voldemort/client/rebalance/AbstractRebalanceTest.java
View file
Expand Up @@ -89,7 +89,6 @@ public abstract class AbstractRebalanceTest {

private static final Logger logger = Logger.getLogger(AbstractRebalanceTest.class.getName());

protected static int NUM_KEYS = 20;
protected static int NUM_RO_CHUNKS_PER_BUCKET = 10;
protected static String testStoreNameRW = "test";
protected static String testStoreNameRW2 = "test2";
Expand Down Expand Up @@ -221,6 +220,7 @@ public void tearDown() {
socketStoreFactory = null;
}

// TODO: Any way to not throw exception from here?
protected abstract Cluster startServers(Cluster cluster,
String StoreDefXmlFile,
List<Integer> nodeToStart,
Expand Down Expand Up @@ -263,9 +263,12 @@ public void checkConsistentMetadata(Cluster targetCluster, List<Integer> serverL
}
}

protected int getNumKeys() {
return NUM_KEYS;
}
/**
* This method determines the "size" of the test to run...
*
* @return
*/
protected abstract int getNumKeys();

@Test(timeout = 600000)
public void testRORWRebalance() throws Exception {
Expand Down
19 changes: 15 additions & 4 deletions test/unit/voldemort/client/rebalance/RebalanceTest.java
View file
Expand Up @@ -46,13 +46,16 @@
@RunWith(Parameterized.class)
public class RebalanceTest extends AbstractRebalanceTest {

Map<Integer, VoldemortServer> serverMap = new HashMap<Integer, VoldemortServer>();
private final int NUM_KEYS = 20;

Map<Integer, VoldemortServer> serverMap;
private final boolean useNio;
private final boolean useDonorBased;

public RebalanceTest(boolean useNio, boolean useDonorBased) {
this.useNio = useNio;
this.useDonorBased = useDonorBased;
this.serverMap = new HashMap<Integer, VoldemortServer>();
}

@Parameters
Expand Down Expand Up @@ -81,8 +84,16 @@ protected Cluster getCurrentCluster(int nodeId) {
}
}

// This method may be susceptible to BindException issues due to TOCTOU
// problem with getLocalCluster.
@Override
protected int getNumKeys() {
return NUM_KEYS;
}

// This method is susceptible to BindException issues due to TOCTOU
// problem with getLocalCluster (which is used to construct cluster that is
// passed in).
// TODO: Refactor AbstractRebalanceTest to take advantage of
// ServerTestUtils.startVoldemortCluster.
@Override
protected Cluster startServers(Cluster cluster,
String storeXmlFile,
Expand All @@ -104,7 +115,7 @@ protected Cluster startServers(Cluster cluster,
storeXmlFile,
properties);

VoldemortServer server = ServerTestUtils.startVoldemortServerInMannerThatMayResultInBindException(socketStoreFactory,
VoldemortServer server = ServerTestUtils.startVoldemortServer(socketStoreFactory,
config,
cluster);
serverMap.put(node, server);
Expand Down
11 changes: 7 additions & 4 deletions test/unit/voldemort/scheduled/StreamingSlopPusherTest.java
View file
Expand Up @@ -93,12 +93,15 @@ public void setUp() throws Exception {
}
}

// This method may be susceptible to BindException issues due to TOCTOU
// problem with getLocalCluster.
private void startServers(int... nodeIds) {
// This method is susceptible to BindException issues due to TOCTOU
// problem with getLocalCluster. It is not obvious how to change this set of
// unit tests to better protect against BindException risk since subsets of
// servers are started and stopped. And, since there are multiple
// invocations of startServers within one test in some cases.
private void startServers(int... nodeIds) throws IOException {
for(int nodeId: nodeIds) {
if(nodeId < NUM_SERVERS) {
servers[nodeId] = ServerTestUtils.startVoldemortServerInMannerThatMayResultInBindException(socketStoreFactory,
servers[nodeId] = ServerTestUtils.startVoldemortServer(socketStoreFactory,
configs[nodeId],
cluster);
}
Expand Down
4 changes: 2 additions & 2 deletions test/unit/voldemort/server/gossip/GossiperTest.java
View file
Expand Up @@ -91,7 +91,7 @@ private void attemptParallelClusterStart(ExecutorService executorService) {

public void run() {
try {
servers.add(ServerTestUtils.startVoldemortServerInMannerThatMayResultInBindException(socketStoreFactory,
servers.add(ServerTestUtils.startVoldemortServer(socketStoreFactory,
ServerTestUtils.createServerConfig(useNio,
j,
TestUtils.createTempDir()
Expand Down Expand Up @@ -179,7 +179,7 @@ private Cluster attemptStartAdditionalServer() throws IOException {
{ 3, 7, 11 } });

// Create a new server
VoldemortServer newServer = ServerTestUtils.startVoldemortServerInMannerThatMayResultInBindException(socketStoreFactory,
VoldemortServer newServer = ServerTestUtils.startVoldemortServer(socketStoreFactory,
ServerTestUtils.createServerConfig(useNio,
3,
TestUtils.createTempDir()
Expand Down
19 changes: 14 additions & 5 deletions test/unit/voldemort/utils/ServerTestUtilsTest.java
View file
Expand Up @@ -75,13 +75,14 @@ public void stressTestStartVoldemortCluster() throws IOException {

// @Test
public void startMultipleVoldemortServers() throws IOException {
Cluster cluster = ServerTestUtils.getLocalCluster(8, new int[][] { { 0 }, { 1 }, { 2 },
{ 3 }, { 4 }, { 5 }, { 6 }, { 7 } });
Cluster cluster = ServerTestUtils.getLocalCluster(16, new int[][] { { 0 }, { 1 }, { 2 },
{ 3 }, { 4 }, { 5 }, { 6 }, { 7 }, { 8 }, { 9 }, { 10 }, { 11 }, { 12 }, { 13 },
{ 14 }, { 15 } });

VoldemortServer[] servers = new VoldemortServer[8];
VoldemortServer[] servers = new VoldemortServer[16];

for(int i = 0; i < 8; i++) {
servers[i] = ServerTestUtils.startVoldemortServerInMannerThatMayResultInBindException(socketStoreFactory,
for(int i = 0; i < 16; i++) {
servers[i] = ServerTestUtils.startVoldemortServer(socketStoreFactory,
ServerTestUtils.createServerConfig(true,
i,
TestUtils.createTempDir()
Expand All @@ -94,6 +95,14 @@ public void startMultipleVoldemortServers() throws IOException {
assertTrue(true);
}

// @Test
public void startMultipleVoldemortServersUnsafe5() throws IOException {
for(int i = 0; i < 5; i++) {
startMultipleVoldemortServers();
}
assertTrue(true);
}

// @Test
public void startMultipleVoldemortServers10() {
for(int i = 0; i < 10; i++) {
Expand Down

0 comments on commit 6e28bb7

Please sign in to comment.