-
Notifications
You must be signed in to change notification settings - Fork 0
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.
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
Support transactions #65
Support transactions #65
Conversation
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer not to expect users to create a 2D array of Strings
* @param args arguments for the custom command | ||
* @return a CompletableFuture with response result from Redis | ||
*/ | ||
CompletableFuture<Object[]> customTransaction(String[][] args); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about exposing?
CompletableFuture<Object[]> customTransaction(Transaction transaction);
This is what the node client does: https://github.com/Bit-Quill/glide-for-redis/blob/46f831c5d2b31a667f7d7c53035b99e8b3851f26/node/src/RedisClient.ts#L98
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may have to create a Transactions class that implements BaseCommands that adds the transaction calls to the list.
Maybe also a ClusterTransactions class that implements BaseClusterCommands too...
reference: https://github.com/Bit-Quill/glide-for-redis/blob/main/node/src/Transaction.ts#L72
@@ -30,7 +32,7 @@ public static CompletableFuture<RedisClient> CreateClient(RedisClientConfigurati | |||
// TODO: Support exception throwing, including interrupted exceptions | |||
return connectionManager | |||
.connectToRedis(config) | |||
.thenApply(ignore -> new RedisClient(connectionManager, commandManager)); | |||
.thenApplyAsync(ignore -> new RedisClient(connectionManager, commandManager)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not run it in the same thread?
var transactionBuilder = Transaction.newBuilder(); | ||
for (var command : transaction) { | ||
transactionBuilder.addCommands( | ||
prepareRedisCommand(command.getRequestType(), command.getArguments())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how does this work routes? Each command gets a route, right?
I think the API.Command object can be used to pass that in, and there will be a third argument in prepareRedisCommand that takes a command.getRouteOption()
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to protobuf - route set to the entire request
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. That makes sense.
Depends on:
returns
Object[2]
with 2 strings:OK
andvvv
.