-
Notifications
You must be signed in to change notification settings - Fork 54
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
Add SMOVE command for node #1476
Conversation
Co-authored-by: Shoham Elias <116083498+shohamazon@users.noreply.github.com>
node/src/Transaction.ts
Outdated
* Creates a new destination set if needed. The operation is atomic. | ||
* See https://valkey.io/commands/smove for more details. | ||
* | ||
* Note: When in cluster mode, `source` and `destination` must map to the same hash slot. |
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.
This is not applicable for transaction, please remove
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 decided to not add it in Transaction?
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 could be routed to a single node only.
It does not protect a user from making a mistake, but this notice is more applicable to exec
function which submits a transaction, rather than to a commands in the transaction.
If we change that decision (it is OK), we need to add the notice to all multi-key commands in transaction in all clients.
@@ -1046,6 +1046,72 @@ export function runBaseTests<Context>(config: { | |||
config.timeout, | |||
); | |||
|
|||
it.each([ProtocolVersion.RESP2, ProtocolVersion.RESP3])( |
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.
Add a cross slot test for this command https://github.com/aws/glide-for-redis/blob/64d2273f093a6f27d0674f741f6ea7303b02563b/node/tests/RedisClusterClient.test.ts#L279
* Add SMOVE command for node * formatting * Apply suggestions from code review Co-authored-by: Shoham Elias <116083498+shohamazon@users.noreply.github.com> * fix comments --------- Co-authored-by: ort-bot <glide-for-redis@amazon.com> Co-authored-by: Ubuntu <ubuntu@ip-172-31-20-143.eu-west-1.compute.internal> Co-authored-by: Shoham Elias <116083498+shohamazon@users.noreply.github.com>
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.