Java - Add RedisException return to connection/command response handl… #788

acarbonetto · 2024-01-12T23:31:02Z

Depends on #717.

Issue #, if available:

API layer for: #589

Description of changes:

In this PR, we're adding RedisException throwing when

Creating a RedisClient and the connection response contains non-success.
Sending a Redis Request and request response contains non-success.

Use Case:

CompletableFuture<RedisClient> createClientRequest = RedisClient.CreateClient(config); 

// throws ExecutionException with a RedisException on an unexpected failure
try (RedisClient client = createClientRequest.get()) 
{
    CompletableFuture<String> getRequest = client.get("key"); 
    String getValue = getRequest.get(); // throws ExecutionException with a RedisException on an unexpected failure
} catch (ExecutionException executionException) {
    if (executionException.getCause() instanceOf RedisException) {
         // try again
  }
}

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

…ing (#57) * Add Redis exceptions and handling Signed-off-by: Andrew Carbonetto <[email protected]> --------- Signed-off-by: Andrew Carbonetto <[email protected]>

Yury-Fridlyand · 2024-01-12T23:45:28Z

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

  public CompletableFuture<Void> closeConnection() {
-    return CompletableFuture.runAsync(channel::close);
+    CompletableFuture<Void> completableFuture = new CompletableFuture<>();
+    channel


You can update this function to return Future<Void> to avoid such ugly conversion F -> CF

shachlanAmazon · 2024-01-13T21:24:33Z

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

-      // TODO: marking resources as closed:
-      // https://github.com/orgs/Bit-Quill/projects/4/views/6?pane=issue&itemId=48063887
-      throw new RuntimeException(interruptedException);
+    } catch (InterruptedException e) {


I'm sorry I didn't notice earlier - we use 4 spaces throughout the project:
https://github.com/aws/glide-for-redis/blob/2f901b0617fe1cce76de8449f9fc28ba30029c2d/.vscode/settings.json#L3
please fix your linter in a separate PR.

shachlanAmazon · 2024-01-13T21:32:35Z

java/client/src/main/java/glide/api/models/exceptions/ClosingException.java

@@ -0,0 +1,8 @@
+package glide.api.models.exceptions;
+
+/** Error returned from Redis client: Redis is closing or unavailable to the client */


Please don't invent new documentation, just use the existing comments used in python / TS. Same for all exception types.

fixed under 37fee4a

shachlanAmazon · 2024-01-14T07:13:31Z

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

    }
-    throw new RuntimeException("Connection response expects an OK response");
+    // successful connection response has an "OK"


the comment doesn't match the return type. what's intention behind the doc?

shachlanAmazon · 2024-01-14T07:52:09Z

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

  }

-  /** Close the connection and the corresponding channel. */
+  /** Close the connection to the channel. */


comment on nothing, repeating the next comment

shachlanAmazon · 2024-01-14T08:25:20Z

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

@@ -162,28 +166,51 @@ private ConnectionRequestOuterClass.ReadFrom mapReadFromEnum(ReadFrom readFrom)
  /** Check a response received from Glide. */
  private Void checkGlideRsResponse(Response response) {
    if (response.hasRequestError()) {


minor:
considering we have 4 repeats of

closeConnection(); throw new ClosingException(<some message>);

maybe just invert the structure of the function:

private Void checkGlideRsResponse(Response response) { if (response.hasConstantResponse()) { return null; } var message; [...] // set message by the response type closeConnection(); throw new ClosingException(message); }

I can't trust that there is no error in the response.
The object could have both an error and a constant response. So I have to check for an error regardless.

Consolidated the logic in 37fee4a

I can't trust that there is no error in the response.

Why not?
I understand that Protobuf doesn't give an interface that promises that, but why can't you trust the core to set only one of the values?

Best practices. I just don't know how core (and any interactive system) would behave in the future. And this provides an extra layer of protection for the user. I'm not sure the saving in processing time here are worth the potential costs of exposing a bug down the road.

shachlanAmazon · 2024-01-14T08:53:33Z

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

@@ -84,7 +91,6 @@ public void ConnectionRequestProtobufGeneration_DefaultRedisClientConfiguration_
    // verify
    // no exception
    assertNull(result.get());
-    verify(channel).connect(eq(expectedProtobufConnectionRequest));


what does the test actually test without this?

Pass without exception.

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

Java - Add RedisException return to connection/command response handl…

2ef4f38

…ing (#57) * Add Redis exceptions and handling Signed-off-by: Andrew Carbonetto <[email protected]> --------- Signed-off-by: Andrew Carbonetto <[email protected]>

acarbonetto requested a review from a team as a code owner January 12, 2024 23:31

acarbonetto self-assigned this Jan 12, 2024

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

Yury-Fridlyand reviewed Jan 12, 2024

View reviewed changes

shachlanAmazon approved these changes Jan 14, 2024

View reviewed changes

acarbonetto added 4 commits January 15, 2024 09:20

Fix comments for review

37fee4a

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

Spotless

e8da8e5

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

spotless

fbddc6f

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

Close connection can return a Future

0ad55d5

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

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

acarbonetto deleted the java/integ_acarbo_api_redisExceptions branch January 16, 2024 02:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Java - Add RedisException return to connection/command response handl… #788

Java - Add RedisException return to connection/command response handl… #788

acarbonetto commented Jan 12, 2024

Yury-Fridlyand Jan 12, 2024 •

edited

Loading

shachlanAmazon Jan 13, 2024

shachlanAmazon Jan 13, 2024

acarbonetto Jan 15, 2024

shachlanAmazon Jan 14, 2024

shachlanAmazon Jan 14, 2024

shachlanAmazon Jan 14, 2024

acarbonetto Jan 15, 2024

acarbonetto Jan 15, 2024 •

edited

Loading

shachlanAmazon Jan 15, 2024

acarbonetto Jan 15, 2024

shachlanAmazon Jan 14, 2024

acarbonetto Jan 15, 2024

		@@ -0,0 +1,8 @@
		package glide.api.models.exceptions;

		/** Error returned from Redis client: Redis is closing or unavailable to the client */

Java - Add RedisException return to connection/command response handl… #788

Java - Add RedisException return to connection/command response handl… #788

Conversation

acarbonetto commented Jan 12, 2024

Yury-Fridlyand Jan 12, 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 15, 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

Yury-Fridlyand Jan 12, 2024 •

edited

Loading

acarbonetto Jan 15, 2024 •

edited

Loading