Java: Refactor CommandManager.java and remove Command.java #916
Conversation
Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>
| String payload = (String) response.get(); | ||
|
|
||
| // verify | ||
| assertEquals(testResponse, response); | ||
| assertEquals(value, payload); | ||
| } | ||
|
|
||
| @Test | ||
| public void customCommand_interruptedException() throws ExecutionException, InterruptedException { |
There was a problem hiding this comment.
test is better covered in IT - this test really doesn't do anything.
Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>
| * @param response Redis protobuf message | ||
| * @return Response Object | ||
| */ | ||
| protected Object handleObjectResponse(Response response) { |
There was a problem hiding this comment.
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.
| @@ -34,25 +36,21 @@ public static CompletableFuture<RedisClusterClient> CreateClient( | |||
|
|
|||
| @Override | |||
| public CompletableFuture<ClusterValue<Object>> customCommand(String[] args) { | |||
There was a problem hiding this comment.
Future PR to switch this to customCommand(String... args)
| * @param responseHandler The handler for the response object | ||
| * @return A result promise of type T | ||
| */ | ||
| protected <T> CompletableFuture<T> submitCommandToChannel( |
There was a problem hiding this comment.
renamed from submitNewCommand to submitCommandToChannel to be more specific and to being unambiguous with the public methods.
| */ | ||
| private Response exceptionHandler(Throwable e) { |
There was a problem hiding this comment.
Moved to the end of the file.
| @@ -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; | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I haven't heard of that in Java. I will look that one 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); | |||
There was a problem hiding this comment.
minor: I think that createDummyCommand conveyed intention rather nicely. consider sendDummyCommand instead.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
Changes to using TEST_ARGS. I think it adds about the same as the createDummyCommand()
| @@ -24,7 +25,7 @@ public void custom_command_returns_single_value() { | |||
|
|
|||
| var client = new TestClient(commandManager, "TEST"); | |||
|
|
|||
| var value = client.customCommand(new String[0]).get(); | |||
| var value = client.customCommand(new String[] {"test"}).get(); | |||
There was a problem hiding this comment.
why is the added argument needed?
There was a problem hiding this comment.
Well, because the server will fail on the backend if you pass an empty list. So this better represents our use case... even if our white box testing doesn't case.
|
|
||
| ArgumentCaptor<RedisRequest.Builder> captor = | ||
| ArgumentCaptor.forClass(RedisRequest.Builder.class); | ||
|
|
||
| service.submitNewCommand(CustomCommand, new String[0], routeType, r -> null); |
There was a problem hiding this comment.
why the change in order of calls?
There was a problem hiding this comment.
I moved the Mapping down to be closer to where it's being used.
Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>
Issue #, if available:
Description of changes:
This PR cleans up the command manager and how we create protobuf commands. This also removes the Command.java model that is no longer needed (as we don't expose any Command object in the API).
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.