Add managers layer to client #44

Yury-Fridlyand · 2023-12-15T23:55:14Z

Part 2 of client implementation. See part 1 in #43/valkey-io#670

The tracking task: https://github.com/orgs/Bit-Quill/projects/4/views/9?pane=issue&itemId=48023248

Proposed usage in Client class:

package babushka.api;

import babushka.connectors.handlers.CallbackDispatcher;
import babushka.connectors.handlers.ChannelHandler;
import babushka.ffi.resolvers.SocketListenerResolver;
import babushka.managers.CommandManager;
import babushka.managers.ConnectionManager;
import java.util.concurrent.atomic.AtomicBoolean;

public class Client {

  public Client() {

    AtomicBoolean connectionStatus = new AtomicBoolean(false);

    CallbackDispatcher callbackDispatcher = new CallbackDispatcher(connectionStatus);
    ChannelHandler channelHandler =
        new ChannelHandler(callbackDispatcher, SocketListenerResolver.getSocket());
    var connectionManager = new ConnectionManager(channelHandler, connectionStatus);
    var commandManager = new CommandManager(channelHandler);
  }
}

And Awaiter:

package babushka.api;

import java.util.concurrent.CancellationException;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;

public class Awaiter {
  private static final long DEFAULT_TIMEOUT_MILLISECONDS = 30000;

  /** Get the future result with default timeout. */
  public static <T> T await(CompletableFuture<T> future) {
    return await(future, DEFAULT_TIMEOUT_MILLISECONDS);
  }

  /** Get the future result with given timeout in ms. */
  public static <T> T await(CompletableFuture<T> future, long timeout) {
    try {
      return future.get(timeout, TimeUnit.MILLISECONDS);
    } catch (ExecutionException e) {
      throw new RuntimeException(e.getMessage(), e.getCause());
    } catch (InterruptedException e) {
      // TODO should shutdown the client service
      if (Thread.currentThread().isInterrupted()) {
        // restore interrupt
        Thread.interrupted();
      }
      throw new RuntimeException("The thread was interrupted", e);
    } catch (TimeoutException e) {
      throw new RuntimeException("Request timed out", e);
    } catch (CancellationException e) {
      throw new RuntimeException("Request was cancelled", e);
    }
  }
}

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

acarbonetto · 2023-12-18T20:45:55Z

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

+
+  private final ClientState.ReadOnlyClientState clientState;
+
+  /** Unique request ID (callback ID). Thread-safe and overflow-safe. */
  private final AtomicInteger requestId = new AtomicInteger(0);


Should we not use CONNECTION_PROMISE_ID in the constructor?

It is ok, since registerConnection calls to registerRequest and share the same future array

