Skip to content

Commit

Permalink
Fix Node AzAffinity Flaky Test (valkey-io#3107)
Browse files Browse the repository at this point in the history 
--------------------------------------------------------------
Signed-off-by: Muhammad Awawdi <mawawdi@amazon.com>
  • Loading branch information
@Muhammad-awawdi-amazon
Muhammad-awawdi-amazon authored Feb 18, 2025
1 parent 9e58738 commit 4de9ae4
Showing 1 changed file with 17 additions and 69 deletions.
86 changes: 17 additions & 69 deletions node/tests/GlideClusterClient.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2170,50 +2170,13 @@ describe("GlideClusterClient", () => {
);

describe("AZAffinity Read Strategy Tests", () => {
async function getNumberOfReplicas(
azClient: GlideClusterClient,
): Promise<number> {
const replicationInfo = (await azClient.info({
sections: [InfoOptions.Replication],
})) as Record<string, string>;
let totalReplicas = 0;
Object.values(replicationInfo).forEach((nodeInfo) => {
const lines = nodeInfo.split(/\r?\n/);
const connectedReplicasLine = lines.find(
(line) =>
line.startsWith("connected_slaves:") ||
line.startsWith("connected_replicas:"),
);

if (connectedReplicasLine) {
const parts = connectedReplicasLine.split(":");
const numReplicas = parseInt(parts[1], 10);

if (!isNaN(numReplicas)) {
// Sum up replicas from each primary node
totalReplicas += numReplicas;
}
}
});

if (totalReplicas > 0) {
return totalReplicas;
}

throw new Error(
"Could not find replica information in any node's response",
);
}

it.each([ProtocolVersion.RESP2, ProtocolVersion.RESP3])(
"should route GET commands to all replicas with the same AZ using protocol %p",
async (protocol) => {
// Skip test if version is below 8.0.0
if (cluster.checkIfServerVersionLessThan("8.0.0")) return;

const az = "us-east-1a";
const GET_CALLS_PER_REPLICA = 3;

let client_for_config_set;
let client_for_testing_az;

Expand All @@ -2224,7 +2187,6 @@ describe("GlideClusterClient", () => {
getClientConfigurationOption(
azCluster.getAddresses(),
protocol,
{ requestTimeout: 3000 },
),
);

Expand All @@ -2234,28 +2196,13 @@ describe("GlideClusterClient", () => {
{ route: "allNodes" },
);

// Retrieve the number of replicas dynamically
const n_replicas = await getNumberOfReplicas(
client_for_config_set,
);

if (n_replicas === 0) {
throw new Error(
"No replicas found in the cluster. Test requires at least one replica.",
);
}

const GET_CALLS = GET_CALLS_PER_REPLICA * n_replicas;
const get_cmdstat = `calls=${GET_CALLS_PER_REPLICA}`;

// Stage 2: Create AZ affinity client and verify configuration
client_for_testing_az =
await GlideClusterClient.createClient(
getClientConfigurationOption(
azCluster.getAddresses(),
protocol,
{
requestTimeout: 3000,
readFrom: "AZAffinity",
clientAz: az,
},
Expand All @@ -2273,17 +2220,22 @@ describe("GlideClusterClient", () => {
),
);

const get_calls_per_replica = 25;
const get_calls = 100;
const get_cmdstat = `cmdstat_get:calls=${get_calls_per_replica}`;

// Stage 3: Set test data and perform GET operations
await client_for_testing_az.set("foo", "testvalue");

for (let i = 0; i < GET_CALLS; i++) {
for (let i = 0; i < get_calls; i++) {
await client_for_testing_az.get("foo");
}

// Stage 4: Verify GET commands were routed correctly
const info_result = (await client_for_testing_az.info(
{ sections: [InfoOptions.All], route: "allNodes" }, // Get both replication and commandstats info
)) as Record<string, string>;
const info_result = (await client_for_testing_az.info({
sections: [InfoOptions.All],
route: "allNodes",
})) as Record<string, string>;

const matching_entries_count = Object.values(
info_result,
Expand All @@ -2296,7 +2248,7 @@ describe("GlideClusterClient", () => {
return isReplicaNode && infoStr.includes(get_cmdstat);
}).length;

expect(matching_entries_count).toBe(n_replicas); // Should expect 12 as the cluster was created with 3 primary and 4 replicas, totalling 12 replica nodes
expect(matching_entries_count).toBe(4);
} finally {
// Cleanup
await client_for_config_set?.configSet(
Expand All @@ -2316,8 +2268,8 @@ describe("GlideClusterClient", () => {
if (cluster.checkIfServerVersionLessThan("8.0.0")) return;

const az = "us-east-1a";
const GET_CALLS = 3;
const get_cmdstat = `calls=${GET_CALLS}`;
const get_calls = 3;
const get_cmdstat = `cmdstat_get:calls=${get_calls}`;
let client_for_config_set;
let client_for_testing_az;

Expand All @@ -2328,7 +2280,6 @@ describe("GlideClusterClient", () => {
getClientConfigurationOption(
azCluster.getAddresses(),
protocol,
{ requestTimeout: 3000 },
),
);

Expand All @@ -2341,7 +2292,7 @@ describe("GlideClusterClient", () => {

await client_for_config_set.configSet(
{ "availability-zone": az },
{ route: { type: "replicaSlotId", id: 12182 } },
{ route: { type: "replicaSlotKey", key: "foo" } },
);

// Stage 2: Create AZ affinity client and verify configuration
Expand All @@ -2351,15 +2302,13 @@ describe("GlideClusterClient", () => {
azCluster.getAddresses(),
protocol,
{
requestTimeout: 3000,
readFrom: "AZAffinity",
clientAz: az,
},
),
);
await client_for_testing_az.set("foo", "testvalue");

for (let i = 0; i < GET_CALLS; i++) {
for (let i = 0; i < get_calls; i++) {
await client_for_testing_az.get("foo");
}

Expand Down Expand Up @@ -2406,7 +2355,7 @@ describe("GlideClusterClient", () => {
// Skip test if version is below 8.0.0
if (cluster.checkIfServerVersionLessThan("8.0.0")) return;

const GET_CALLS = 4;
const get_calls = 4;
const replica_calls = 1;
const get_cmdstat = `cmdstat_get:calls=${replica_calls}`;
let client_for_testing_az;
Expand All @@ -2421,7 +2370,6 @@ describe("GlideClusterClient", () => {
{
readFrom: "AZAffinity",
clientAz: "non-existing-az",
requestTimeout: 3000,
},
),
);
Expand All @@ -2432,7 +2380,7 @@ describe("GlideClusterClient", () => {
});

// Issue GET commands
for (let i = 0; i < GET_CALLS; i++) {
for (let i = 0; i < get_calls; i++) {
await client_for_testing_az.get("foo");
}

Expand All @@ -2449,7 +2397,7 @@ describe("GlideClusterClient", () => {
return nodeResponses.includes(get_cmdstat);
}).length;

// Validate that only one replica handled the GET calls
// Validate that the get calls were distributed across replicas, each replica recieved 1 get call
expect(matchingEntriesCount).toBe(4);
} finally {
// Cleanup: Close the client after test execution
Expand Down

0 comments on commit 4de9ae4

Please sign in to comment.