Refactor to Client-Manager-Connection layers #42

acarbonetto · 2023-12-01T17:10:23Z

Refactor of main Java client into layers of responsibility:

client api layer
integration layer
connection manager layer

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

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

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

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

java/client/src/main/java/babushka/api/Client.java

java/.gitignore

java/build.gradle

Yury-Fridlyand · 2023-12-01T19:35:55Z

java/client/src/main/java/babushka/api/Client.java

+
+  private final ConnectionManager connectionManager;
+  private final CommandManager commandManager;
+


private final Object configuration;

which contains:

flushing policy

logging config

profiling config

multithreading config

etc

kitten pictures

definitely.
Most definitely with 🐱 pictures

java/client/src/main/java/babushka/api/Client.java

Yury-Fridlyand · 2023-12-01T20:09:32Z

java/client/src/main/java/babushka/managers/CommandManager.java

+  }
+
+  private static RedisRequest redisSingleCommand(RequestType command, List<String> args) {
+    var commandArgs = RedisRequestOuterClass.Command.ArgsArray.newBuilder();


Optimize imports to shorten the identifiers (and below too)

Yury-Fridlyand · 2023-12-01T20:10:41Z

java/client/src/main/java/babushka/managers/CommandManager.java

+  public static String resolveRedisResponseToString(Response response) {
+    if (response.hasConstantResponse()) {
+      return BabushkaCoreNativeDefinitions.valueFromPointer(response.getRespPointer()).toString();


Suggested change

public static String resolveRedisResponseToString(Response response) {

if (response.hasConstantResponse()) {

return BabushkaCoreNativeDefinitions.valueFromPointer(response.getRespPointer()).toString();

public String resolveRedisResponseToString(Response response) {

if (response.hasConstantResponse()) {

return nativeDefinitions.valueFromPointer(response.getRespPointer()).toString();

Yury-Fridlyand · 2023-12-01T20:15:07Z

java/client/src/main/java/babushka/managers/CommandManager.java

+    return CompletableFuture.supplyAsync(
+            () -> {
+              socketConnection.writeAndFlush(request);
+              return future;
+            })
+        .thenCompose(f -> f)


nit

Suggested change

return CompletableFuture.supplyAsync(

() -> {

socketConnection.writeAndFlush(request);

return future;

})

.thenCompose(f -> f)

return CompletableFuture.runAsync(() -> socketConnection.writeAndFlush(request))

.thenApply(v -> future)

Yury-Fridlyand · 2023-12-01T20:16:19Z

java/client/src/main/java/babushka/managers/ConnectionManager.java

+   * Close socket connection and drop all channels
+   * TODO: provide feedback that the connection was properly closed
+   */
+  public void closeConnection() {


return string or bool?

not sure...

Yury-Fridlyand · 2023-12-01T20:16:33Z

java/client/src/main/java/babushka/managers/ConnectionManager.java

+        .addAddresses(
+            ConnectionRequestOuterClass.NodeAddress.newBuilder().setHost(host).setPort(port).build())
+        .setTlsMode(useSsl ? ConnectionRequestOuterClass.TlsMode.SecureTls : ConnectionRequestOuterClass.TlsMode.NoTls)
+        .setClusterModeEnabled(clusterMode)
+        .setReadFrom(ConnectionRequestOuterClass.ReadFrom.Primary)


optimize imports

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

acarbonetto · 2023-12-01T18:47:00Z

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());
+                log_warn("received error", format!("{} for callback {}", error_message.as_str(), callback_index));


we should revert this

acarbonetto · 2023-12-01T18:47:25Z

java/README.md


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


"Babushka"... but honestly, we have to change this anyways.

acarbonetto · 2023-12-01T21:51:59Z

java/client/src/main/java/babushka/api/Client.java

+
+  private final ConnectionManager connectionManager;
+  private final CommandManager commandManager;
+


definitely.
Most definitely with 🐱 pictures

java/client/src/main/java/babushka/api/Client.java

Elen-Ghulam · 2023-12-01T22:06:44Z

java/client/build.gradle

 }

 tasks.register('protobuf', Exec) {
    doFirst {
-        project.mkdir(Paths.get(project.projectDir.path, 'src/main/java/javababushka/generated').toString())
+        project.mkdir(Paths.get(project.projectDir.path, 'src/main/java/babushka/models/protobuf').toString())


Suggestion: place hard wired strings in a config file or declare it as a constant

jonathanl-bq

Part 1 of my review

jonathanl-bq · 2023-12-01T22:22:46Z

java/client/src/main/java/babushka/api/Awaiter.java

+    try {
+      return future.get(timeout, TimeUnit.MILLISECONDS);
+    } catch (Exception ignored) {
+      return null;


We want to ignore the exception here? Why? Also, should we be returning null instead of Optional?

I've added a TODO to deal with this properly.

jonathanl-bq · 2023-12-01T22:27:24Z

java/client/src/main/java/babushka/api/Commands.java

+   * @param key The key name
+   * @param value The value to set
+   */
+  public Future<String> asyncSet(String key, String value) {


We probably don't want to return Future over CompletableFuture. Otherwise, you can't compose dependent async computations on the client side in a non-blocking manner, which you would be able to do with thenCompose in CompletableFuture.

Good points.
We need to update this in our API, but good to be consistent.

jonathanl-bq · 2023-12-01T22:28:09Z

java/client/src/main/java/babushka/api/Commands.java

+   *
+   * @param key The key name
+   */
+  public Future<String> asyncGet(String key) {


Same here. We probably should not return a regular Future.

jonathanl-bq · 2023-12-01T22:30:51Z

java/client/src/main/java/babushka/api/Commands.java

+   */
+  public String get(String key) {
+    return Awaiter.await(asyncGet(key));
+    // TODO support non-strings


https://redis.io/commands/get/ says it only supports Strings

This had to do with the response.
We want to support response objects, not just strings (in case something went wrong)

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

jonathanl-bq

Part 2 of my review

jonathanl-bq · 2023-12-01T22:37:55Z

java/client/src/main/java/babushka/connectors/SocketConnection.java

+      SocketConnection.socketPath = socketPath;
+      return;
+    }
+    throw new RuntimeException("socket path can only be declared once");


Is it common practice to throw a generic RuntimeException like this?

This shouldn't happen... so we throw Runtime for unexpected activity

java/client/src/main/java/babushka/connectors/SocketConnection.java

jonathanl-bq · 2023-12-01T22:47:09Z

java/client/src/main/java/babushka/connectors/handlers/ReadHandler.java

+   */
+  @Override
+  public void channelRead(@NonNull ChannelHandlerContext ctx, @NonNull Object msg)
+      throws Exception {


Do we want to be throwing generic Exception here?

ChannelInboundHandlerAdapter throws a general Exception

jonathanl-bq · 2023-12-01T22:50:35Z

java/client/src/main/java/babushka/ffi/resolvers/BabushkaCoreNativeDefinitions.java

+package babushka.ffi.resolvers;
+
+public class BabushkaCoreNativeDefinitions {
+  public static native String startSocketListenerExternal() throws Exception;


Again, should we be throwing generic Exception here?

jonathanl-bq · 2023-12-01T22:56:23Z

java/client/src/main/java/babushka/managers/CallbackManager.java

+   */
+  public int registerRequest(CompletableFuture<Response> future) {
+    int callbackId = requestId.incrementAndGet();
+    CallbackManager.responses.put(callbackId, future);


Does the responses map just pile up Futures? I'm worried these may just pile up until we OOM.

that could happen if we aren't getting any responses from Babushka and keep on sending requests.

jonathanl-bq · 2023-12-01T23:14:45Z

java/client/src/main/java/babushka/models/protobuf/redis_request/RedisRequestOuterClass.java

@@ -0,0 +1,6868 @@
+// Generated by the protocol buffer compiler.  DO NOT EDIT!


Again, generated file probably should be removed.

jonathanl-bq · 2023-12-01T23:14:57Z

java/client/src/main/java/babushka/models/protobuf/response/ResponseOuterClass.java

@@ -0,0 +1,2162 @@
+// Generated by the protocol buffer compiler.  DO NOT EDIT!


Again, generated file probably should be removed.

jonathanl-bq · 2023-12-01T23:18:01Z

java/client/src/main/java/babushka/models/RequestBuilder.java

+   * @param response Redis Response
+   * @return String or null
+   */
+  public static String resolveRedisResponseToString(ResponseOuterClass.Response response) {


We don't want to return an Optional<String>? Also, should the input be non-nullable?

We don't want to return a String.
We want to support our own Response objects with support for success responses and errors.

jonathanl-bq · 2023-12-01T23:18:50Z

java/client/src/main/java/babushka/models/RequestBuilder.java

+        .build();
+  }
+
+  public static RedisRequestOuterClass.RedisRequest redisSingleCommand(


Should we make sure the inputs are non-nullable?

Garbage in. Garbage out?

jonathanl-bq · 2023-12-01T23:19:50Z

java/client/src/main/java/babushka/models/RequestBuilder.java

+
+  /** Build a protobuf connection request object */
+  // TODO support more parameters and/or configuration object
+  public static ConnectionRequestOuterClass.ConnectionRequest getConnectionRequest(


Should we be allowing nullable host strings?

maybe...? why would a user do that?

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

Yury-Fridlyand · 2023-12-02T00:52:36Z

java/client/src/main/java/babushka/connectors/handlers/ReadHandler.java

+      CallbackManager.connectionRequests.pop().complete(response);
+    } else {
+      CallbackManager.responses.get(callbackId).complete(response);


Suggested change

CallbackManager.connectionRequests.pop().complete(response);

} else {

CallbackManager.responses.get(callbackId).complete(response);

CallbackManager.connectionRequests.pop().completeAsync(() -> response);

} else {

CallbackManager.responses.get(callbackId).completeAsync(() -> response);

Yury-Fridlyand · 2023-12-02T00:53:56Z

java/client/src/main/java/babushka/managers/CommandManager.java

+              socketConnection.writeAndFlush(redisSingleCommand(command, args));
+              return future;
+            })
+        .thenCompose(f -> f)


Suggested change

.thenCompose(f -> f)

.thenComposeAsync(f -> f)

Yury-Fridlyand · 2023-12-02T00:54:29Z

java/client/src/main/java/babushka/managers/CommandManager.java

+              return future;
+            })
+        .thenCompose(f -> f)
+        .thenApply(RequestBuilder::resolveRedisResponseToString);


Suggested change

.thenApply(RequestBuilder::resolveRedisResponseToString);

.thenApplyAsync(RequestBuilder::resolveRedisResponseToString);

java/client/src/main/java/babushka/managers/ConnectionManager.java

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

1. Create a new channel per client. `Channel` is wrapped by `ChannedHandler` class. 2. Create `CallbackManager` per client. Signed-off-by: Yury-Fridlyand <[email protected]>

Yury-Fridlyand · 2023-12-06T05:21:19Z

java/client/src/main/java/babushka/connectors/SocketConnection.java

+
+  private static String socketPath;
+
+  public static void setSocketPath(String socketPath) {


A security issue - a user may call this function before we do. Options there:

don't export this function (make private or make class private)

make socketPath be set statically automatically

Function should be syncronized though.

Yury-Fridlyand · 2023-12-08T06:34:58Z

java/client/src/main/java/babushka/models/RequestBuilder.java

+   * @param response Redis Response
+   * @return String or null
+   */
+  public static String resolveRedisResponseToString(Response response) {


Move this func to another file

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

Yury-Fridlyand and others added 13 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]>

Revert IT.

9cecf23

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

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

Optimize a bit.

aa5aa52

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

Refactor to Client-Manager-Connection layers

5481dc2

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

acarbonetto changed the title ~~Dev refactor netty client~~ Refactor to Client-Manager-Connection layers Dec 1, 2023

Yury-Fridlyand reviewed Dec 1, 2023

View reviewed changes

Update refactor to include client factory builders

667d277

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

acarbonetto commented Dec 1, 2023

View reviewed changes

Elen-Ghulam reviewed Dec 1, 2023

View reviewed changes

jonathanl-bq reviewed Dec 1, 2023

View reviewed changes

Add unit tests for API

537d538

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

jonathanl-bq reviewed Dec 1, 2023

View reviewed changes

Clean up sync calls; add close connection

2f81317

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

Yury-Fridlyand reviewed Dec 2, 2023

View reviewed changes

acarbonetto and others added 2 commits December 4, 2023 14:23

Update for review comments

e828f0c

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

Applying some fixes from dev-split-netty-client branch.

3086e6f

1. Create a new channel per client. `Channel` is wrapped by `ChannedHandler` class. 2. Create `CallbackManager` per client. Signed-off-by: Yury-Fridlyand <[email protected]>

Yury-Fridlyand reviewed Dec 6, 2023

View reviewed changes

Yury-Fridlyand reviewed Dec 8, 2023

View reviewed changes

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-refactor-netty-client branch January 20, 2024 02:40


		private final ConnectionManager connectionManager;
		private final CommandManager commandManager;

		@@ -0,0 +1,6868 @@
		// Generated by the protocol buffer compiler. DO NOT EDIT!

		@@ -0,0 +1,2162 @@
		// Generated by the protocol buffer compiler. DO NOT EDIT!

	.thenApply(RequestBuilder::resolveRedisResponseToString);
	.thenApplyAsync(RequestBuilder::resolveRedisResponseToString);


		private static String socketPath;

		public static void setSocketPath(String socketPath) {

Refactor to Client-Manager-Connection layers #42

Refactor to Client-Manager-Connection layers #42

Conversation

acarbonetto commented Dec 1, 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

jonathanl-bq left a 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

jonathanl-bq left a 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

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 commented Dec 1, 2023 •

edited

Loading