Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Java: Refactor CommandManager.java and remove Command.java #916

Merged
merged 3 commits into from
Feb 8, 2024
Merged

Java: Refactor CommandManager.java and remove Command.java #916

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8ea0cb5
Java: Refactor command_manager and remove command
acarbonetto Feb 7, 2024
c334078
Update commandmanager to remove optional<route> argument
acarbonetto Feb 7, 2024
118afaf
Updates for review comments
acarbonetto Feb 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 14 additions & 14 deletions java/client/src/main/java/glide/api/BaseClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,18 +26,6 @@ public abstract class BaseClient implements AutoCloseable {
protected final ConnectionManager connectionManager;
protected final CommandManager commandManager;

/**
* Extracts the response from the Protobuf response and either throws an exception or returns the
* appropriate response as an Object
*
* @param response Redis protobuf message
* @return Response Object
*/
protected Object handleObjectResponse(Response response) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving this below. There should be about a dozen handleXResponse functions to be included in upcoming PRs and they should be below the CreateClient calls.

// convert protobuf response into Object and then Object into T
return new BaseCommandResponseResolver(RedisValueResolver::valueFromPointer).apply(response);
}

/**
* Async request for an async (non-blocking) Redis client.
*
Expand Down Expand Up @@ -74,8 +62,8 @@ protected static <T> CompletableFuture<T> CreateClient(
* Closes this resource, relinquishing any underlying resources. This method is invoked
* automatically on objects managed by the try-with-resources statement.
*
* <p>see: <a
* href="https://docs.oracle.com/javase/8/docs/api/java/lang/AutoCloseable.html#close--">AutoCloseable::close()</a>
* @see <a
* href="https://docs.oracle.com/javase/8/docs/api/java/lang/AutoCloseable.html#close--">AutoCloseable::close()</a>
*/
@Override
public void close() throws ExecutionException {
Expand All @@ -100,4 +88,16 @@ protected static ConnectionManager buildConnectionManager(ChannelHandler channel
protected static CommandManager buildCommandManager(ChannelHandler channelHandler) {
return new CommandManager(channelHandler);
}

/**
* Extracts the response from the Protobuf response and either throws an exception or returns the
* appropriate response as an <code>Object</code>.
*
* @param response Redis protobuf message
* @return Response <code>Object</code>
*/
protected Object handleObjectResponse(Response response) {
// convert protobuf response into Object
return new BaseCommandResponseResolver(RedisValueResolver::valueFromPointer).apply(response);
}
}
7 changes: 3 additions & 4 deletions java/client/src/main/java/glide/api/RedisClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
/** Copyright GLIDE-for-Redis Project Contributors - SPDX Identifier: Apache-2.0 */
package glide.api;

import static redis_request.RedisRequestOuterClass.RequestType.CustomCommand;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor:
I don't know about Java, but in Rust it's considered a code smell to import enum value names without the enum name, mostly because it might cause name collisions, and reduces legibility - it's less clear that the passed value is an enum of a certain kind.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't heard of that in Java. I will look that one up.


import glide.api.commands.BaseCommands;
import glide.api.models.configuration.RedisClientConfiguration;
import glide.managers.CommandManager;
import glide.managers.ConnectionManager;
import glide.managers.models.Command;
import java.util.concurrent.CompletableFuture;

/**
Expand All @@ -30,8 +31,6 @@ public static CompletableFuture<RedisClient> CreateClient(RedisClientConfigurati

@Override
public CompletableFuture<Object> customCommand(String[] args) {
Command command =
Command.builder().requestType(Command.RequestType.CUSTOM_COMMAND).arguments(args).build();
return commandManager.submitNewCommand(command, this::handleObjectResponse);
return commandManager.submitNewCommand(CustomCommand, args, this::handleObjectResponse);
}
}
22 changes: 10 additions & 12 deletions java/client/src/main/java/glide/api/RedisClusterClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,16 @@
/** Copyright GLIDE-for-Redis Project Contributors - SPDX Identifier: Apache-2.0 */
package glide.api;

import static redis_request.RedisRequestOuterClass.RequestType.CustomCommand;
acarbonetto marked this conversation as resolved.
Show resolved Hide resolved

import glide.api.commands.ClusterBaseCommands;
import glide.api.models.ClusterValue;
import glide.api.models.configuration.RedisClusterClientConfiguration;
import glide.api.models.configuration.RequestRoutingConfiguration.Route;
import glide.managers.CommandManager;
import glide.managers.ConnectionManager;
import glide.managers.models.Command;
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.CompletableFuture;

/**
Expand All @@ -34,25 +36,21 @@ public static CompletableFuture<RedisClusterClient> CreateClient(

@Override
public CompletableFuture<ClusterValue<Object>> customCommand(String[] args) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Future PR to switch this to customCommand(String... args)

Command command =
Command.builder().requestType(Command.RequestType.CUSTOM_COMMAND).arguments(args).build();
// TODO if a command returns a map as a single value, ClusterValue misleads user
return commandManager.submitNewCommand(
command, response -> ClusterValue.of(handleObjectResponse(response)));
CustomCommand,
args,
Optional.empty(),
response -> ClusterValue.of(handleObjectResponse(response)));
}

@Override
@SuppressWarnings("unchecked")
public CompletableFuture<ClusterValue<Object>> customCommand(String[] args, Route route) {
Command command =
Command.builder()
.requestType(Command.RequestType.CUSTOM_COMMAND)
.arguments(args)
.route(route)
.build();

return commandManager.submitNewCommand(
command,
CustomCommand,
args,
Optional.of(route),
response ->
route.isSingleNodeRoute()
? ClusterValue.ofSingleValue(handleObjectResponse(response))
Expand Down
118 changes: 70 additions & 48 deletions java/client/src/main/java/glide/managers/CommandManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,11 @@
import glide.api.models.exceptions.ClosingException;
import glide.connectors.handlers.CallbackDispatcher;
import glide.connectors.handlers.ChannelHandler;
import glide.managers.models.Command;
import java.util.Optional;
import java.util.concurrent.CompletableFuture;
import lombok.RequiredArgsConstructor;
import redis_request.RedisRequestOuterClass;
import redis_request.RedisRequestOuterClass.Command;
import redis_request.RedisRequestOuterClass.Command.ArgsArray;
import redis_request.RedisRequestOuterClass.RedisRequest;
import redis_request.RedisRequestOuterClass.RequestType;
Expand All @@ -34,21 +34,52 @@ public class CommandManager {
/**
* Build a command and send.
*
* @param command The command to execute
* @param requestType Redis command type
* @param arguments Redis command arguments
* @param responseHandler The handler for the response object
* @return A result promise of type T
*/
public <T> CompletableFuture<T> submitNewCommand(
Command command, RedisExceptionCheckedFunction<Response, T> responseHandler) {
RedisRequestOuterClass.RequestType requestType,
String[] arguments,
RedisExceptionCheckedFunction<Response, T> responseHandler) {

RedisRequest.Builder command = prepareRedisRequest(requestType, arguments);
return submitNewCommand(command, responseHandler);
}

/**
* Build a command and send.
*
* @param requestType Redis command type
* @param arguments Redis command arguments
* @param route Command routing parameters
* @param responseHandler The handler for the response object
* @return A result promise of type T
*/
public <T> CompletableFuture<T> submitNewCommand(
RequestType requestType,
String[] arguments,
Optional<Route> route,
RedisExceptionCheckedFunction<Response, T> responseHandler) {

RedisRequest.Builder command = prepareRedisRequest(requestType, arguments, route);
return submitNewCommand(command, responseHandler);
}

/**
* Take a redis request and send to channel.
*
* @param command The Redis command request as a builder to execute
* @param responseHandler The handler for the response object
* @return A result promise of type T
*/
protected <T> CompletableFuture<T> submitNewCommand(
RedisRequest.Builder command, RedisExceptionCheckedFunction<Response, T> responseHandler) {
// write command request to channel
// when complete, convert the response to our expected type T using the given responseHandler
return channel
.write(
prepareRedisRequest(
command.getRequestType(),
command.getArguments(),
Optional.ofNullable(command.getRoute())),
true)
.write(command, true)
.exceptionally(this::exceptionHandler)
.thenApplyAsync(responseHandler::apply);
}
Expand All @@ -71,68 +102,59 @@ private Response exceptionHandler(Throwable e) {
}

/**
* Build a protobuf command/transaction request object.<br>
* Used by {@link CommandManager}.
* Build a protobuf command request object with routing options.
*
* @param command - Redis command
* @param args - Redis command arguments as string array
* @return An uncompleted request. CallbackDispatcher is responsible to complete it by adding a
* callback id.
* @param requestType Redis command type
* @param arguments Redis command arguments
* @param route Command routing parameters
* @return An incomplete request. {@link CallbackDispatcher} is responsible to complete it by
* adding a callback id.
*/
private RedisRequestOuterClass.RedisRequest.Builder prepareRedisRequest(
Command.RequestType command, String[] args) {
RedisRequestOuterClass.Command.ArgsArray.Builder commandArgs =
RedisRequestOuterClass.Command.ArgsArray.newBuilder();
for (var arg : args) {
protected RedisRequest.Builder prepareRedisRequest(
RequestType requestType, String[] arguments, Optional<Route> route) {
ArgsArray.Builder commandArgs = ArgsArray.newBuilder();
for (var arg : arguments) {
commandArgs.addArgs(arg);
}

// TODO: set route properly when no RouteOptions given
return RedisRequestOuterClass.RedisRequest.newBuilder()
.setSingleCommand(
RedisRequestOuterClass.Command.newBuilder()
.setRequestType(mapRequestTypes(command))
.setArgsArray(commandArgs.build())
.build())
.setRoute(
RedisRequestOuterClass.Routes.newBuilder()
.setSimpleRoutes(RedisRequestOuterClass.SimpleRoutes.AllNodes)
.build());
}
var builder =
RedisRequest.newBuilder()
.setSingleCommand(
Command.newBuilder()
.setRequestType(requestType)
.setArgsArray(commandArgs.build())
.build());

private RequestType mapRequestTypes(Command.RequestType inType) {
switch (inType) {
case CUSTOM_COMMAND:
return RequestType.CustomCommand;
}
throw new RuntimeException("Unsupported request type");
return prepareRedisRequestRoute(builder, route);
}

/**
* Build a protobuf command/transaction request object with routing options.<br>
* Used by {@link CommandManager}.
* Build a protobuf command request object.
*
* @param command Redis command type
* @param args Redis command arguments
* @param route Command routing parameters
* @param requestType Redis command type
* @param arguments Redis command arguments
* @return An uncompleted request. {@link CallbackDispatcher} is responsible to complete it by
* adding a callback id.
*/
private RedisRequest.Builder prepareRedisRequest(
Command.RequestType command, String[] args, Optional<Route> route) {
protected RedisRequest.Builder prepareRedisRequest(RequestType requestType, String[] arguments) {
ArgsArray.Builder commandArgs = ArgsArray.newBuilder();
for (var arg : args) {
for (var arg : arguments) {
commandArgs.addArgs(arg);
}

var builder =
RedisRequest.newBuilder()
.setSingleCommand(
RedisRequestOuterClass.Command.newBuilder()
.setRequestType(mapRequestTypes(command))
Command.newBuilder()
.setRequestType(requestType)
.setArgsArray(commandArgs.build())
.build());

return prepareRedisRequestRoute(builder, Optional.empty());
acarbonetto marked this conversation as resolved.
Show resolved Hide resolved
}

private RedisRequest.Builder prepareRedisRequestRoute(
RedisRequest.Builder builder, Optional<Route> route) {
if (route.isEmpty()) {
acarbonetto marked this conversation as resolved.
Show resolved Hide resolved
return builder;
}
Expand Down
29 changes: 0 additions & 29 deletions java/client/src/main/java/glide/managers/models/Command.java
View file
Open in desktop

This file was deleted.

15 changes: 5 additions & 10 deletions java/client/src/test/java/glide/ExceptionHandlingTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static redis_request.RedisRequestOuterClass.RequestType.CustomCommand;
import static response.ResponseOuterClass.RequestErrorType.Disconnect;
import static response.ResponseOuterClass.RequestErrorType.ExecAbort;
import static response.ResponseOuterClass.RequestErrorType.Timeout;
Expand All @@ -26,8 +27,6 @@
import glide.managers.BaseCommandResponseResolver;
import glide.managers.CommandManager;
import glide.managers.ConnectionManager;
import glide.managers.models.Command;
import glide.managers.models.Command.RequestType;
import io.netty.channel.ChannelFuture;
import java.io.IOException;
import java.util.concurrent.CancellationException;
Expand Down Expand Up @@ -95,7 +94,7 @@ public void channel_is_closed_when_disconnected_on_command() {
var channelHandler = new TestChannelHandler(callbackDispatcher);
var commandManager = new CommandManager(channelHandler);

var future = commandManager.submitNewCommand(createDummyCommand(), r -> null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: I think that createDummyCommand conveyed intention rather nicely. consider sendDummyCommand instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

createDummyCommand made more sense when we had the Command.java, but that was removed in this PR. Instead, we're just passing the object parameters directly.
I DO agree that the "dummy" command conveyed information to a reader that this parameter is irrelevant to the test. Maybe I can do something about that and make it more obvious...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes to using TEST_ARGS. I think it adds about the same as the createDummyCommand()

var future = commandManager.submitNewCommand(CustomCommand, new String[0], r -> null);
callbackDispatcher.completeRequest(null);
var exception = assertThrows(ExecutionException.class, future::get);
// a ClosingException thrown from CallbackDispatcher::completeRequest and then
Expand All @@ -112,7 +111,7 @@ public void channel_is_not_closed_when_error_was_in_command_pipeline() {
var channelHandler = new TestChannelHandler(callbackDispatcher);
var commandManager = new CommandManager(channelHandler);

var future = commandManager.submitNewCommand(createDummyCommand(), r -> null);
var future = commandManager.submitNewCommand(CustomCommand, new String[0], r -> null);
callbackDispatcher.completeRequest(null);
var exception = assertThrows(ExecutionException.class, future::get);
// a RequestException thrown from CallbackDispatcher::completeRequest and then
Expand All @@ -129,7 +128,7 @@ public void command_manager_rethrows_non_RedisException_too() {
var channelHandler = new TestChannelHandler(callbackDispatcher);
var commandManager = new CommandManager(channelHandler);

var future = commandManager.submitNewCommand(createDummyCommand(), r -> null);
var future = commandManager.submitNewCommand(CustomCommand, new String[0], r -> null);
callbackDispatcher.completeRequest(null);
var exception = assertThrows(ExecutionException.class, future::get);
// a IOException thrown from CallbackDispatcher::completeRequest and then wrapped
Expand Down Expand Up @@ -210,7 +209,7 @@ public void dont_close_connection_when_callback_dispatcher_receives_response_wit
var channelHandler = new TestChannelHandler(callbackDispatcher);
var commandManager = new CommandManager(channelHandler);

var future = commandManager.submitNewCommand(createDummyCommand(), r -> null);
var future = commandManager.submitNewCommand(CustomCommand, new String[0], r -> null);
var response =
Response.newBuilder()
.setCallbackIdx(0)
Expand Down Expand Up @@ -280,10 +279,6 @@ private static RedisClientConfiguration createDummyConfig() {
return RedisClientConfiguration.builder().build();
}

private static Command createDummyCommand() {
return Command.builder().requestType(RequestType.CUSTOM_COMMAND).build();
}

/** Test ChannelHandler extension which allows to validate whether the channel was closed. */
private static class TestChannelHandler extends ChannelHandler {

Expand Down
Loading
Loading