Split netty client into two files #40

Yury-Fridlyand · 2023-11-24T21:53:02Z

Please ignore big commit history - it is caused by fact that dev branch was diverged.

This PR contains split JNI/Netty client into two files:

NettyWrapper
Client itself

Client works as a wrapper for NettyWrapper (probably I should rename it).

NettyWrapper creates a singleton channel, which can be used by unlimited number of clients concurrently.

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]>

* 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: Yury-Fridlyand <[email protected]>

jonathanl-bq · 2023-11-24T22:23:21Z

babushka-core/src/socket_listener.rs

@@ -24,6 +24,7 @@ use std::cell::Cell;
 use std::rc::Rc;
 use std::{env, str};
 use std::{io, thread};
+use std::fmt::format;


We shouldn't need to import this.

Thanks, removed in d58bff9.

jonathanl-bq · 2023-11-24T22:31:17Z

java/client/src/main/java/javababushka/NettyWrapper.java

+  }
+
+  public void close() {
+    // channel.closeFuture().sync()


We don't want to check in this comment here, do we?

Removed in d58bff9.

java/benchmarks/src/main/java/javababushka/benchmarks/utils/Benchmarking.java

jonathanl-bq · 2023-11-24T22:32:15Z

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

@@ -23,11 +23,6 @@ public String getName() {
    return name;
  }

-  @Override
-  public void closeConnection() {
-    testClient.closeConnection();


Do we not need to close the connection anymore? I'm not sure why this is removed.

Another client instance may still use the connection - it is common for all.

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

acarbonetto · 2023-11-28T00:26:05Z

babushka-core/src/socket_listener.rs

@@ -210,7 +210,7 @@ async fn write_result(
                    error_message.into(),
                ))
            } else {
-                log_warn("received error", error_message.as_str());


did you mean to check this in?

Yes, it is more verbose

While true, this change is irrelevant to the PR. We should put this in a separate PR.

acarbonetto · 2023-11-28T00:27:35Z

java/integTest/build.gradle

@@ -0,0 +1,18 @@
+plugins {


this probably belongs in a separate PR?

Oh right, reverting

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

acarbonetto · 2023-11-28T19:56:49Z

babushka-core/src/socket_listener.rs

@@ -210,7 +210,7 @@ async fn write_result(
                    error_message.into(),
                ))
            } else {
-                log_warn("received error", error_message.as_str());


While true, this change is irrelevant to the PR. We should put this in a separate PR.

java/client/src/main/java/javababushka/NettyWrapper.java

acarbonetto · 2023-11-28T20:24:59Z

java/client/src/main/java/javababushka/NettyWrapper.java

+  // Futures to handle responses. Index is callback id, starting from 1 (0 index is for connection
+  // request always).
+  // Is it not a concurrent nor sync collection, but it is synced on adding. No removes.
+  private final Map<Integer, CompletableFuture<Response>> responses = new ConcurrentHashMap<>();


We should have a builder factor to define all of this together, and then we can share the responses map between files.
This would also mean we don't define the instance object statically (better to define it explicitly or at first use).

Please, re-review it is moved to another class

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

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

acarbonetto

@Yury-Fridlyand We should go full MVC on this refactor/redesign. This will make it much easier to test. As is, we kind of have a MV* but we haven't all decided what our Model is (protobuf?) and both our client and wrappers/SocketManager has multiple responsibilities. Now would be a good time to split up these responsibilities... I'm thinking we should also split connection requests from command requests since those requests types have completely divergent calls.

acarbonetto · 2023-11-30T13:30:18Z

java/benchmarks/build.gradle

@@ -36,7 +34,7 @@ java {

 application {
    // Define the main class for the application.
-    mainClass = 'javababushka.benchmarks.BenchmarkingApp'
+    mainClass = 'babushka.benchmarks.BenchmarkingApp'


I've been meaning to make this change, but a new name here is imminent. I was going to wait until we needed to change to 'Glide' or whatever...

I was requested to do this upstream. So I made the same change here to have less conflicts later.

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

jonathanl-bq and others added 30 commits October 3, 2023 13:33

Add Jedis and Lettuce benchmarks

06574ea

Start ignoring .gradle files

5990767

Update gitignore and remove generated files from git

6ae93f5

Signed-off-by: acarbonetto <[email protected]>

Update gitignore and remove generated files from git

8203c4d

Signed-off-by: acarbonetto <[email protected]>

Update gitignore and remove generated files from git

e517744

Signed-off-by: acarbonetto <[email protected]>

Add benchmarks for GET non-existing

2a11e9a

Revert "Update gitignore and remove generated files from git"

cbb0dcb

This reverts commit d9b26a6.

fix redis-rs submodules

5f51a5b

Signed-off-by: acarbonetto <[email protected]>

Randomize commands in Java benchmarks

52df672

rename chooseAction to randomAction

84f0efc

Add a Java benchmarking app (#7)

1983974

* Add a java app to run benchmarks --------- Signed-off-by: acarbonetto <[email protected]>

Add Readme and update install_and_test script to runJava

6c1fb45

Signed-off-by: acarbonetto <[email protected]>

Add Readme and update install_and_test script to runJava

46d0cf6

Signed-off-by: acarbonetto <[email protected]>

Combine java pipeline and java benchmarks (#8)

d526f96

* 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)

436da8f

* 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]>

Move duplicated logic in benchmark JS scripts to a single file, and c…

016f5f6

…onvert to TypeScript. (valkey-io#456) removed duplicated logic and refactored to typescript Signed-off-by: acarbonetto <[email protected]>

Rename jabushka to javababushka (#14)

8664d05

Signed-off-by: acarbonetto <[email protected]>

Add missing renames. (#17)

b15f93e

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

Update redis-rs to match main branch

d99d27a

Signed-off-by: acarbonetto <[email protected]>

Create clients only once per iteration (#19)

540f49a

Signed-off-by: acarbonetto <[email protected]>

Add Java-client benchmarking app

035ef20

Signed-off-by: acarbonetto <[email protected]>

spotless apply

0883985

Signed-off-by: acarbonetto <[email protected]>

Update on command line options

d15ade1

Signed-off-by: acarbonetto <[email protected]>

Update README

e3634ba

Signed-off-by: acarbonetto <[email protected]>

Spotless apply:

500a043

Signed-off-by: acarbonetto <[email protected]>

Update README example

0bf1678

Signed-off-by: acarbonetto <[email protected]>

update commandline defaults for review comments

12efc63

Signed-off-by: acarbonetto <[email protected]>

Remove TLS flag argument from option

951aa8b

Signed-off-by: acarbonetto <[email protected]>

Revert some changes added by mistake.

986f468

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

Yury-Fridlyand requested review from jonathanl-bq, acarbonetto, Elen-Ghulam and SanHalacogluImproving November 24, 2023 21:53

jonathanl-bq reviewed Nov 24, 2023

View reviewed changes

Yury-Fridlyand added 2 commits November 27, 2023 12:57

Address PR feedback.

d58bff9

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

Fix generating callback ID and handling new connections.

507ef9f

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

acarbonetto reviewed Nov 28, 2023

View reviewed changes

Revert IT.

9cecf23

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

acarbonetto reviewed Nov 28, 2023

View reviewed changes

Yury-Fridlyand mentioned this pull request Nov 29, 2023

Add java JNI/Netty client - rust part. valkey-io/valkey-glide#631

Merged

acarbonetto and others added 8 commits November 29, 2023 14:21

Add unit tests for Client

8ffdbea

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

Implement Bulk value case in FFI layer

4ecd5d8

More split.

2bf997e

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

Renaming

1aa0707

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

Fix build.

14e3e18

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

Clean up commented logging.

6d2ae81

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

Javadoc

e7ee9a9

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

Revert some renamings.

540120e

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

acarbonetto reviewed Nov 30, 2023

View reviewed changes

Yury-Fridlyand added 6 commits November 30, 2023 10:51

Optimize a bit.

aa5aa52

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

Split more.

9a0c5d6

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

Grab some changes from upstream.

2c9aa38

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

Misc fixes.

fbff211

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

Open channel (connection) per client, share thread pools.

701da44

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

Minor fixes.

5bf6672

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

Yury-Fridlyand deleted the branch integ-java-client-milestone-1 December 15, 2023 18:32

Yury-Fridlyand closed this Dec 15, 2023

Yury-Fridlyand deleted the dev-split-netty-client branch December 15, 2023 18:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split netty client into two files #40

Split netty client into two files #40

Yury-Fridlyand commented Nov 24, 2023 •

edited

Loading

jonathanl-bq Nov 24, 2023

Yury-Fridlyand Nov 27, 2023

jonathanl-bq Nov 24, 2023

Yury-Fridlyand Nov 27, 2023

jonathanl-bq Nov 24, 2023

Yury-Fridlyand Nov 27, 2023

acarbonetto Nov 28, 2023

Yury-Fridlyand Nov 28, 2023

acarbonetto Nov 28, 2023

acarbonetto Nov 28, 2023

Yury-Fridlyand Nov 28, 2023

acarbonetto Nov 28, 2023

acarbonetto Nov 28, 2023

Yury-Fridlyand Nov 30, 2023

acarbonetto left a comment

acarbonetto Nov 30, 2023

Yury-Fridlyand Nov 30, 2023

Split netty client into two files #40

Split netty client into two files #40

Conversation

Yury-Fridlyand commented Nov 24, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acarbonetto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yury-Fridlyand commented Nov 24, 2023 •

edited

Loading