-
Notifications
You must be signed in to change notification settings - Fork 71
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
Node: Handle commands with binary output in transaction #2133
Node: Handle commands with binary output in transaction #2133
Conversation
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
There is a CI failure related to your changes. |
Can't reproduce it locally, but I fixed that. |
@@ -1,4 +1,5 @@ | |||
#### Changes | |||
* Node: Added handlind commands with binary output in transactions ([#2133](https://github.com/valkey-io/valkey-glide/pull/2133)) |
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.
* Node: Added handlind commands with binary output in transactions ([#2133](https://github.com/valkey-io/valkey-glide/pull/2133)) | |
* Node: Added handling for commands with binary output in transactions ([#2133](https://github.com/valkey-io/valkey-glide/pull/2133)) |
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 think we should align more closely with the Java implementation. I'd suggest that we move the Decoder from the exec()
call to the Transaction object. The user can set the decoder (optionally) when creating the Transaction object, or by calling something like .withBinarySafeOutput()
like we do in Java.
In this case, exec()
would only take a transaction object.
Reference: https://github.com/valkey-io/valkey-glide/pull/1814/files#diff-a9af498f347847324483b3ee806c7a98babbfba5ff1313c7998edecdc652aa5a
transaction.requiresBinaryDecorer | ||
) { | ||
throw new RequestError( | ||
"Transaction has a command which requres `Decoder.Bytes`.", |
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.
"Transaction has a command which requres `Decoder.Bytes`.", | |
"Transaction has a command which requires `Decoder.Bytes`.", |
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.
Maybe:
"Pass decoder=Decoder.Bytes as part of the exec() command to process transactions that return Bytes."
exec
callsexec
client.close()
in IT where it was missingrunBaseTests
andrunCommonTests
) to accept client config and redefine paramscontext
from shared testsShould be merged before #2126 and #2129