Java client: update error handling #865

Yury-Fridlyand · 2024-01-27T00:59:51Z

Move error handling to CallbackDispatcher
Handle case of incorrect callback ID with ClosingError (ref: Rust core returns incorrect callback ID on connection failure #851)
Close channel on ClosingError
Tests

* Add exception handling and tests. Signed-off-by: Yury-Fridlyand <[email protected]> * Update tests. Signed-off-by: Yury-Fridlyand <[email protected]> * spotless Signed-off-by: Yury-Fridlyand <[email protected]> * typo Signed-off-by: Yury-Fridlyand <[email protected]> * Address PR review comments. Signed-off-by: Yury-Fridlyand <[email protected]> --------- Signed-off-by: Yury-Fridlyand <[email protected]>

shachlanAmazon · 2024-01-28T07:21:27Z

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

@@ -63,18 +64,17 @@ public void setUp() throws ExecutionException, InterruptedException {

    @SneakyThrows
    @Test
-    public void ConnectionRequestProtobufGeneration_DefaultRedisClientConfiguration_returns() {
+    public void connection_request_protobuf_generation_default_standalone_configuration() {


why are you modifying the casing scheme of the tests, and will this affect other PRs? will the new scheme be enforced with a linter?

If it is, please spin large stylistic changes to another PR. If it isn't, why bother?

why are you modifying the casing scheme

https://github.com/aws/glide-for-redis/blob/e39f4ae12d1583a1d62d6cbce4aba5966ff270b8/java/client/src/test/resources/junit-platform.properties#L2
This thing replaces underscores by spaces, which makes test report more readable. There is no similar feature for CaMel casing.

will the new scheme be enforced with a linter?

no, both function names are valid (at least for a test cases)

spin large stylistic changes to another PR

We have only couple of files with UT, so all changes are here. I still open to move it another PR.

Reverted in 794833e

shachlanAmazon · 2024-01-28T07:23:39Z

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

@@ -41,11 +41,11 @@ public ChannelHandler(CallbackDispatcher callbackDispatcher, String socketPath)
    /**
     * Open a new channel for a new client and running it on the provided EventLoopGroup
     *
-     * @param eventLoopGroup - ELG to run handler on


if you have multiple lines of pure style changes, please spin them out to a separate PR.

Reverted in 794833e

shachlanAmazon · 2024-01-28T07:56:35Z

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

 import response.ResponseOuterClass.Response;

 /** Holder for resources required to dispatch responses and used by {@link ReadHandler}. */
 @RequiredArgsConstructor
 public class CallbackDispatcher {

    /** Unique request ID (callback ID). Thread-safe and overflow-safe. */
-    private final AtomicInteger nextAvailableRequestId = new AtomicInteger(0);
+    protected final AtomicInteger nextAvailableRequestId = new AtomicInteger(0);


why the change?

We create TestCallbackDispatcher (which inherits CallbackDispatcher) for tests. Access to protected fields is required to verify correctness of the data processing.
This is true for other classes too where access modifier was changed.

shachlanAmazon · 2024-01-28T08:52:08Z

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

            future.completeAsync(() -> response);
        } else {
            // TODO: log an error.
            // probably a response was received after shutdown or `registerRequest` call was missing
+
+            System.err.printf(


if this happens, the client is in an erroneous state and should close.

Added in 794833e

shachlanAmazon · 2024-01-28T08:52:22Z

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

@@ -24,8 +24,8 @@ public class ChannelHandler {

    private static final String THREAD_POOL_NAME = "glide-channel";

-    private final Channel channel;
-    private final CallbackDispatcher callbackDispatcher;
+    protected final Channel channel;


why was this changed?

For tests, ref #865 (comment)

shachlanAmazon · 2024-01-28T09:39:41Z

java/client/src/test/java/glide/ExceptionHandlingTests.java

+
+        var protobufErrorsToJavaClientErrors =
+                Map.of(
+                        Unspecified, RequestException.class,


this list is nice, but what mechanism ensures that if there's a new exception type the list will update? Can't we iterate over the types using reflection?

We can iterate with reflection, but mapping is still hardcoded. I'll try to make mapping as a class property to avoid duplicating it in tests.

Reflection is ugly and requires additional error handling. For nice mapping we should update the enum, but it is managed by protobuf.
I keep this unchanged.

aaron-congo · 2024-01-29T17:09:24Z

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

            future.completeAsync(() -> response);
        } else {
            // TODO: log an error.
            // probably a response was received after shutdown or `registerRequest` call was missing
+
+            System.err.printf(
+                    "Received a response for not registered callback id %d, request error = %s%n",


Is this supposed to be \n instead of %n?

Suggested change

"Received a response for not registered callback id %d, request error = %s%n",

"Received a response for not registered callback id %d, request error = %s\n",

Not a mistake - %n is rendered as \n. Thank you for proofreading though.

aaron-congo · 2024-01-29T17:11:18Z

java/client/src/test/java/glide/ExceptionHandlingTests.java

+            callbackDispatcher.completeRequest(response);
+
+            var exception = assertThrows(ExecutionException.class, future::get);
+            // a ClosingException thrown from CallbackDispatcher::completeRequest and then


Suggested change

// a ClosingException thrown from CallbackDispatcher::completeRequest and then

// a RedisException thrown from CallbackDispatcher::completeRequest and then

Thanks fixed in 794833e

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

Yury-Fridlyand · 2024-01-29T23:29:54Z

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

            future.completeAsync(() -> response);
        } else {
            // TODO: log an error.
            // probably a response was received after shutdown or `registerRequest` call was missing
+
+            System.err.printf(


Added in 794833e

Yury-Fridlyand · 2024-01-29T23:31:07Z

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

            future.completeAsync(() -> response);
        } else {
            // TODO: log an error.
            // probably a response was received after shutdown or `registerRequest` call was missing
+
+            System.err.printf(
+                    "Received a response for not registered callback id %d, request error = %s%n",


Not a mistake - %n is rendered as \n. Thank you for proofreading though.

Yury-Fridlyand · 2024-01-29T23:31:17Z

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

@@ -41,11 +41,11 @@ public ChannelHandler(CallbackDispatcher callbackDispatcher, String socketPath)
    /**
     * Open a new channel for a new client and running it on the provided EventLoopGroup
     *
-     * @param eventLoopGroup - ELG to run handler on


Reverted in 794833e

Yury-Fridlyand · 2024-01-29T23:32:35Z

java/client/src/test/java/glide/ExceptionHandlingTests.java

+
+        var protobufErrorsToJavaClientErrors =
+                Map.of(
+                        Unspecified, RequestException.class,


Reflection is ugly and requires additional error handling. For nice mapping we should update the enum, but it is managed by protobuf.
I keep this unchanged.

Yury-Fridlyand · 2024-01-29T23:32:55Z

java/client/src/test/java/glide/ExceptionHandlingTests.java

+            callbackDispatcher.completeRequest(response);
+
+            var exception = assertThrows(ExecutionException.class, future::get);
+            // a ClosingException thrown from CallbackDispatcher::completeRequest and then


Thanks fixed in 794833e

Yury-Fridlyand · 2024-01-29T23:33:06Z

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

@@ -71,7 +71,7 @@ public void createClient_withConfig_successfullyReturnsRedisClient() {

    @SneakyThrows
    @Test
-    public void createClient_errorOnConnectionThrowsExecutionException() {
+    public void createClient_error_on_connection_throws_ExecutionException() {


Reverted in 794833e

Yury-Fridlyand · 2024-01-29T23:33:16Z

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

-    public void submitNewCommand_returnObjectResult()
-            throws ExecutionException, InterruptedException {
+    @SneakyThrows
+    public void submitNewCommand_return_Object_result() {


Reverted in 794833e

Yury-Fridlyand · 2024-01-29T23:33:26Z

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

@@ -63,18 +64,17 @@ public void setUp() throws ExecutionException, InterruptedException {

    @SneakyThrows
    @Test
-    public void ConnectionRequestProtobufGeneration_DefaultRedisClientConfiguration_returns() {
+    public void connection_request_protobuf_generation_default_standalone_configuration() {


Reverted in 794833e

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

shachlanAmazon · 2024-01-31T12:53:25Z

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

+            // According to https://github.com/aws/glide-for-redis/issues/851
+            // a response with a closing error may arrive with any/random callback ID (usually -1)
+            // CommandManager and ConnectionManager would close the UDS channel on ClosingException
+            responses


minor: consider wrapping this repeated code in a function, and in that function also close the channel.

or maybe I'm missing where the channel is closed?

nvm, saw that closing the channel happens in the exception handler.

I tried to make all error handling in one place, including channel closing, but CallbackDispatcher does not (and can not) have refenrence to the channel.

Updated in 0dbec7b. Will use new function to handle read error too in the next PR.

shachlanAmazon · 2024-01-31T12:55:07Z

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

+            System.err.printf(
+                    "Received a response for not registered callback id %d, request error = %s%n",
+                    callbackId, response.getRequestError());
+            responses


you cancel all of the responses, but you should also close the channel.

nvm, saw that it's happening in the exception handler.

shachlanAmazon · 2024-01-31T13:02:38Z

java/client/src/test/java/glide/ExceptionHandlingTests.java

+    @Test
+    public void verify_future_pipeline_abortion() {
+        var future = new CompletableFuture<Boolean>();
+        var future2 = future.exceptionally(e -> false).thenApplyAsync(r -> r ? 42 : 100500);


what are you testing here? that the Future.exceptionally function works correctly?

Yes, I show the exception handling pipeline in one place. In client it is distributed over multiple classes.

shachlanAmazon · 2024-01-31T15:56:59Z

java/client/src/test/java/glide/ExceptionHandlingTests.java

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


"handle" is an anti-pattern in test names, because it makes it hard to understand whether the test is correct or not. It's preferable that the test should say what happens -
callback_dispatcher_closes_connection_on_response_without_error_but_with_incorrect_callback_id

Renamed in 0dbec7b + added a comment

The fully correct description is too long, because callback dispatcher does not close the channel, it forces connection manager and command manager to do that.

shachlanAmazon · 2024-01-31T15:59:13Z

java/client/src/test/java/glide/ExceptionHandlingTests.java

+        var exception = assertThrows(ExecutionException.class, future1::get);
+        // a ClosingException thrown from CallbackDispatcher::completeRequest and then
+        // rethrown by CommandManager::exceptionHandler
+        assertTrue(exception.getCause() instanceof ClosingException);


you might want to offload the checks for closed connections to a function, instead of repeating the checks in all closing tests.

shachlanAmazon · 2024-01-31T15:59:50Z

java/client/src/test/java/glide/ExceptionHandlingTests.java

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


same - instead of "handle", say what's expected to happen

callback_dispatcher_closes_channel_and_throws_ClosingException
or
callback_dispatcher_with_ClosingError_closes_channel_and_throws_ClosingException

Renamed in 0dbec7b + added a comment

shachlanAmazon · 2024-01-31T15:59:57Z

java/client/src/test/java/glide/ExceptionHandlingTests.java

+    @ParameterizedTest(name = "{0}")
+    @MethodSource("getProtobufErrorsToJavaClientErrorsMapping")
+    @SneakyThrows
+    public void callback_dispatcher_handles_response_with_request_error(


callback_dispatcher_does_not_close_channel_and_throws_RedisException
or
callback_dispatcher_with_RequestError_does_not_close_channel_and_throws_RedisException

Renamed in 0dbec7b + added a comment

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

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

Yury-Fridlyand added the java issues and fixes related to the java client label Jan 27, 2024

Yury-Fridlyand requested review from asafpamzn and shachlanAmazon January 27, 2024 00:59

Yury-Fridlyand requested a review from a team as a code owner January 27, 2024 00:59

shachlanAmazon reviewed Jan 28, 2024

View reviewed changes

aaron-congo reviewed Jan 29, 2024

View reviewed changes

acarbonetto approved these changes Jan 29, 2024

View reviewed changes

Address PR feedback.

794833e

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

Yury-Fridlyand commented Jan 29, 2024

View reviewed changes

aaron-congo approved these changes Jan 30, 2024

View reviewed changes

acarbonetto mentioned this pull request Jan 30, 2024

Implementing Customizable Event Loop Groups and Domain Socket Channels for Netty Channel Configuration #861

Merged

Minor test update.

4a69077

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

shachlanAmazon approved these changes Jan 31, 2024

View reviewed changes

shachlanAmazon reviewed Jan 31, 2024

View reviewed changes

Yury-Fridlyand added 5 commits January 31, 2024 09:52

Merge branch 'main' into java/integ_yuryf_closing_error

f4cb71a

Rename tests.

0dbec7b

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

Add IT.

34a84a1

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

Update IT for different redis versions and different OS.

769ab53

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

Merge remote-tracking branch 'upstream/main' into java/integ_yuryf_cl…

49a7eed

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

Yury-Fridlyand merged commit 888109e into valkey-io:main Feb 1, 2024
11 checks passed

Yury-Fridlyand deleted the java/integ_yuryf_closing_error branch February 1, 2024 01:50

	"Received a response for not registered callback id %d, request error = %s%n",
	"Received a response for not registered callback id %d, request error = %s\n",

	// a ClosingException thrown from CallbackDispatcher::completeRequest and then
	// a RedisException thrown from CallbackDispatcher::completeRequest and then

Java client: update error handling #865

Java client: update error handling #865

Conversation

Yury-Fridlyand commented Jan 27, 2024

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

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 Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acarbonetto Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acarbonetto Jan 31, 2024 •

edited

Loading

acarbonetto Jan 31, 2024 •

edited

Loading