Node: transaction commands for json module #2690

prateek-kumar-improving · 2024-11-14T16:33:42Z

Issue link

This Pull Request is linked to issue (URL): #2683

Checklist

Before submitting the PR make sure the following are checked:

This Pull Request is related to one issue.
Commit message has a detailed description of what changed and why.
Tests are added or updated.
CHANGELOG.md and documentation files are updated.
Destination branch is correct - main or release
Commits will be squashed upon merging.

Test results on 61a85d0:

Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>

node/tests/ServerModules.test.ts

node/tests/TestUtilities.ts

Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>

Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>

acarbonetto · 2024-11-15T19:08:10Z

node/tests/TestUtilities.ts

+    baseTransaction: ClusterTransaction,
+): Promise<[string, GlideReturnType][]> {
+    const responseData: [string, GlideReturnType][] = [];
+    const key1 = "key1" + uuidv4();


update the key to use the same prefix

acarbonetto · 2024-11-15T19:13:08Z

node/src/server-modules/GlideJson.ts

+     * is atomic only at the slot level. If one or more slot-specific requests fail, the entire
+     * call will return the first encountered error, even though some requests may have succeeded
+     * while others did not. If this behavior impacts your application logic, consider splitting
+     * the request into sub-requests per slot to ensure atomicity.


@remarks When in cluster mode, all keys in the transaction must be mapped to the same slot.

This is the remark on BaseClient mget as well. Should that also be updated?

No
transaction goes to only 1 node always, but a command executed outside of transaction could be routed to multiple nodes.
Good finding - should be updated for mget in transaction for all clients

Agreed. I have made a comment on 3 PRs.

@Yury-Fridlyand should it be added to MGET? (not JSON.MGET)
I think our logic was that the same comment applies to all commands (including between commands) and so it's defined on the wiki instead of in the javadoc.

Same comment could be applied to all multi-key commands.

I mean JSON.MGET
I remember we were not adding same slot notice in transaction, but I don't remember the reason...

… transaction tests Signed-off-by: Jonathan Louie <Jonathan.Louie@improving.com>

Signed-off-by: Jonathan Louie <Jonathan.Louie@improving.com>

* Node: Add Transaction for JSON commands --------- Signed-off-by: Prateek Kumar <prateek.kumar@improving.com> Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com> Signed-off-by: Jonathan Louie <Jonathan.Louie@improving.com> Co-authored-by: Andrew Carbonetto <andrew.carbonetto@improving.com> Co-authored-by: Jonathan Louie <Jonathan.Louie@improving.com>

prateek-kumar-improving added 2 commits November 14, 2024 21:53

Node: Add Transaction for JSON commands

b95281f

Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>

Node: JSON transaction commands - fix review comments

d717ac4

Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>

prateek-kumar-improving requested a review from a team as a code owner November 14, 2024 16:33

prateek-kumar-improving had a problem deploying to AWS_ACTIONS November 14, 2024 16:33 — with GitHub Actions Error

Node: JSON transaction commands - change log added

e099720

Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>

prateek-kumar-improving had a problem deploying to AWS_ACTIONS November 14, 2024 16:36 — with GitHub Actions Error

acarbonetto reviewed Nov 14, 2024

View reviewed changes

node/tests/ServerModules.test.ts Outdated Show resolved Hide resolved

node/tests/ServerModules.test.ts Outdated Show resolved Hide resolved

node/tests/TestUtilities.ts Outdated Show resolved Hide resolved

node/tests/TestUtilities.ts Outdated Show resolved Hide resolved

acarbonetto changed the title ~~Node transaction commands json~~ Node: transaction commands for json module Nov 14, 2024

acarbonetto added the node Node.js wrapper label Nov 14, 2024

Fix JSON.DEBUG calls to use second arg

87ae30a

Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>

acarbonetto had a problem deploying to AWS_ACTIONS November 15, 2024 06:23 — with GitHub Actions Error

Fix MGET test

22ed2ae

Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>

acarbonetto had a problem deploying to AWS_ACTIONS November 15, 2024 06:33 — with GitHub Actions Error

Fix package.json

c17cf9a

Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>

acarbonetto had a problem deploying to AWS_ACTIONS November 15, 2024 06:39 — with GitHub Actions Error

acarbonetto had a problem deploying to AWS_ACTIONS November 15, 2024 06:40 — with GitHub Actions Error

Fix test names

e5b1304

Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>

acarbonetto had a problem deploying to AWS_ACTIONS November 15, 2024 06:41 — with GitHub Actions Error

acarbonetto approved these changes Nov 15, 2024

View reviewed changes

Node: Transaction for JSON module - review comments fixed

85f1bb2

Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>

prateek-kumar-improving had a problem deploying to AWS_ACTIONS November 15, 2024 17:56 — with GitHub Actions Error

acarbonetto reviewed Nov 15, 2024

View reviewed changes

jamesx-improving changed the base branch from release-1.2 to feature/module-transaction November 15, 2024 19:59

Update docs for JSON.MGET in transactions and rename key for JSON ARR…

7295719

… transaction tests Signed-off-by: Jonathan Louie <Jonathan.Louie@improving.com>

jonathanl-bq had a problem deploying to AWS_ACTIONS November 15, 2024 21:17 — with GitHub Actions Error

jonathanl-bq approved these changes Nov 15, 2024

View reviewed changes

Run prettier

70b8227

Signed-off-by: Jonathan Louie <Jonathan.Louie@improving.com>

jonathanl-bq had a problem deploying to AWS_ACTIONS November 15, 2024 22:21 — with GitHub Actions Error

Add Avi's eslint fix

61a85d0

Signed-off-by: Jonathan Louie <Jonathan.Louie@improving.com>

jonathanl-bq had a problem deploying to AWS_ACTIONS November 16, 2024 00:23 — with GitHub Actions Failure

acarbonetto mentioned this pull request Nov 16, 2024

Support transactions for VSS and JSON commands #2683

Open

6 tasks

acarbonetto merged commit ecfe7a6 into feature/module-transaction Nov 16, 2024
46 of 49 checks passed

acarbonetto deleted the node-transaction-commands-json branch November 16, 2024 00:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Node: transaction commands for json module #2690

Node: transaction commands for json module #2690

prateek-kumar-improving commented Nov 14, 2024 •

edited by acarbonetto

Loading

acarbonetto Nov 15, 2024

acarbonetto Nov 15, 2024

jonathanl-bq Nov 15, 2024

Yury-Fridlyand Nov 15, 2024

acarbonetto Nov 15, 2024

acarbonetto Nov 15, 2024

acarbonetto Nov 15, 2024

Yury-Fridlyand Nov 15, 2024

Node: transaction commands for json module #2690

Node: transaction commands for json module #2690

Conversation

prateek-kumar-improving commented Nov 14, 2024 • edited by acarbonetto Loading

Issue link

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prateek-kumar-improving commented Nov 14, 2024 •

edited by acarbonetto

Loading