-
Notifications
You must be signed in to change notification settings - Fork 70
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
[to remove] Java benchmarks: add Jedis
and Lettuce
clients (sync and async)
#540
Conversation
Signed-off-by: acarbonetto <[email protected]>
Signed-off-by: acarbonetto <[email protected]>
Signed-off-by: acarbonetto <[email protected]>
This reverts commit d9b26a6.
Signed-off-by: acarbonetto <[email protected]>
* Add a java app to run benchmarks --------- Signed-off-by: acarbonetto <[email protected]>
Signed-off-by: acarbonetto <[email protected]>
Signed-off-by: acarbonetto <[email protected]>
* Merge Pull Request #5 - Add java pipeline. Also changed: * Merged two projects. * Updated CI. * Fixed tests and updated `junit` version. * Spotless. * Add new gradle tasks. Signed-off-by: Yury-Fridlyand <[email protected]>
* Add sync and async clients both to tests. Signed-off-by: Yury-Fridlyand <[email protected]> * Minor fixes. Signed-off-by: Yury-Fridlyand <[email protected]> --------- Signed-off-by: Yury-Fridlyand <[email protected]>
…onvert to TypeScript. (valkey-io#456) removed duplicated logic and refactored to typescript Signed-off-by: acarbonetto <[email protected]>
* Add Jedis and Lettuce benchmarks * Start ignoring .gradle files * Update gitignore and remove generated files from git Signed-off-by: acarbonetto <[email protected]> * Update gitignore and remove generated files from git Signed-off-by: acarbonetto <[email protected]> * Update gitignore and remove generated files from git Signed-off-by: acarbonetto <[email protected]> * Add benchmarks for GET non-existing * Revert "Update gitignore and remove generated files from git" This reverts commit d9b26a6. * fix redis-rs submodules Signed-off-by: acarbonetto <[email protected]> * Randomize commands in Java benchmarks * rename chooseAction to randomAction * Add a Java benchmarking app (#7) * Add a java app to run benchmarks --------- Signed-off-by: acarbonetto <[email protected]> * Add Readme and update install_and_test script to runJava Signed-off-by: acarbonetto <[email protected]> * Add Readme and update install_and_test script to runJava Signed-off-by: acarbonetto <[email protected]> * Combine java pipeline and java benchmarks (#8) * Merge Pull Request #5 - Add java pipeline. Also changed: * Merged two projects. * Updated CI. * Fixed tests and updated `junit` version. * Spotless. * Add new gradle tasks. Signed-off-by: Yury-Fridlyand <[email protected]> * Add sync and async clients both to tests. (#12) * Add sync and async clients both to tests. Signed-off-by: Yury-Fridlyand <[email protected]> * Minor fixes. Signed-off-by: Yury-Fridlyand <[email protected]> --------- Signed-off-by: Yury-Fridlyand <[email protected]> * Add dataSize option to java benchmark. Signed-off-by: Yury-Fridlyand <[email protected]> --------- Signed-off-by: acarbonetto <[email protected]> Signed-off-by: Yury-Fridlyand <[email protected]> Co-authored-by: Jonathan Louie <[email protected]> Co-authored-by: acarbonetto <[email protected]> Co-authored-by: jonathanl-bq <[email protected]>
Signed-off-by: acarbonetto <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
* Add option to run tests on multiple clients in concurrency * Common pool of iterations. * Awaiting result from async methods. Signed-off-by: Yury-Fridlyand <[email protected]> * minor fix Signed-off-by: Yury-Fridlyand <[email protected]> * Change while-loop; Spotless Apply Signed-off-by: acarbonetto <[email protected]> --------- Signed-off-by: Yury-Fridlyand <[email protected]> Signed-off-by: acarbonetto <[email protected]> Co-authored-by: acarbonetto <[email protected]>
Signed-off-by: acarbonetto <[email protected]>
Signed-off-by: acarbonetto <[email protected]>
* Updated ClientCount to client_count for uniformity for rust.
* Add JSON reporting. Signed-off-by: Yury-Fridlyand <[email protected]> * Fix for #26. Signed-off-by: Yury-Fridlyand <[email protected]> * Update java/benchmarks/src/main/java/javababushka/benchmarks/utils/Benchmarking.java Signed-off-by: Yury-Fridlyand <[email protected]> Co-authored-by: Andrew Carbonetto <[email protected]> * Use `Optional`. Signed-off-by: Yury-Fridlyand <[email protected]> * Address PR feedback. Signed-off-by: Yury-Fridlyand <[email protected]> --------- Signed-off-by: Yury-Fridlyand <[email protected]> Co-authored-by: Andrew Carbonetto <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
…java_benchmarks Signed-off-by: Yury-Fridlyand <[email protected]>
benchmarks/install_and_test.sh
Outdated
function runJavaBenchmark(){ | ||
cd ${BENCH_FOLDER}/../java | ||
echo "./gradlew run --args=\"--resultsFile=${BENCH_FOLDER}/$1 --clients $chosenClients --host $host --port $port\"" | ||
# ./gradlew run --args="--resultsFile=../$1 --dataSize $2 --concurrentTasks $concurrentTasks --clients $chosenClients --host $host --port $port --clientCount $clientCount $tlsFlag" |
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.
Why is it commented out?
java/benchmarks/src/main/java/javababushka/benchmarks/BenchmarkingApp.java
Outdated
Show resolved
Hide resolved
java/benchmarks/src/main/java/javababushka/benchmarks/AsyncClient.java
Outdated
Show resolved
Hide resolved
java/benchmarks/src/main/java/javababushka/benchmarks/BenchmarkingApp.java
Show resolved
Hide resolved
try { | ||
lettuceClient.waitForResult(setResult); | ||
} catch (Exception e) { | ||
fail("Can SET redis result without Exception"); |
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.
I don't understand the exception message, should it be "can't"?
try { | ||
actions.get(action).go(); | ||
} catch (Exception e) { | ||
// timed out - exception from Future::get |
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.
We don't want to measure performance with exceptions, we want to fail the benchmark on errors. Do you have timeout issues with one/some of the clients?
} | ||
|
||
@Override | ||
public void connectToRedis(ConnectionSettings connectionSettings) { |
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.
So we currently have benchmarks only for a standalone redis. Are you working on expanding it to cluster clients as well?
tasks.add( | ||
() -> { | ||
int iterationIncrement = iterationCounter.getAndIncrement(); | ||
int clientIndex = iterationIncrement % clients.size(); |
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.
It's different than what we have in the python and node benchmarks - here you attach a client to be used in the entire task, and in python/node the client is being chosen in each command.
In a case where concurrent tasks < number of clients, there will be clients that won't get any commands to run.
public static Map<ChosenAction, Operation> getActionMap( | ||
Client client, int dataSize, boolean async) { | ||
|
||
String value = RandomStringUtils.randomAlphanumeric(dataSize); |
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.
To match python/node benchmarks please change the value to
"0".repeat(dataSize);
hi, this is a pretty huge PR. If it can't be split into separate PRs, can you at least try to split it into coherent commits, so that each commit could be reviewed relatively separately? ATM you have a lot of tiny commits, with some reverts, and it's hard to understand in what order the changes should be reviewed. |
Signed-off-by: acarbonetto <[email protected]>
Hey @Yury-Fridlyand, once you're done - please tag us in a comment so we'll know it's ready for another round. |
* Add lettuce cluster client when cluster mode enabled --------- Signed-off-by: Andrew Carbonetto <[email protected]>
Signed-off-by: acarbonetto <[email protected]>
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.
comments on the non-Java code.
@@ -1,42 +0,0 @@ | |||
{ |
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.
please revert all of these deletions.
@@ -1,37 +0,0 @@ | |||
{ |
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.
same
@@ -1,41 +0,0 @@ | |||
{ |
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.
same
@@ -226,7 +226,7 @@ int num_of_concurrent_tasks | |||
{"num_of_tasks", num_of_concurrent_tasks}, | |||
{"data_size", data_size}, | |||
{"tps", tps}, | |||
{"clientCount", clients.Length}, | |||
{"client_count", clients.Length}, |
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.
please try to keep PRs concise, and fix a single issue per-PR (or at least touch a single area of code). This and changes to non-java benchmarks, isn't relevant to this PR.
@@ -167,7 +167,7 @@ async function run_clients( | |||
num_of_tasks: num_of_concurrent_tasks, | |||
data_size, | |||
tps, | |||
clientCount: clients.length, |
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.
same
@@ -226,7 +226,7 @@ async def run_clients( | |||
"num_of_tasks": num_of_concurrent_tasks, | |||
"data_size": data_size, | |||
"tps": tps, | |||
"clientCount": len(clients), |
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.
same
@@ -130,7 +130,7 @@ async fn perform_benchmark(args: Args) { | |||
Value::Number((number_of_operations as i64 * 1000 / stopwatch.elapsed_ms()).into()), | |||
); | |||
results_json.insert( | |||
"clientCount".to_string(), |
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.
same
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.
initial java comments.
iterationIncrement + 1, iterations, clientIndex + 1, clientCount); | ||
} | ||
|
||
var actions = getActionMap(clients.get(clientIndex), dataSize, async); |
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.
why does a new action map need to be created for each iteration? or more than once?
asyncTasks.forEach( | ||
future -> { | ||
try { | ||
var futureResult = future.get(); |
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.
I don't see where the different tasks are offloaded to different threads here. You don't use a fork/join service or an executor - this seems like the tasks are executed serially, instead of concurrently. what am I missing?
} | ||
|
||
@Override | ||
public void set(String key, String value) { |
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.
this seems like the wrong way to work with Jedis. first of all, all of the threads that share the same client also share the same jedisResource, which isn't threadsafe.
second, because Jedis doesn't support asynchronous API, the way to send multiple commands on the same connection concurrently is by using pipelines.
at the very least, you shouldn't reuse resources across threads, but a more accurate benchmark also wouldn't check single commands on each resource, but will pipeline multiple commands.
* Add Java-client benchmarking app Signed-off-by: acarbonetto <[email protected]> * spotless apply Signed-off-by: acarbonetto <[email protected]> * Update on command line options Signed-off-by: acarbonetto <[email protected]> * Update README Signed-off-by: acarbonetto <[email protected]> * Spotless apply: Signed-off-by: acarbonetto <[email protected]> * Update README example Signed-off-by: acarbonetto <[email protected]> * update commandline defaults for review comments Signed-off-by: acarbonetto <[email protected]> * Remove TLS flag argument from option Signed-off-by: acarbonetto <[email protected]> * Add lettuce clients for benchmarking Signed-off-by: acarbonetto <[email protected]> * Spotless apply Signed-off-by: acarbonetto <[email protected]> * Add Jedis clients Signed-off-by: acarbonetto <[email protected]> * Add to app Signed-off-by: acarbonetto <[email protected]> * Add for-loop for data size list Signed-off-by: acarbonetto <[email protected]> * Add TPS for all async items Signed-off-by: acarbonetto <[email protected]> * spotless apply Signed-off-by: acarbonetto <[email protected]> * Fix TPS calculations Signed-off-by: acarbonetto <[email protected]> * Accept TLS as a flag Signed-off-by: acarbonetto <[email protected]> * Start threads; then wait for results Signed-off-by: acarbonetto <[email protected]> * Add java-jni client Signed-off-by: acarbonetto <[email protected]> * Handle Exceptions from client; add JniSyncClient fixes Signed-off-by: acarbonetto <[email protected]> * Clean up latency and add error checking Signed-off-by: acarbonetto <[email protected]> * Minor fixes. Signed-off-by: Yury-Fridlyand <[email protected]> * Fix result printing. Signed-off-by: Yury-Fridlyand <[email protected]> * Add TPS. Signed-off-by: Yury-Fridlyand <[email protected]> * Remove duplicates. Reorganize and fix imports. Signed-off-by: Yury-Fridlyand <[email protected]> * Int ctor fix. Signed-off-by: Yury-Fridlyand <[email protected]> * Iteration 1. Signed-off-by: Yury-Fridlyand <[email protected]> * Iteration 2: connected! Signed-off-by: Yury-Fridlyand <[email protected]> * Iteration 3: `get` and `set`. Signed-off-by: Yury-Fridlyand <[email protected]> * Iteration 4: benchmark. Signed-off-by: Yury-Fridlyand <[email protected]> * Iteration 5: some fixes. Signed-off-by: Yury-Fridlyand <[email protected]> * Change number of threads in Benchmarking threadpool * Revert "Change number of threads in Benchmarking threadpool" This reverts commit e3f7596. * Add more flushing rules and UT. Signed-off-by: Yury-Fridlyand <[email protected]> * Client clean up. Signed-off-by: Yury-Fridlyand <[email protected]> * Client optimizations. (#37) * Client optimizations. Signed-off-by: Yury-Fridlyand <[email protected]> * minor cleanup. Signed-off-by: Yury-Fridlyand <[email protected]> * Optimize building a command. Signed-off-by: Yury-Fridlyand <[email protected]> * Typo fix. Signed-off-by: Yury-Fridlyand <[email protected]> * Minor rename. Signed-off-by: Yury-Fridlyand <[email protected]> * Clean up Redis close connection Signed-off-by: Andrew Carbonetto <[email protected]> * Clean up Redis close connection Signed-off-by: Andrew Carbonetto <[email protected]> * Minor changes. Signed-off-by: Yury-Fridlyand <[email protected]> * Add todos to closeConnection() Signed-off-by: Andrew Carbonetto <[email protected]> --------- Signed-off-by: Yury-Fridlyand <[email protected]> Signed-off-by: Andrew Carbonetto <[email protected]> Co-authored-by: Andrew Carbonetto <[email protected]> * Address PR feedback. Signed-off-by: Yury-Fridlyand <[email protected]> * Rename Signed-off-by: Yury-Fridlyand <[email protected]> * Rename2 Signed-off-by: Yury-Fridlyand <[email protected]> * Fix CI Signed-off-by: Yury-Fridlyand <[email protected]> * More fixes. Signed-off-by: Yury-Fridlyand <[email protected]> * Some changes. Signed-off-by: Yury-Fridlyand <[email protected]> * add null check Signed-off-by: Yury-Fridlyand <[email protected]> * autoflush Signed-off-by: Yury-Fridlyand <[email protected]> * Apply suggestions from code review Signed-off-by: Yury-Fridlyand <[email protected]> Co-authored-by: Andrew Carbonetto <[email protected]> * minor changes Signed-off-by: Yury-Fridlyand <[email protected]> --------- Signed-off-by: acarbonetto <[email protected]> Signed-off-by: Yury-Fridlyand <[email protected]> Signed-off-by: Andrew Carbonetto <[email protected]> Co-authored-by: acarbonetto <[email protected]> Co-authored-by: Jonathan Louie <[email protected]>
Jedis
and Lettuce
clients (sync and async)Jedis
and Lettuce
clients (sync and async)
@acarbonetto why did you add "[to remove]" to the PR's name? this PR will be merged, not accumulated in other PRs, because it will make the next PR hard to review, because all of the code of this PR will be there, too. Github doesn't have a good mechanism for marking code as reviewed and to mark it out of the current PR. We merge PRs to |
That is an intention, not an explanation for why would you do that. As I said, if this PR is superseded into other PRs, we will need to re-review it on every other PR. Additionally, what you're effectively suggesting is long-lived branches, which are a harder to maintain and update. |
You requested to split this PR, so what we actually did. |
oh, no. I owe you an apology - I followed the order of PRs in the wrong order. Yes, this PR should be removed. |
Description of changes:
Jedis
andLettuce
clients.Notes:
Jedis
has no async API, so Jedis async API replaced by Java async featuresJedis
does not support concurrent accessBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.