Add test clients for benchmarking. #632

Yury-Fridlyand · 2023-11-22T21:20:25Z

Issue #, if available:
Part 3 of 4. (see #629)

Description of changes:
This PR contains testing clients for #629. Depends on #629 and #631 - some code is commented out. Should be merged after these PRs!

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Signed-off-by: Yury-Fridlyand <[email protected]>

Yury-Fridlyand · 2023-11-24T01:22:11Z

Add client timeout as requiested in #629 (comment)

Signed-off-by: Yury-Fridlyand <[email protected]>

Yury-Fridlyand · 2023-11-24T04:29:25Z

Add client timeout as requiested in #629 (comment)

Fixed in 07cfd8b

Signed-off-by: Yury-Fridlyand <[email protected]>

acarbonetto · 2023-11-24T19:18:42Z

java/benchmarks/src/main/java/javababushka/benchmarks/clients/babushka/JniNettyClient.java

@@ -0,0 +1,50 @@
+package javababushka.benchmarks.clients.babushka;
+
+/*


Note: Commented out for this PR.
Will be included as part of #631

barshaul · 2023-11-26T07:42:58Z

Hey @Yury-Fridlyand, the tests are failing and the PR is still marked as 'draft' - so let me know when it's ready for review

Yury-Fridlyand · 2023-11-27T15:53:38Z

Hey @Yury-Fridlyand, the tests are failing and the PR is still marked as 'draft' - so let me know when it's ready for review

Marked as draft to prevent merge, but it is ready for review. I'll update this PR once #629 and #631 merged.

Signed-off-by: Andrew Carbonetto <[email protected]>

Signed-off-by: Yury-Fridlyand <[email protected]>

barshaul · 2023-12-05T13:01:51Z

@Yury-Fridlyand Please rebase over the changes from #629, there is an overlap with this two PRs which makes it hard to understand what needs to be reviewed. Ping me when it's rebased and ready for review

…ilestone-1-benchmarking-clients Signed-off-by: Yury-Fridlyand <[email protected]>

Signed-off-by: Yury-Fridlyand <[email protected]>

Yury-Fridlyand · 2023-12-07T02:00:01Z

@barshaul, ready for review

barshaul · 2023-12-05T12:58:46Z

java/benchmarks/src/main/java/babushka/benchmarks/BenchmarkingApp.java

@@ -97,7 +93,7 @@ private static Options getOptions() {
            .hasArg(true)
            .desc(
                "one of:"
-                    + " all|jedis|jedis_async|lettuce|lettuce_async|babushka_async|all_async|all_sync"
+                    + " all|jedis|lettuce|lettuce_async|babushka_async|all_async|all_sync"


Commented on prev PR - make sure to rebase over the changes commited there

barshaul · 2023-12-07T07:47:36Z

java/README.md

+The Java client contains the following parts:
+
+1. A Java client (lib folder): wrapper to rust-client.
+2. An examples script: to sanity test babushka and similar java-clients against a redis host.


I couldn't find this script?

And we don't want the examples folder to contain examples of other clients, only babushka

It does not exist yet (as well as the client itself)
Updated in 3e0f70b.

barshaul · 2023-12-07T07:53:41Z

java/README.md

+
+1. A Java client (lib folder): wrapper to rust-client.
+2. An examples script: to sanity test babushka and similar java-clients against a redis host.
+3. A benchmark app: to performance benchmark test babushka and similar java-clients against a redis host.


A benchmark app: A dedicated benchmarking tool designed to evaluate and compare the performance of Babushka and other Java clients.

Updated in 3e0f70b

barshaul · 2023-12-07T07:55:54Z

java/README.md

+* `port`: redis server port number
+* `tls`: redis TLS configured
+
+### Troubleshooting


Already commented for Andrew PR, it isn't specific for the Java wrapper, please remove it

Removed in 3e0f70b.

barshaul · 2023-12-07T07:57:09Z

java/README.md

@@ -109,3 +115,18 @@ You can run benchmarks using `./gradlew run`. You can set arguments using the ar
 ./gradlew run --args="-help"
 ./gradlew run --args="-resultsFile=output -dataSize \"100 1000\" -concurrentTasks \"10 100\" -clients all -host localhost -port 6279 -clientCount \"1 5\" -tls"
 ```
+
+The following arguments are accepted: 


Adding it to the README file will require to maintain it when the benchmark app will be changed. It's enough to describe the different options in the -help option

I agree, but benchmarking apps are not supposed to be updated.

We do plan to add more arguments in the future (e.g. data value, command type, changing the ratio of set/get etc...)

barshaul · 2023-12-07T08:11:54Z

java/benchmarks/src/main/java/babushka/benchmarks/clients/jedis/JedisClient.java

+  @Override
+  public void connectToRedis(ConnectionSettings connectionSettings) {
+    pool =
+        new JedisPool(connectionSettings.host, connectionSettings.port, connectionSettings.useSsl);


It looks like you're not testing Jedis in a cluster mode at all, redis cluster should be used with JedisCluster

Fixed in 3e0f70b.

barshaul · 2023-12-07T08:14:17Z

java/benchmarks/src/main/java/babushka/benchmarks/clients/babushka/JniNettyClient.java

+
+/*
+
+public class JniNettyClient implements SyncClient, AsyncClient<Response> {


Please remove it from this PR and add it when the java-babushka will be added

Removed in 3e0f70b.

barshaul · 2023-12-07T08:18:13Z

java/benchmarks/src/test/java/babushka/benchmarks/jedis/JedisClientIT.java

+  private static JedisClient jedisClient;
+
+  @BeforeAll
+  static void initializeJedisClient() {


IMO we don't want to have other clients tests in our project, keeping them in the benchmark is enough. @shachlanAmazon what do you think?

definitely. Jedis & Lettuce can test themselves, we don't need to test them. And we don't test the benchmark, just like we don't test our tests.

We can remove these tests now. I agree that they no longer add any value to our clients.

Removed in 3e0f70b.

barshaul · 2023-12-07T08:18:47Z

java/benchmarks/src/test/java/babushka/benchmarks/lettuce/LettuceAsyncClientIT.java

+  private static LettuceAsyncClient otherLettuceClient;
+
+  @BeforeAll
+  static void initializeLettuceClient() {


same - other clients tests

Removed in 3e0f70b.

…ilestone-1-benchmarking-clients Signed-off-by: Yury-Fridlyand <[email protected]>

Signed-off-by: Yury-Fridlyand <[email protected]>

barshaul · 2023-12-11T16:21:28Z

.../benchmarks/src/main/java/babushka/benchmarks/clients/lettuce/LettuceAsyncClusterClient.java

+  private StatefulRedisClusterConnection<String, String> clusterConnection;
+
+  @Override
+  public void connectToRedis(ConnectionSettings connectionSettings) {


@Yury-Fridlyand I think that the same as you did for Jedis, no need to have a separate file for LettuceClusterClient, you can use LettuceAsyncClient both for standalone and cluster

barshaul · 2023-12-11T16:26:47Z

java/README.md

+The Java client contains the following parts:
+
+1. A Java client (lib folder): wrapper to rust client.
+2. An examples script: to sanity test babushka against a Redis host.


The example script would probably be with the rest examples under examples/ folder, so we don't need to specify it here. Lets remove number 2

barshaul

Fix the last comments, squash commits & ship it

Signed-off-by: Yury-Fridlyand <[email protected]>

Add test clients for benchmarking.

ddc71b4

Signed-off-by: Yury-Fridlyand <[email protected]>

Yury-Fridlyand added the java issues and fixes related to the java client label Nov 22, 2023

Yury-Fridlyand requested review from jonathanl-bq, barshaul, asafpamzn, acarbonetto and Elen-Ghulam November 22, 2023 21:20

Yury-Fridlyand mentioned this pull request Nov 24, 2023

Add benchmarking application. No test clients yet. #629

Merged

Address PR feedback.

07cfd8b

Signed-off-by: Yury-Fridlyand <[email protected]>

Restore Jedis async.

07e98db

Signed-off-by: Yury-Fridlyand <[email protected]>

acarbonetto reviewed Nov 24, 2023

View reviewed changes

acarbonetto and others added 4 commits November 27, 2023 12:51

Make JedisClient use a shared pool of resources

fb41f7a

Signed-off-by: Andrew Carbonetto <[email protected]>

Remove JAVA_ASYNC option in the benchmarking

8e6c565

Signed-off-by: Andrew Carbonetto <[email protected]>

renaming

ec04c28

Signed-off-by: Yury-Fridlyand <[email protected]>

Revert some renamings.

f6fbbb7

Signed-off-by: Yury-Fridlyand <[email protected]>

Yury-Fridlyand mentioned this pull request Nov 30, 2023

[to remove] Java benchmarks: add Jedis and Lettuce clients (sync and async) #540

Closed

Yury-Fridlyand added 2 commits December 6, 2023 16:33

Merge remote-tracking branch 'upstream/main' into integ-java-client-m…

6635229

…ilestone-1-benchmarking-clients Signed-off-by: Yury-Fridlyand <[email protected]>

Add Jedis and Lettuce to benchmarking.

fb755af

Signed-off-by: Yury-Fridlyand <[email protected]>

Yury-Fridlyand marked this pull request as ready for review December 7, 2023 01:59

Yury-Fridlyand requested a review from a team as a code owner December 7, 2023 01:59

barshaul requested changes Dec 7, 2023

View reviewed changes

Yury-Fridlyand added 2 commits December 8, 2023 18:55

Merge remote-tracking branch 'upstream/main' into integ-java-client-m…

af3df15

…ilestone-1-benchmarking-clients Signed-off-by: Yury-Fridlyand <[email protected]>

Address PR feedback.

3e0f70b

Signed-off-by: Yury-Fridlyand <[email protected]>

barshaul reviewed Dec 11, 2023

View reviewed changes

barshaul approved these changes Dec 11, 2023

View reviewed changes

Address PR feedback.

874a4de

Signed-off-by: Yury-Fridlyand <[email protected]>

Yury-Fridlyand merged commit 42243dc into valkey-io:main Dec 11, 2023
3 checks passed

Yury-Fridlyand deleted the integ-java-client-milestone-1-benchmarking-clients branch December 11, 2023 23:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test clients for benchmarking. #632

Add test clients for benchmarking. #632

Yury-Fridlyand commented Nov 22, 2023

Yury-Fridlyand commented Nov 24, 2023

Yury-Fridlyand commented Nov 24, 2023

acarbonetto Nov 24, 2023

barshaul commented Nov 26, 2023

Yury-Fridlyand commented Nov 27, 2023

barshaul commented Dec 5, 2023

Yury-Fridlyand commented Dec 7, 2023

barshaul Dec 5, 2023

barshaul Dec 7, 2023

barshaul Dec 7, 2023

Yury-Fridlyand Dec 9, 2023

barshaul Dec 7, 2023

Yury-Fridlyand Dec 9, 2023

barshaul Dec 7, 2023

Yury-Fridlyand Dec 9, 2023

barshaul Dec 7, 2023

Yury-Fridlyand Dec 9, 2023

barshaul Dec 10, 2023 •

edited

Loading

barshaul Dec 7, 2023

Yury-Fridlyand Dec 9, 2023

barshaul Dec 7, 2023

Yury-Fridlyand Dec 9, 2023

barshaul Dec 7, 2023

shachlanAmazon Dec 7, 2023

acarbonetto Dec 8, 2023

Yury-Fridlyand Dec 9, 2023

barshaul Dec 7, 2023

Yury-Fridlyand Dec 9, 2023

barshaul Dec 11, 2023 •

edited

Loading

barshaul Dec 11, 2023

barshaul left a comment

		@@ -0,0 +1,50 @@
		package javababushka.benchmarks.clients.babushka;

		/*


		/*

		public class JniNettyClient implements SyncClient, AsyncClient<Response> {

Add test clients for benchmarking. #632

Add test clients for benchmarking. #632

Conversation

Yury-Fridlyand commented Nov 22, 2023

Yury-Fridlyand commented Nov 24, 2023

Yury-Fridlyand commented Nov 24, 2023

Choose a reason for hiding this comment

barshaul commented Nov 26, 2023

Yury-Fridlyand commented Nov 27, 2023

barshaul commented Dec 5, 2023

Yury-Fridlyand commented Dec 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barshaul Dec 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barshaul Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barshaul left a comment

Choose a reason for hiding this comment

barshaul Dec 10, 2023 •

edited

Loading

barshaul Dec 11, 2023 •

edited

Loading