Add cluster client, request routes configuration and support for bulk response #59
Conversation
Yury-Fridlyand
requested review from
aaron-congo,
jonathanl-bq,
acarbonetto,
Elen-Ghulam and
SanHalacogluImproving
java/client/src/main/java/glide/api/models/configuration/RedisValue.java
Outdated
Show resolved
Hide resolved
java/client/src/main/java/glide/api/models/configuration/RedisValue.java
Outdated
Show resolved
Hide resolved
java/client/src/main/java/glide/api/models/configuration/RedisValue.java
Outdated
Show resolved
Hide resolved
java/client/src/main/java/glide/api/models/configuration/RedisValue.java
Outdated
Show resolved
Hide resolved
| * <li>{@link SlotIdRoute} | ||
| * </ul> | ||
| */ | ||
| public interface Routes {} |
There was a problem hiding this comment.
as much as I prefer this layout, we should instead follow the existing format of their configurations in Python/Node.
e.g.
https://github.com/Bit-Quill/glide-for-redis/blob/main/node/src/RedisClusterClient.ts#L51
There was a problem hiding this comment.
I followed protobuf definitions.
There was a problem hiding this comment.
This would be inconsistent with how we're going to implement the SetOptions
There was a problem hiding this comment.
You are going to have to refactor a lot of code if you don't rebase against #49.
For example, I'm not sure the changes in CommandManager and CusterCommandManager will be necessary, as the get/set calls were removed from these classes.
| * <li>{@link SlotIdRoute} | ||
| * </ul> | ||
| */ | ||
| public interface Routes {} |
There was a problem hiding this comment.
This would be inconsistent with how we're going to implement the SetOptions
Yury-Fridlyand
force-pushed
the
java/dev_yuryf_cluster_bulk_resp
branch
from
8eaf785 to
b7f9bbd
Compare
| * @return a CompletableFuture with response result from Redis | ||
| */ | ||
| CompletableFuture<Object> customCommand(String[] args); | ||
| CompletableFuture<T> customCommand(String[] args); |
There was a problem hiding this comment.
The whole point of customCommand is that we won't know the return type. We should return Object in all cases.
There was a problem hiding this comment.
For customCommand:
T = Object for standalone client
T = RedisValue for cluster client
There was a problem hiding this comment.
It would be better to duplicate. Why don't we just implement BaseCommands for the RedisClient, and ClusterBaseCommands for the RedisClusterClient? I don't see the value to using a generic here when there are only two possible options.
There was a problem hiding this comment.
Actually, more than two, becase now T is RedisValue<V> for cluster client. So having T we avoid code duplicating N times for sake of maintanability and so on.
| * | ||
| * @param <T> The data return type. | ||
| */ | ||
| public interface BaseCommands<T> { |
There was a problem hiding this comment.
BaseCommands don't have return types. The RedisValue return type would only be value for cluster mode, standalone mode should have a type t defined.
We should show how this works for both cluster mode and standalone mode. Maybe also prototype how this would work with a string handler, which returns a RedisValue...?
| @Getter | ||
| public class RedisValue { | ||
| /** Get per-node value. */ | ||
| private Map<String, Object> multiValue = null; |
There was a problem hiding this comment.
In some cases, we should know what the return type would be. For example, for CONFIG GET, we know we get a Map back from Redis for each node.
The single value would also be a Map. We should include a type in this object that defines the return type for single Value or the value type of the map for multiValue.
There was a problem hiding this comment.
Consider the work we put together under:
java/integ_acarbo_api_baseCommands...Bit-Quill:glide-for-redis:java/dev_acabo_cluster_rework_with_examples
There was a problem hiding this comment.
Reworked, RedisValue has a type parameter as of now
|
|
||
| public enum RouteType { | ||
| /** Route request to all nodes. */ | ||
| AllNodes, |
There was a problem hiding this comment.
Feels like we should follow the convention to capitalize all enums. No?
| @AllArgsConstructor | ||
| public abstract class BaseClient implements AutoCloseable { | ||
| public abstract class BaseClient<T> implements AutoCloseable { |
There was a problem hiding this comment.
Why is the type T attached/defined in the client. Would the type be defined per-request command, and not for the entire client? I'd like to see how this works for both standalone and cluster commands when the type returned is known (not customCommand).
|
|
||
| /** | ||
| * Extracts the response from the Protobuf response and either throws an exception or returns the | ||
| * appropriate response has an Object |
There was a problem hiding this comment.
small typo
| * appropriate response has an Object | |
| * appropriate response as an Object |
| * | ||
| * @param <T> The wrapped data type | ||
| */ | ||
| @Getter |
There was a problem hiding this comment.
should we use a straight getter here, or does it make better sense to throw an exception if the wrong call is made?
| @@ -58,6 +58,7 @@ public ChannelHandler( | |||
| .channel(domainSocketChannelClass) | |||
| .handler(channelInitializer) | |||
| .connect(domainSocketAddress) | |||
| .syncUninterruptibly() | |||
There was a problem hiding this comment.
Rebased on main, where sync is used
| * @return A result promise of type T | ||
| */ | ||
| public <T> CompletableFuture<T> submitNewCommand( | ||
| Command command, RedisExceptionCheckedFunction<Response, T> responseHandler, Route route) { |
There was a problem hiding this comment.
I think it looks cleaner if the route is before the responseHandler?
| * @param route node routing configuration for the command | ||
| * @return CompletableFuture with the response | ||
| */ | ||
| CompletableFuture<T> customCommand(String[] args, Route route); |
There was a problem hiding this comment.
Why doesn't this return CompletableFuture<RedisValue<Object>>?
There was a problem hiding this comment.
Because cluster client implements this interface with
@Override
public CompletableFuture<RedisValue<Object>> customCommand(String[] args)
...| * @return a CompletableFuture with response result from Redis | ||
| */ | ||
| CompletableFuture<Object> customCommand(String[] args); | ||
| CompletableFuture<T> customCommand(String[] args); |
There was a problem hiding this comment.
It would be better to duplicate. Why don't we just implement BaseCommands for the RedisClient, and ClusterBaseCommands for the RedisClusterClient? I don't see the value to using a generic here when there are only two possible options.
| * @param <T> The wrapped data type | ||
| */ | ||
| @Getter | ||
| public class RedisValue<T> { |
There was a problem hiding this comment.
I'm not sure if we've discussed this already (I feel like we may have), but would ClusterValue be a better name? It seems we don't use this for standalone clients.
Signed-off-by: Yury-Fridlyand <[email protected]>
Yury-Fridlyand
force-pushed
the
java/dev_yuryf_cluster_bulk_resp
branch
from
dab3bc3 to
2639c41
Compare
Signed-off-by: Yury-Fridlyand <[email protected]>
Yury-Fridlyand
force-pushed
the
java/integ_yuryf_cluster_bulk_resp
branch
from
b2e7b60 to
9345634
Compare
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
| * Async (non-blocking) client for Redis in Cluster mode. Use {@link #CreateClient} to request a | ||
| * client to Redis. | ||
| */ | ||
| public class ClusterClient extends BaseClient implements ClusterBaseCommands<ClusterValue<Object>> { |
| * | ||
| * @param <T> The data return type. | ||
| */ | ||
| public interface ClusterBaseCommands<T> extends BaseCommands<T> { |
There was a problem hiding this comment.
I'd need to see how this will work for StringCommands, HashCommands, etc.
I think this model is overly complex just to save a single line in an interface. And if we have several types (StringCommands will need around 4 generic types defined) this starts to look really ugly.
Signed-off-by: Yury-Fridlyand <[email protected]>
| // return function to convert protobuf.Response into the response object by | ||
| // calling valueFromPointer | ||
| return (new BaseCommandResponseResolver(RedisValueResolver::valueFromPointer)).apply(response); | ||
| protected Object handleObjectResponse(Response response) { |
| * @param responseHandler The handler for the response object | ||
| * @return A result promise of type T | ||
| */ | ||
| public <T> CompletableFuture<T> submitNewCommand( |
There was a problem hiding this comment.
Is it better to using Optional(Route) and avoid overloading and duplicating code?
| import java.util.Map; | ||
|
|
||
| /** | ||
| * A union-like type which can store single or bulk value retrieved from Redis. |
There was a problem hiding this comment.
| * A union-like type which can store single or bulk value retrieved from Redis. | |
| * A union-like type which can store single-value or multi-value retrieved from Redis. The multi-value, if defined, contains the routed value as a Map<String, Object> containing a cluster node address to cluster node value. |
|
|
||
| /** Get the single value. */ | ||
| public T getSingleValue() { | ||
| assert !hasMultiData(); |
There was a problem hiding this comment.
nit: is it worth creating a hasSingleData() method?
|
|
||
| private ClusterValue() {} | ||
|
|
||
| /** Get per-node value. */ |
There was a problem hiding this comment.
We will need a more explicit javadoc if we're going to expose this to the user.
Please mention that hasMultiValue() should be called first to avoid exceptions
| } | ||
|
|
||
| /** Get the single value. */ | ||
| public T getSingleValue() { |
There was a problem hiding this comment.
javadoc - please mention that hasSingleValue() should be called first to avoid exceptions
| } | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| public static <T> ClusterValue<T> of(Object data) { |
| } | ||
|
|
||
| /** Get the value type. Use it prior to accessing the data. */ | ||
| public boolean hasMultiData() { |
There was a problem hiding this comment.
javadoc - please be more specific since this is exposed to the user.
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Yury-Fridlyand
merged commit f296c00
into
java/integ_yuryf_cluster_bulk_resp
… bulk response (valkey-io#864) * Add cluster client, request routes configuration and support for bulk response (#59) Signed-off-by: Yury-Fridlyand <[email protected]>
… response (#59) * Add cluster client and routes support for cluster client. Signed-off-by: Yury-Fridlyand <[email protected]> * Address PR feedback and add tests. Signed-off-by: Yury-Fridlyand <[email protected]> * Minor javadoc update. Signed-off-by: Yury-Fridlyand <[email protected]> * Minor javadoc fix Signed-off-by: Yury-Fridlyand <[email protected]> * Address PR review comments. Signed-off-by: Yury-Fridlyand <[email protected]> * Address PR review comments. 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]>
Depends on:
Features:
RedisValue, which is union to a string and a mapUse case:
Meanwhile a regular client has no option to set routes to a command.
