Node: add AZ Affinity ReadFrom strategy Support #2686

Muhammad-awawdi-amazon · 2024-11-13T23:22:49Z

Issue link

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

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.

node/src/BaseClient.ts

avifenesh · 2024-11-14T10:09:25Z

node/src/BaseClient.ts

+     *
+     * @example
+     * ```typescript
+     * configuration.client_az = 'us-east-1a';


Does this is the only type possible? should we restrict to one working standard or making sure we tell the user he was wrong?

wdym here? enforcing the client_az to be in a certain format? and if not, throw an error message?

Not throwing, loging.
Its more for the core level as its mutual, but the question is do we know what should be the right form and help the user more in order to not make mistakes.
Even on the level of saying known optional forms: AWS EC2 uses ... AWS EKS uses ... GCP uses ... so at least its easy for him to validate it.

Everyone has their own technical area and things can be confusing when you meet them first time.
Somebody might think its 1a for example.

accidentally deleted Yuri's comment here.

node/tests/GlideClusterClient.test.ts

node/src/BaseClient.ts

node/tests/GlideClusterClient.test.ts

avifenesh · 2024-11-14T10:33:41Z

Some comments.
For the testing logic i will let @adarovadya review and approve. I know some of my questions might be relevant to Adar and not to you directly, feel free to tell her to address it.

Yury-Fridlyand · 2024-11-15T23:14:09Z

node/src/BaseClient.ts

+    | "preferReplica"
+    /** Spread the requests between replicas in the same client's AZ (Availability zone) in a round robin manner.
+        If no replica is available, route the requests to the primary.*/
+    | "AZAffinity"


Suggested change

| "AZAffinity"

| { type: "AZAffinity", clientAz?: string; }

That is more correct location of clientAz.

I think it's possible that in the future we will want to support fully baked support for EC2, EKS and all the known and widely used platform. Meaning, the user will provide something else than clientAz, and I'm afraid that putting those together will make issues later.
Try to think about the possible design and let me know if you don't think it will affect. IDK, the designs options doc is existing, but no decision has been taken, so I can speculate as you. I can share the doc if you want to dive into this question more deeply.

Sticked to the original, as when I updated it to what you're suggesting errors started appearing. LMK what you guys think

Signed-off-by: Muhammad Awawdi <[email protected]> az NODE tests Signed-off-by: Muhammad Awawdi <[email protected]> CR fixes Signed-off-by: Muhammad Awawdi <[email protected]> CR change Signed-off-by: Muhammad Awawdi <[email protected]> Modified tests to dynamically pull the replicas number, and added more replicas to cluster config Signed-off-by: Muhammad Awawdi <[email protected]> updated number of replicas for the cluster, dropped the part for CMD as its irrelevant Signed-off-by: Muhammad Awawdi <[email protected]> prettier Signed-off-by: Muhammad Awawdi <[email protected]> Created a new cluster for AzAffinity in CME, and added Standalone tests for CMD Signed-off-by: Muhammad Awawdi <[email protected]>

Signed-off-by: Muhammad Awawdi <[email protected]>

adarovadya · 2024-11-20T08:35:16Z

Please add Change.log comment. see example - https://github.com/valkey-io/valkey-glide/pull/2676/files#diff-06572a96a58dc510037d5efa622f9bec8519bc1beab13c9f251e97e657a9d4edR2

adarovadya · 2024-11-20T09:16:30Z

node/src/BaseClient.ts

@@ -574,6 +574,19 @@ export interface BaseClientConfiguration {
     * used.
     */
    inflightRequestsLimit?: number;
+    /**
+     * Availability Zone of the client.
+     * This setting ensures that read operations are directed to nodes within the specified AZ.


If ReadFrom strategy is AZAffinity, this setting ensures that readonly commands are directed to replicas within the specified AZ if exits.

adarovadya · 2024-11-20T11:56:42Z

node/tests/GlideClusterClient.test.ts

+                getServerVersion,
+            );
+
+            azCluster = await ValkeyCluster.createCluster(


maybe you can check the version before and to create it only for valkey 8 and above

adarovadya · 2024-11-20T12:00:33Z

node/tests/GlideClusterClient.test.ts

+                                    !("value" in value)
+                                )
+                                    return false;
+                                return value.value.includes(get_cmdstat);


this test will fail. The AZ strategy searching foe replica in the same az, if not exits it will other replica in round robin. so in this case every replica will run the get command once

Signed-off-by: Muhammad Awawdi <[email protected]>

* Added AZAffinity strategy to Node.js --------- Signed-off-by: Muhammad Awawdi <[email protected]>

Muhammad-awawdi-amazon requested a review from a team as a code owner November 13, 2024 23:22

Muhammad-awawdi-amazon force-pushed the AZ-node-tests branch 3 times, most recently from b2bd25d to 512af5e Compare November 13, 2024 23:45

avifenesh force-pushed the AZ-node-tests branch from db6a4c7 to 06b0a1a Compare November 14, 2024 00:11

Yury-Fridlyand added node Node.js wrapper testing Everything about testing labels Nov 14, 2024

Muhammad-awawdi-amazon self-assigned this Nov 14, 2024

Muhammad-awawdi-amazon requested a review from avifenesh November 14, 2024 10:02

avifenesh reviewed Nov 14, 2024

View reviewed changes

node/src/BaseClient.ts Outdated Show resolved Hide resolved

avifenesh reviewed Nov 14, 2024

View reviewed changes

node/tests/GlideClusterClient.test.ts Show resolved Hide resolved

avifenesh reviewed Nov 14, 2024

View reviewed changes

node/src/BaseClient.ts Outdated Show resolved Hide resolved

avifenesh reviewed Nov 14, 2024

View reviewed changes

node/tests/GlideClusterClient.test.ts Show resolved Hide resolved

Muhammad-awawdi-amazon force-pushed the AZ-node-tests branch from 06b0a1a to a6108e9 Compare November 14, 2024 12:36

Yury-Fridlyand reviewed Nov 15, 2024

View reviewed changes

Muhammad-awawdi-amazon force-pushed the AZ-node-tests branch 4 times, most recently from f045df9 to 5469747 Compare November 17, 2024 10:26

valkey-io deleted a comment from Yury-Fridlyand Nov 17, 2024

acarbonetto requested review from acarbonetto and tjzhang-BQ November 18, 2024 17:46

Muhammad-awawdi-amazon force-pushed the AZ-node-tests branch 2 times, most recently from 00f12ef to ff525d7 Compare November 19, 2024 12:26

Muhammad-awawdi-amazon force-pushed the AZ-node-tests branch from ff525d7 to ad8f145 Compare November 19, 2024 12:41

Muhammad-awawdi-amazon requested a review from adarovadya November 19, 2024 12:42

Merge branch 'release-1.2' into AZ-node-tests

a8e25fb

Minor Changes for the new cluster creation

fde057f

Signed-off-by: Muhammad Awawdi <[email protected]>

Muhammad-awawdi-amazon mentioned this pull request Nov 19, 2024

AZ Awareness #643

Closed

adarovadya reviewed Nov 20, 2024

View reviewed changes

Muhammad-awawdi-amazon changed the title ~~Az node tests~~ Node: add AZ Affinity ReadFrom strategy Support Nov 20, 2024

Muhammad-awawdi-amazon added 7 commits November 20, 2024 16:16

updated tests

65765c8

Signed-off-by: Muhammad Awawdi <[email protected]>

test update

546e19c

Signed-off-by: Muhammad Awawdi <[email protected]>

prettier

146a982

Signed-off-by: Muhammad Awawdi <[email protected]>

final test update

0397a2c

Signed-off-by: Muhammad Awawdi <[email protected]>

removed unnecessary console.logs

c106fe2

Signed-off-by: Muhammad Awawdi <[email protected]>

deleted standalone tests

1bd96bb

Signed-off-by: Muhammad Awawdi <[email protected]>

Merge branch 'release-1.2' into AZ-node-tests

1b764d9

adarovadya approved these changes Nov 20, 2024

View reviewed changes

adarovadya merged commit 3c25ab9 into valkey-io:release-1.2 Nov 20, 2024
19 checks passed

prateek-kumar-improving pushed a commit that referenced this pull request Nov 26, 2024

Node: add AZ Affinity ReadFrom strategy Support (#2686)

9c26a5b

* Added AZAffinity strategy to Node.js --------- Signed-off-by: Muhammad Awawdi <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Node: add AZ Affinity ReadFrom strategy Support #2686

Node: add AZ Affinity ReadFrom strategy Support #2686

Muhammad-awawdi-amazon commented Nov 13, 2024 •

edited

Loading

avifenesh Nov 14, 2024

Muhammad-awawdi-amazon Nov 14, 2024

avifenesh Nov 14, 2024

avifenesh Nov 16, 2024

Muhammad-awawdi-amazon Nov 17, 2024

avifenesh commented Nov 14, 2024

Yury-Fridlyand Nov 15, 2024

avifenesh Nov 16, 2024

Muhammad-awawdi-amazon Nov 17, 2024

adarovadya commented Nov 20, 2024

adarovadya Nov 20, 2024

adarovadya Nov 20, 2024

adarovadya Nov 20, 2024

Node: add AZ Affinity ReadFrom strategy Support #2686

Node: add AZ Affinity ReadFrom strategy Support #2686

Conversation

Muhammad-awawdi-amazon commented Nov 13, 2024 • edited 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

avifenesh commented Nov 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adarovadya commented Nov 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Muhammad-awawdi-amazon commented Nov 13, 2024 •

edited

Loading