-
Notifications
You must be signed in to change notification settings - Fork 105
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add cli driver for all clients #159
Conversation
ndbench-cli/build.gradle
Outdated
dependencies { | ||
compile project(':ndbench-core') | ||
compile project(':ndbench-api') | ||
compile (project(':ndbench-cass-plugins')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add the rest of the plugins in build.gradle when CLI support for them is implemented
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
* @author amcp@amazon.com | ||
*/ | ||
public class NdbenchEntrypoint { | ||
public static class Args { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pull out to a separate class or figure out how to use Archaius interface for CLI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
* - add command line arguments for all generic and client-specific parameters | ||
* - | ||
* | ||
* @author amcp@amazon.com |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name not email
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
@@ -0,0 +1,346 @@ | |||
/* | |||
* Copyright 2018 Amazon Web Services, Inc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Netflix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
}); | ||
|
||
try { | ||
driver.init(dynamodb); //TODO make database client configurable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add an argument for the driver
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not implementing other drivers now, so put this feedback off.
@@ -31,7 +31,7 @@ public RandomStringKeyGenerator(boolean preLoadKeys, int numKeys) { | |||
|
|||
@Override | |||
public String getNextKey() { | |||
int randomKeyIndex = kRandom.nextInt(getNumKeys()); | |||
int randomKeyIndex = kRandom.nextInt(numKeys); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix in another pr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -4,7 +4,6 @@ repositories { | |||
} | |||
} | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove all the noise changes in this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
@@ -252,8 +252,8 @@ public String writeSingle(String key) throws Exception { | |||
|
|||
private List<WriteRequest> generateWriteRequests(List<String> keys) { | |||
return keys.stream() | |||
.map(key -> ImmutableMap.of(partitionKeyName, new AttributeValue(key), | |||
ATTRIBUTE_NAME, new AttributeValue(this.dataGenerator.getRandomValue()))) | |||
.map(key -> ImmutableMap.of("id", new AttributeValue(key), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use the partitionKeyName field
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
@@ -15,7 +16,7 @@ | |||
private static final Logger Logger = LoggerFactory.getLogger(AutotuneTest.class); | |||
|
|||
|
|||
@Test | |||
@Ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
investigate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
settings.gradle
Outdated
@@ -1,3 +1,13 @@ | |||
rootProject.name='ndbench' | |||
include 'ndbench-web','ndbench-api','ndbench-core','ndbench-cass-plugins','ndbench-sample-plugins','ndbench-dyno-plugins','ndbench-es-plugins','ndbench-geode-plugins','ndbench-janusgraph-plugins', 'ndbench-evcache-plugins', 'ndbench-dynamodb-plugins' | |||
|
|||
include 'ndbench-web' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make this change in a different PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
536f570
to
d0216bc
Compare
evcache causes exceptions related to Eureka on startup. Any ideas? |
2914272
to
a14dc44
Compare
Also, seems like the properties are not getting initialized, despite my customizations:
Result:
|
3d24ba5
to
049dd24
Compare
@ipapapa I fixed the system property issue by modifying build.gradle. The following command now works: DISCOVERY_ENV=AWS ./gradlew run \
-Dndbench.config.cli.clientName=DynamoDBKeyValue \
-Dndbench.config.cli.bulkSize=25 \
-Dndbench.config.cli.timeoutMillis=360000 \
-Dndbench.config.dataSize=1000 \
-Dndbench.config.numKeys=100000000 \
-Dndbench.config.readEnabled=true \
-Dndbench.config.readRateLimit=3000 \
-Dndbench.config.writeEnabled=true \
-Dndbench.config.writeRateLimit=3000 \
-Dndbench.config.dynamodb.consistentread=true \
-Dndbench.config.dynamodb.maxRetries=0 \
-Dndbench.config.dynamodb.requestTimeout=100 \
-Dndbench.config.dynamodb.maxConnections=62 \
-Dndbench.config.dynamodb.numWriters=36 \
-Dndbench.config.dynamodb.numReaders=26 |
LGTM... @vinaykumarchella ? |
6bba88e
to
981260a
Compare
93ce80a
to
f6569b2
Compare
keys.add("T" + i); | ||
} | ||
logger.info("Preloaded " + numKeys + " keys"); | ||
keys.addAll(IntStream.of(getNumKeys()).boxed().map(i -> "T" + i).collect(Collectors.toList())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a peek for the info statement
Just back from short vacation, will go through it today and provide my comments on this |
import org.slf4j.LoggerFactory; | ||
|
||
/** | ||
* This class is a CLI entry point to facilitate quick testing of the Netflix Database Benchmark. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Netflix Database Benchmark
--> Netflix Data Benchmark (NdBench)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Integer.valueOf(cliConfigs.getBulkSize()) | ||
); | ||
long millisToWait = Integer.valueOf(cliConfigs.getCliTimeoutMillis()); | ||
logger.info("Waiting " + millisToWait + " ms for reads and writes to finish"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this deviate the goal of NdBench being run continuously without a preset time? Can we have default to run indefinitely and being in CLI
, you can stop with program interruption inputs (e.g., CTRL+C etc.,). also if needed we can provide the functionality to set the time, lets say - default getCliTimeoutMillis
would be 0 or -1, setting it to positive number goes through the timeout functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With programmatic tables and large amounts of IOPS, it is easy to forget to delete the table after a Control-C and incur extra cost. The timeout is an extra safeguard to control the cost of experiments. I agree about overloading the meaning of non-positive numbers. Will fix.
Overall looks good, provided minor feedback above |
Partially addresses Netflix#118
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Setup on EC2 Amazon Linux:
Test 1: