Java: API layer with RedisClient #737

acarbonetto · 2023-12-29T17:38:15Z

Depends on #717.

Issue #, if available:

API layer for: #589

Description of changes:

In this PR, we're adding an API layer that creates a RedisClient async-ly using a RedisConfiguration. It then connects to Redis using the ConnectionManager. There are several builders in the RedisClient that use different configurations (localhost, host and port, or a full config object).

Out of Scope

Command Manager: sending requests to the command manager
All command calls will be added in a follow-up PR.
Transaction commands will be added in a follow-up PR.

Use cases

// connect to Redis
CompletableFuture<babushka.client.api.RedisClient> redisClientConnection =
    babushka.client.api.RedisClient.CreateClient(configuration);

// resolve the Future and get a RedisClient
babushka.client.api.RedisClient redisClient = redisClientConnection.get();

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

shachlanAmazon · 2023-12-31T09:01:36Z

java/client/src/main/java/glide/api/RedisClient.java

+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+/** Factory class for creating Glide/Redis-client connections */


this class is the actual connection, not just the factory.

It should say it's the standalone redis client

Updated in d23001f.

shachlanAmazon · 2023-12-31T09:04:13Z

java/client/src/test/java/glide/api/ConnectionManagerTest.java

+    // verify
+    // no exception
+    assertNull(result.get());
+    verify(channel).connect(eq(expectedProtobufConnectionRequest));


I don't understand what these tests actually test. What kind of behavior breakage will this test catch?

I don't see the value for quite a few of these tests. They seem to just be passthrough for the mock,

They test that the protobuf connection request is built as expected.
They will break if the protobuf connection requests change in a way that is unexpected.

shachlanAmazon · 2023-12-31T09:07:56Z

java/client/src/test/java/glide/api/ConnectionManagerTest.java

+  }
+
+  @Test
+  public void CheckBabushkaResponse_ConstantResponse_returnsSuccessfully()


where are you testing that the constant response was passed to the user?

Note: On a connection, the user receives a RedisClient with a completed CompletableFuture. They don't see the constant response. If we did that, we would have to change our CreateClient method.

I will add unit tests for CreateClient.

Tests added under 09fd5c7

shachlanAmazon · 2023-12-31T09:30:59Z

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

+  private final AtomicInteger requestId = new AtomicInteger(0);
+
+  /**
+   * Storage of Futures to handle responses. Map key is callback id, which starts from 1.<br>


remove
throughout

Updated in d23001f.

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

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

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

barshaul · 2024-01-02T14:54:14Z

java/client/src/main/java/glide/api/RedisClient.java

+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+/** Factory class for creating Glide/Redis-client connections */


It should say it's the standalone redis client

barshaul · 2024-01-02T15:00:07Z

java/client/src/main/java/glide/api/RedisClient.java

+  }
+
+  /**
+   * Async (non-blocking) connection to Redis.


should be changed to something like:
Creates an async (non-blocking) client for Redis in Standalone mode.

in general, the user-facing term we use is client, not connection. That is, unless we're discussing a concrete connection to a single node, or the UDS connection (which is mostly discussed internally and isn't user facing).

Updated in d23001f.

java/client/src/main/java/glide/api/RedisClient.java

barshaul · 2024-01-02T15:08:57Z

java/client/src/main/java/glide/api/models/configuration/BaseClientConfiguration.java

+import lombok.Singular;
+import lombok.experimental.SuperBuilder;
+
+/** Represents the configuration settings for a Redis client. */


should be changed to something like-
Represents the shared configuration settings for both Standalone and Cluster Redis clients.

Updated in d23001f.

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

barshaul · 2024-01-02T15:13:44Z

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

+
+  CompletableFuture<Response> channel;
+
+  public CompletableFuture<Void> closeConnection() {


Adding to Shachar's question - why CommandManager has closeConnection function?

barshaul · 2024-01-02T15:17:38Z

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

+    } else if (response.hasConstantResponse()) {
+      return null;
+    } else if (response.hasRespPointer()) {
+      throw new RuntimeException("Unexpected data in response");


In a connection request, we only handle the error scenario and the OK successful response. This would be unexpected... I don't know what to do with the data...

In conversation today, we decided that a ClosingException would be best (and cancel all the waiting responses).
I'll add a TODO and fix it under https://github.com/orgs/Bit-Quill/projects/4/views/6?pane=issue&itemId=48063887

barshaul · 2024-01-02T15:19:34Z

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

+    } else if (response.hasRespPointer()) {
+      throw new RuntimeException("Unexpected data in response");
+    }
+    // TODO commented out due to #710 https://github.com/aws/babushka/issues/710


can we remove this todo?
This PR changing connection response to OK was merged:
#711

Updated in d23001f.

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

* Add java client connection layer. Signed-off-by: Andrew Carbonetto <[email protected]> --------- Signed-off-by: Yury-Fridlyand <[email protected]> Signed-off-by: Andrew Carbonetto <[email protected]> Co-authored-by: Yury-Fridlyand <[email protected]> Co-authored-by: SanHalacogluImproving <[email protected]>

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

java/client/src/main/java/glide/api/RedisClient.java

java/client/src/test/java/glide/api/RedisClientCreateTest.java

shachlanAmazon

round

java/client/src/main/java/glide/api/BaseClient.java

java/client/src/main/java/glide/api/RedisClient.java

java/client/src/main/java/glide/api/models/configuration/BaseClientConfiguration.java

java/client/src/test/java/glide/api/RedisClientCreateTest.java

java/client/src/test/java/glide/managers/ConnectionManagerTest.java

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

java/client/src/main/java/glide/connectors/handlers/ChannelHandler.java

java/client/src/main/java/glide/api/RedisClient.java

java/client/src/main/java/glide/api/BaseClient.java

Yury-Fridlyand · 2024-01-06T00:40:45Z

java/client/src/main/java/glide/api/BaseClient.java

+      connectionManager.closeConnection().get();
+    } catch (InterruptedException interruptedException) {
+      // AutoCloseable functions are strongly advised to avoid throwing InterruptedExceptions
+      // TODO: marking resources as closed:


restore interrupt

I'm not sure this is right.
see: https://github.com/orgs/Bit-Quill/projects/4/views/6?pane=issue&itemId=48063887
Ultimately, it would be best to throw a RedisException/ClosingException after interrupting on all existing responses.

java/client/src/test/java/glide/api/RedisClientCreateTest.java

java/client/src/test/java/glide/managers/ConnectionManagerTest.java

java/src/lib.rs

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

Yury-Fridlyand

Minor things left. Do you have uncommited changes? I had to unresolve few things, because they are not fixed yet.

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

java/client/src/test/java/glide/api/RedisClientCreateTest.java

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

Yury-Fridlyand · 2024-01-10T04:15:43Z

java/client/src/main/java/glide/api/RedisClient.java

+    // TODO: Support exception throwing, including interrupted exceptions
+    return connectionManager
+        .connectToRedis(config)
+        .thenApply(ignore -> new RedisClient(connectionManager, commandManager));


Suggested change

.thenApply(ignore -> new RedisClient(connectionManager, commandManager));

.thenApplyAsync(ignore -> new RedisClient(connectionManager, commandManager));

Is it necessary to run this in a separate thread?

Yury-Fridlyand · 2024-01-10T04:16:57Z

java/client/src/main/java/glide/api/RedisClient.java

+    ChannelHandler channelHandler = buildChannelHandler();
+    ConnectionManager connectionManager = buildConnectionManager(channelHandler);
+    CommandManager commandManager = buildCommandManager(channelHandler);
+    // TODO: Support exception throwing, including interrupted exceptions


I think all exeptions are properly propagated from that async pipeline. They are wrapped by ExectionException. Please, re-check. Maybe you can remove this TODO and/or add a test.

We have a task to add exception handling. It would be 'nicer' if the API supported throwing RedisException, rather than an ExecutionException with a RedisException.
I'm pretty certain we can extend CompletableFutures to handle RedisException, in parallel with ExcutionExceptions and that would provide a better experience for our users.

Also, tests are already included in the follow-up PR, that adds RedisExceptions.

java/client/src/main/java/glide/connectors/handlers/ChannelHandler.java

Yury-Fridlyand · 2024-01-10T04:23:15Z

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

+    if (msg instanceof Response) {
+      Response response = (Response) msg;
+      callbackDispatcher.completeRequest(response);
+      ctx.fireChannelRead(msg);


why not fireChannelReadComplete?

Because that doesn't pass the message to other read handlers. That tells the read pipeline to complete, I believe (documentation on that method is somewhat lacking).

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

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

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

shachlanAmazon · 2024-01-11T17:30:00Z

java/client/src/main/java/glide/api/RedisClient.java

+   *
+   * @return a Future to connect and return a RedisClient
+   */
+  public static CompletableFuture<RedisClient> CreateClient() {


please don't add new public functions that don't exist in other wrappers without discussing it with us.

Removed in 0c7d03b

shachlanAmazon · 2024-01-11T17:36:07Z

java/client/src/main/java/glide/api/models/configuration/BaseClientConfiguration.java

+import lombok.experimental.SuperBuilder;
+
+/**
+ * Configuration settings class for creating a Redis Client. Shared settings for both a standalone


minor: correct me if I'm wrong, but isn't it "for both standalone [...]" with the "a"?

fixed in 0c7d03b

shachlanAmazon · 2024-01-11T18:23:04Z

java/client/src/test/java/glide/api/RedisClientCreateTest.java

+
+  @Test
+  @SneakyThrows
+  public void createClient_withConfig_successfullyReturnsRedisClient() {


I still don't see the value in these tests, but if you insist on having them 🤷

Understood. I think it's useful to have API unit tests that solidify the API contract with the user, even if it's through a static interface.

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

acarbonetto requested a review from a team as a code owner December 29, 2023 17:38

This was referenced Dec 29, 2023

[Closing...] Java: API layer with RedisClient and basic commands #734

Closed

Connection request protobuf creation from config Bit-Quill/valkey-glide#48

Closed

shachlanAmazon reviewed Dec 31, 2023

View reviewed changes

barshaul reviewed Jan 2, 2024

View reviewed changes

acarbonetto commented Jan 3, 2024

View reviewed changes

java/client/src/main/java/glide/managers/ConnectionManager.java Show resolved Hide resolved

acarbonetto and others added 9 commits January 3, 2024 11:13

Remove command calls from API

62836d8

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

Remove exception models from PR

9e95ce6

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

Update ConnectionManager to receive configuration

114ed94

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

Remove unwanted file

de3c4f3

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

Update JavaDocs and comments

1a31d3b

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

Update comments; Add CreateClient unit tests

2a9e6ff

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

Clean up RedisClient and ConnectionManager after mergre

75d2494

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

More clean up after merge

617ab3e

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

acarbonetto force-pushed the java/integ_acarbo_refactor_api_createclient branch from 09fd5c7 to 617ab3e Compare January 3, 2024 22:35

acarbonetto added 2 commits January 3, 2024 16:00

Fix failed connection test

1e1842c

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

Spotless fix

26dd364

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

acarbonetto commented Jan 4, 2024

View reviewed changes

shachlanAmazon reviewed Jan 4, 2024

View reviewed changes

Update CreateClient tests

3ae1f07

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

acarbonetto added the java issues and fixes related to the java client label Jan 5, 2024

acarbonetto added 2 commits January 4, 2024 16:59

Clean up tests and some minor comments

098122f

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

Cleaning up tests and adding TODOs

76b5cd7

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

Yury-Fridlyand reviewed Jan 6, 2024

View reviewed changes

Yury-Fridlyand mentioned this pull request Jan 6, 2024

Design documentation for adding a raw-FFI thread manager Bit-Quill/valkey-glide#31

Open

acarbonetto added 2 commits January 8, 2024 09:41

Revert lib.rs change

8092b62

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

Add comment to make connectionManager static

30a8d64

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

Yury-Fridlyand reviewed Jan 10, 2024

View reviewed changes

Spotless

6f1e3da

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

acarbonetto added 2 commits January 9, 2024 18:20

Apply default test name replacer

818a704

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

Update comments from review comments

8546d9f

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

Yury-Fridlyand reviewed Jan 10, 2024

View reviewed changes

Spotless

31bc4c5

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

acarbonetto force-pushed the java/integ_acarbo_refactor_api_createclient branch from 6fae6dc to 31bc4c5 Compare January 10, 2024 04:31

Add javadoc

9b02892

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

Yury-Fridlyand approved these changes Jan 10, 2024

View reviewed changes

Merge branch 'main' into java/integ_acarbo_refactor_api_createclient

1d18311

shachlanAmazon approved these changes Jan 11, 2024

View reviewed changes

acarbonetto added 2 commits January 11, 2024 12:40

Update for comments

0c7d03b

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

Spotless

572fc7b

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

acarbonetto merged commit 2f901b0 into valkey-io:main Jan 11, 2024
3 checks passed

acarbonetto deleted the java/integ_acarbo_refactor_api_createclient branch February 23, 2024 17:11


		CompletableFuture<Response> channel;

		public CompletableFuture<Void> closeConnection() {

	.thenApply(ignore -> new RedisClient(connectionManager, commandManager));
	.thenApplyAsync(ignore -> new RedisClient(connectionManager, commandManager));

Java: API layer with RedisClient #737

Java: API layer with RedisClient #737

Conversation

acarbonetto commented Dec 29, 2023 • edited Loading

Out of Scope

Use cases

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acarbonetto Jan 2, 2024 • 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

acarbonetto Jan 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shachlanAmazon Jan 2, 2024 • edited Loading

Choose a reason for hiding this comment

acarbonetto Jan 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acarbonetto Jan 2, 2024 • 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

acarbonetto Jan 2, 2024 • edited Loading

Choose a reason for hiding this comment

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

acarbonetto commented Dec 29, 2023 •

edited

Loading

acarbonetto Jan 2, 2024 •

edited

Loading

acarbonetto Jan 2, 2024 •

edited

Loading

shachlanAmazon Jan 2, 2024 •

edited

Loading

acarbonetto Jan 2, 2024 •

edited

Loading

acarbonetto Jan 2, 2024 •

edited

Loading

acarbonetto Jan 2, 2024 •

edited

Loading