acarbonetto · 2023-12-18T20:46:18Z

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

    var future = new CompletableFuture<Response>();
    responses.put(callbackId, future);
    return Pair.of(callbackId, future);
  }

  public CompletableFuture<Response> registerConnection() {
-    return connectionPromise;
+    var res = registerRequest();
+    if (res.getKey() != CONNECTION_PROMISE_ID) {


Is this necessary?

Yes, to protect vs re-using a client for another connection

acarbonetto · 2023-12-18T20:47:29Z

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

-    if (callbackId == 0) {
-      connectionPromise.completeAsync(() -> response);
+    int callbackId =
+        clientState.isInitializing() ? response.getCallbackIdx() : CONNECTION_PROMISE_ID;


add a comment to say that connection responses don't return a callbackIdx

Added in 647d802.

acarbonetto · 2023-12-18T20:48:01Z

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

-      connectionPromise.completeAsync(() -> response);
+    int callbackId =
+        clientState.isInitializing() ? response.getCallbackIdx() : CONNECTION_PROMISE_ID;
+    var future = responses.get(callbackId);


can we avoid using var?

here or everywhere?

acarbonetto · 2023-12-18T20:51:25Z

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

-      responses.get(callbackId).completeAsync(() -> response);
-      responses.remove(callbackId);
+      // TODO: log an error.
+      // probably a response was received after shutdown or `registerRequest` call was missing


This should only really happen after shutdown...? should we check the state and just log that a response was completed.

It is a part of logging task

acarbonetto · 2023-12-18T20:58:03Z

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

+   *
+   * @param key The key name
+   */
+  public CompletableFuture<String> get(String key) {


We can remove this - we only need submitNewRequest

Which class should call get/set? Should I do submitNewRequest public then?

acarbonetto · 2023-12-18T20:58:10Z

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

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


acarbonetto · 2023-12-18T20:59:33Z

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

+    } else if (response.hasRespPointer()) {
+      return RedisValueResolver.valueFromPointer(response.getRespPointer()).toString();
+    }
+    return null;


if this doesn't happen, we should throw Exception instead

Fixed in 647d802

acarbonetto · 2023-12-18T21:00:10Z

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

+          "Unexpected response data: "
+              + RedisValueResolver.valueFromPointer(response.getRespPointer()));
+    }
+    return false;


can this happen?

Only if something went completely wrong on rust side. Surprising, protobuf allows to build such response object with no complains.

Changed in 647d802

acarbonetto · 2023-12-18T21:00:49Z

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

+import redis_request.RedisRequestOuterClass.Routes;
+import redis_request.RedisRequestOuterClass.SimpleRoutes;
+
+public class RequestBuilder {


I think this should be RedisRequestBuilder

I disagree, we have 2 types of requests: RedisRequest and ConnectionRequest and this guy builds both.

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

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

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

acarbonetto · 2023-12-21T18:33:00Z

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

+    synchronized (responses) {
+      if (callbackId == null) {
+        long size = responses.mappingCount();
+        callbackId = (int) (size < Integer.MAX_VALUE ? size : -(size - Integer.MAX_VALUE));


maybe just protect if this wraps around and re-creates zero... but this scenario would be rare and seriously bad.

Yury-Fridlyand requested review from jduo, cdaudt, jonathanl-bq, acarbonetto, Elen-Ghulam and SanHalacogluImproving December 15, 2023 23:55

Yury-Fridlyand force-pushed the java/integ_yuryf_client_part2 branch from ff63182 to 12dcaba Compare December 16, 2023 00:16

Managers

1bff530

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

Yury-Fridlyand force-pushed the java/dev_yuryf_client_part2 branch from 3b5d20b to 1bff530 Compare December 16, 2023 00:17

Yury-Fridlyand added 2 commits December 15, 2023 19:42

Refactor

e8fe498

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

Split ClientState and Platform.

e5281ca

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

acarbonetto reviewed Dec 18, 2023

View reviewed changes

Yury-Fridlyand added 4 commits December 18, 2023 17:34

Refactor

3c786c6

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

Address PR feedback.

647d802

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

Minor fix.

e5a4e10

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

Typo fix.

11fae77

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

Yury-Fridlyand commented Dec 19, 2023

View reviewed changes

java/client/src/main/java/babushka/connectors/handlers/CallbackDispatcher.java Outdated Show resolved Hide resolved

Yury-Fridlyand added 2 commits December 20, 2023 13:42

Refactor

3f394c6

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

javadocs

7b415da

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

acarbonetto approved these changes Dec 21, 2023

View reviewed changes

Yury-Fridlyand merged commit 68b42a5 into java/integ_yuryf_client_part2 Dec 21, 2023
3 checks passed

Yury-Fridlyand deleted the java/dev_yuryf_client_part2 branch December 21, 2023 18:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add managers layer to client #44

Add managers layer to client #44

Yury-Fridlyand commented Dec 15, 2023 •

edited

Loading

acarbonetto Dec 18, 2023

Yury-Fridlyand Dec 19, 2023

acarbonetto Dec 18, 2023

Yury-Fridlyand Dec 19, 2023

acarbonetto Dec 18, 2023

Yury-Fridlyand Dec 19, 2023

acarbonetto Dec 18, 2023

Yury-Fridlyand Dec 19, 2023

acarbonetto Dec 18, 2023

Yury-Fridlyand Dec 19, 2023

acarbonetto Dec 18, 2023

Yury-Fridlyand Dec 19, 2023

acarbonetto Dec 18, 2023

acarbonetto Dec 18, 2023

Yury-Fridlyand Dec 19, 2023

acarbonetto Dec 18, 2023

Yury-Fridlyand Dec 19, 2023

Yury-Fridlyand Dec 19, 2023

acarbonetto Dec 18, 2023

Yury-Fridlyand Dec 19, 2023

acarbonetto Dec 21, 2023

Add managers layer to client #44

Add managers layer to client #44

Conversation

Yury-Fridlyand commented Dec 15, 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

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

Yury-Fridlyand commented Dec 15, 2023 •

edited

Loading