Skip to content

Commit 210ba24

Browse files
avifeneshjhpung
andauthored
Node allow uncovered slots cscan (#2815)
* addresed comments Signed-off-by: avifenesh <[email protected]> * Go: add allow_non_covered_slots to ClusterScan and related commands Signed-off-by: avifenesh <[email protected]> * feat: Implement continuous slot scanning until next unscanned slot Signed-off-by: jhpung <[email protected]> * fix: improve slot scanning logic when address not found Signed-off-by: jhpung <[email protected]> * add allowNonCoveredSlots option to ScanOptions and update GlideClusterClient Signed-off-by: avifenesh <[email protected]> * test: add tests for GlideClusterClient scan with allowNonCoveredSlots option Signed-off-by: avifenesh <[email protected]> * refactor: enhance cluster readiness check and improve error handling in scan tests Signed-off-by: avifenesh <[email protected]> --------- Signed-off-by: avifenesh <[email protected]> Signed-off-by: jhpung <[email protected]> Co-authored-by: jhpung <[email protected]>
1 parent 96d974d commit 210ba24

9 files changed

+250
-17
lines changed

CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
#### Changes
2+
* Node, Python: Add allow uncovered slots scanning flag option in cluster scan ([#2814](https://github.com/valkey-io/valkey-glide/pull/2814), [#2815](https://github.com/valkey-io/valkey-glide/pull/2815))
23
* Go: Add HINCRBY command ([#2847](https://github.com/valkey-io/valkey-glide/pull/2847))
34
* Go: Add HINCRBYFLOAT command ([#2846](https://github.com/valkey-io/valkey-glide/pull/2846))
45
* Go: Add SUNIONSTORE command ([#2805](https://github.com/valkey-io/valkey-glide/pull/2805))

eslint.config.mjs

+8
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
import eslint from "@eslint/js";
33
import prettierConfig from "eslint-config-prettier";
44
import tseslint from "typescript-eslint";
5+
import jsdoc from "eslint-plugin-jsdoc";
56

67
export default tseslint.config(
78
eslint.configs.recommended,
@@ -54,6 +55,13 @@ export default tseslint.config(
5455
next: "*",
5556
},
5657
],
58+
"@typescript-eslint/indent": ["error", 4, {
59+
"SwitchCase": 1,
60+
"ObjectExpression": 1,
61+
"FunctionDeclaration": {"parameters": "first"},
62+
"FunctionExpression": {"parameters": "first"},
63+
"ignoredNodes": ["TSTypeParameterInstantiation"]
64+
}],
5765
},
5866
},
5967
prettierConfig,

node/DEVELOPER.md

+1
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,7 @@ In order to run these tests, use:
156156
```bash
157157
npm run test-modules -- --cluster-endpoints=<address>:<port>
158158
```
159+
159160
Note: these tests don't run with standalone server as of now.
160161

161162
### REPL (interactive shell)

node/src/Commands.ts

+13
Original file line numberDiff line numberDiff line change
@@ -3857,6 +3857,19 @@ export interface ScanOptions extends BaseScanOptions {
38573857
type?: ObjectType;
38583858
}
38593859

3860+
/**
3861+
* Options for the SCAN command.
3862+
* `match`: The match filter is applied to the result of the command and will only include keys that match the pattern specified.
3863+
* `count`: `COUNT` is a just a hint for the command for how many elements to fetch from the server, the default is 10.
3864+
* `type`: The type of the object to scan.
3865+
* Types are the data types of Valkey: `string`, `list`, `set`, `zset`, `hash`, `stream`.
3866+
* `allowNonCoveredSlots`: If true, the scan will keep scanning even if slots are not covered by the cluster.
3867+
* By default, the scan will stop if slots are not covered by the cluster.
3868+
*/
3869+
export interface ClusterScanOptions extends ScanOptions {
3870+
allowNonCoveredSlots?: boolean;
3871+
}
3872+
38603873
/**
38613874
* Options specific to the ZSCAN command, extending from the base scan options.
38623875
*/

node/src/GlideClusterClient.ts

+9-8
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ import {
2323
FunctionStatsSingleResponse,
2424
InfoOptions,
2525
LolwutOptions,
26-
ScanOptions,
26+
ClusterScanOptions,
2727
createClientGetName,
2828
createClientId,
2929
createConfigGet,
@@ -146,7 +146,7 @@ export namespace GlideClusterClientConfiguration {
146146
/**
147147
* Configuration options for creating a {@link GlideClusterClient | GlideClusterClient}.
148148
*
149-
* Extends `BaseClientConfiguration` with properties specific to `GlideClusterClient`, such as periodic topology checks
149+
* Extends {@link BaseClientConfiguration | BaseClientConfiguration} with properties specific to `GlideClusterClient`, such as periodic topology checks
150150
* and Pub/Sub subscription settings.
151151
*
152152
* @remarks
@@ -579,7 +579,7 @@ export class GlideClusterClient extends BaseClient {
579579
*/
580580
protected scanOptionsToProto(
581581
cursor: string,
582-
options?: ScanOptions,
582+
options?: ClusterScanOptions,
583583
): command_request.ClusterScan {
584584
const command = command_request.ClusterScan.create();
585585
command.cursor = cursor;
@@ -596,6 +596,7 @@ export class GlideClusterClient extends BaseClient {
596596
command.objectType = options.type;
597597
}
598598

599+
command.allowNonCoveredSlots = options?.allowNonCoveredSlots ?? false;
599600
return command;
600601
}
601602

@@ -604,7 +605,7 @@ export class GlideClusterClient extends BaseClient {
604605
*/
605606
protected createClusterScanPromise(
606607
cursor: ClusterScanCursor,
607-
options?: ScanOptions & DecoderOption,
608+
options?: ClusterScanOptions & DecoderOption,
608609
): Promise<[ClusterScanCursor, GlideString[]]> {
609610
// separate decoder option from scan options
610611
const { decoder, ...scanOptions } = options || {};
@@ -633,7 +634,7 @@ export class GlideClusterClient extends BaseClient {
633634
*
634635
* @param cursor - The cursor object that wraps the scan state.
635636
* To start a new scan, create a new empty `ClusterScanCursor` using {@link ClusterScanCursor}.
636-
* @param options - (Optional) The scan options, see {@link ScanOptions} and {@link DecoderOption}.
637+
* @param options - (Optional) The scan options, see {@link ClusterScanOptions} and {@link DecoderOption}.
637638
* @returns A Promise resolving to an array containing the next cursor and an array of keys,
638639
* formatted as [`ClusterScanCursor`, `string[]`].
639640
*
@@ -651,14 +652,14 @@ export class GlideClusterClient extends BaseClient {
651652
* console.log(allKeys); // ["key1", "key2", "key3"]
652653
*
653654
* // Iterate over keys matching a pattern
654-
* await client.mset([{key: "key1", value: "value1"}, {key: "key2", value: "value2"}, {key: "notMykey", value: "value3"}, {key: "somethingElse", value: "value4"}]);
655+
* await client.mset([{key: "key1", value: "value1"}, {key: "key2", value: "value2"}, {key: "notMyKey", value: "value3"}, {key: "somethingElse", value: "value4"}]);
655656
* let cursor = new ClusterScanCursor();
656657
* const matchedKeys: GlideString[] = [];
657658
* while (!cursor.isFinished()) {
658659
* const [cursor, keys] = await client.scan(cursor, { match: "*key*", count: 10 });
659660
* matchedKeys.push(...keys);
660661
* }
661-
* console.log(matchedKeys); // ["key1", "key2", "notMykey"]
662+
* console.log(matchedKeys); // ["key1", "key2", "notMyKey"]
662663
*
663664
* // Iterate over keys of a specific type
664665
* await client.mset([{key: "key1", value: "value1"}, {key: "key2", value: "value2"}, {key: "key3", value: "value3"}]);
@@ -674,7 +675,7 @@ export class GlideClusterClient extends BaseClient {
674675
*/
675676
public async scan(
676677
cursor: ClusterScanCursor,
677-
options?: ScanOptions & DecoderOption,
678+
options?: ClusterScanOptions & DecoderOption,
678679
): Promise<[ClusterScanCursor, GlideString[]]> {
679680
return this.createClusterScanPromise(cursor, options);
680681
}

node/tests/ScanTest.test.ts

+162
Original file line numberDiff line numberDiff line change
@@ -12,13 +12,15 @@ import {
1212
GlideString,
1313
ObjectType,
1414
ProtocolVersion,
15+
GlideClusterClientConfiguration,
1516
} from "..";
1617
import { ValkeyCluster } from "../../utils/TestUtils.js";
1718
import {
1819
flushAndCloseClient,
1920
getClientConfigurationOption,
2021
getServerVersion,
2122
parseEndpoints,
23+
waitForClusterReady as isClusterReadyWithExpectedNodeCount,
2224
} from "./TestUtilities";
2325

2426
const TIMEOUT = 50000;
@@ -376,6 +378,166 @@ describe("Scan GlideClusterClient", () => {
376378
},
377379
TIMEOUT,
378380
);
381+
382+
it.each([ProtocolVersion.RESP2, ProtocolVersion.RESP3])(
383+
`GlideClusterClient scan with allowNonCoveredSlots %p`,
384+
async (protocol) => {
385+
const testCluster = await ValkeyCluster.createCluster(
386+
true,
387+
3,
388+
0,
389+
getServerVersion,
390+
);
391+
const config: GlideClusterClientConfiguration = {
392+
addresses: testCluster
393+
.getAddresses()
394+
.map(([host, port]) => ({ host, port })),
395+
protocol,
396+
};
397+
const testClient = await GlideClusterClient.createClient(config);
398+
399+
try {
400+
for (let i = 0; i < 10000; i++) {
401+
const result = await testClient.set(`${uuidv4()}`, "value");
402+
expect(result).toBe("OK");
403+
}
404+
405+
// Perform an initial scan to ensure all works as expected
406+
let cursor = new ClusterScanCursor();
407+
let result = await testClient.scan(cursor);
408+
cursor = result[0];
409+
expect(cursor.isFinished()).toBe(false);
410+
411+
// Set 'cluster-require-full-coverage' to 'no' to allow operations with missing slots
412+
await testClient.configSet({
413+
"cluster-require-full-coverage": "no",
414+
});
415+
416+
// Forget one server to simulate a node failure
417+
const addresses = testCluster.getAddresses();
418+
const addressToForget = addresses[0];
419+
const allOtherAddresses = addresses.slice(1);
420+
const idToForget = await testClient.customCommand(
421+
["CLUSTER", "MYID"],
422+
{
423+
route: {
424+
type: "routeByAddress",
425+
host: addressToForget[0],
426+
port: addressToForget[1],
427+
},
428+
},
429+
);
430+
431+
for (const address of allOtherAddresses) {
432+
await testClient.customCommand(
433+
["CLUSTER", "FORGET", idToForget as string],
434+
{
435+
route: {
436+
type: "routeByAddress",
437+
host: address[0],
438+
port: address[1],
439+
},
440+
},
441+
);
442+
}
443+
444+
// Wait for the cluster to stabilize after forgetting a node
445+
const ready = await isClusterReadyWithExpectedNodeCount(
446+
testClient,
447+
allOtherAddresses.length,
448+
);
449+
expect(ready).toBe(true);
450+
451+
// Attempt to scan without 'allowNonCoveredSlots', expecting an error
452+
// Since it might take time for the inner core to forget the missing node,
453+
// we retry the scan until the expected error is thrown.
454+
455+
const maxRetries = 10;
456+
let retries = 0;
457+
let errorReceived = false;
458+
459+
while (retries < maxRetries && !errorReceived) {
460+
retries++;
461+
cursor = new ClusterScanCursor();
462+
463+
try {
464+
while (!cursor.isFinished()) {
465+
result = await testClient.scan(cursor);
466+
cursor = result[0];
467+
}
468+
469+
// If scan completes without error, wait and retry
470+
await new Promise((resolve) =>
471+
setTimeout(resolve, 1000),
472+
);
473+
} catch (error) {
474+
if (
475+
error instanceof Error &&
476+
error.message.includes(
477+
"Could not find an address covering a slot, SCAN operation cannot continue",
478+
)
479+
) {
480+
// Expected error occurred
481+
errorReceived = true;
482+
} else {
483+
// Unexpected error, rethrow
484+
throw error;
485+
}
486+
}
487+
}
488+
489+
expect(errorReceived).toBe(true);
490+
491+
// Perform scan with 'allowNonCoveredSlots: true'
492+
cursor = new ClusterScanCursor();
493+
494+
while (!cursor.isFinished()) {
495+
result = await testClient.scan(cursor, {
496+
allowNonCoveredSlots: true,
497+
});
498+
cursor = result[0];
499+
}
500+
501+
expect(cursor.isFinished()).toBe(true);
502+
503+
// Get keys using 'KEYS *' from the remaining nodes
504+
const keys: GlideString[] = [];
505+
506+
for (const address of allOtherAddresses) {
507+
const result = await testClient.customCommand(
508+
["KEYS", "*"],
509+
{
510+
route: {
511+
type: "routeByAddress",
512+
host: address[0],
513+
port: address[1],
514+
},
515+
},
516+
);
517+
keys.push(...(result as GlideString[]));
518+
}
519+
520+
// Scan again with 'allowNonCoveredSlots: true' and collect results
521+
cursor = new ClusterScanCursor();
522+
const results: GlideString[] = [];
523+
524+
while (!cursor.isFinished()) {
525+
result = await testClient.scan(cursor, {
526+
allowNonCoveredSlots: true,
527+
});
528+
results.push(...result[1]);
529+
cursor = result[0];
530+
}
531+
532+
// Compare the sets of keys obtained from 'KEYS *' and 'SCAN'
533+
expect(new Set(results)).toEqual(new Set(keys));
534+
} finally {
535+
testClient.close();
536+
await testCluster.close();
537+
}
538+
},
539+
TIMEOUT,
540+
);
379541
});
380542

381543
//standalone tests

node/tests/TestUtilities.ts

+45
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,51 @@ function intoArrayInternal(obj: any, builder: string[]) {
8484
}
8585
}
8686

87+
// The function is used to check if the cluster is ready with the count nodes known command using the client supplied.
88+
// The way it works is by parsing the response of the CLUSTER INFO command and checking if the cluster_state is ok and the cluster_known_nodes is equal to the count.
89+
// If so, we know the cluster is ready, and it has the amount of nodes we expect.
90+
export async function waitForClusterReady(
91+
client: GlideClusterClient,
92+
count: number,
93+
): Promise<boolean> {
94+
const timeout = 20000; // 20 seconds timeout in milliseconds
95+
const startTime = Date.now();
96+
97+
while (true) {
98+
if (Date.now() - startTime > timeout) {
99+
return false;
100+
}
101+
102+
const clusterInfo = await client.customCommand(["CLUSTER", "INFO"]);
103+
// parse the response
104+
const clusterInfoMap = new Map<string, string>();
105+
106+
if (clusterInfo) {
107+
const clusterInfoLines = clusterInfo
108+
.toString()
109+
.split("\n")
110+
.filter((line) => line.length > 0);
111+
112+
for (const line of clusterInfoLines) {
113+
const [key, value] = line.split(":");
114+
115+
clusterInfoMap.set(key.trim(), value.trim());
116+
}
117+
118+
if (
119+
clusterInfoMap.get("cluster_state") == "ok" &&
120+
Number(clusterInfoMap.get("cluster_known_nodes")) == count
121+
) {
122+
break;
123+
}
124+
}
125+
126+
await new Promise((resolve) => setTimeout(resolve, 2000));
127+
}
128+
129+
return true;
130+
}
131+
87132
/**
88133
* accept any variable `v` and convert it into String, recursively
89134
*/

package.json

+7-5
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,14 @@
11
{
22
"devDependencies": {
3-
"@eslint/js": "^9.10.0",
3+
"@eslint/js": "9.17.0",
44
"@types/eslint__js": "^8.42.3",
55
"@types/eslint-config-prettier": "^6.11.3",
6-
"eslint": "9.14.0",
6+
"eslint": "9.17.0",
77
"eslint-config-prettier": "^9.1.0",
8-
"prettier": "^3.3.3",
9-
"typescript": "^5.6.2",
10-
"typescript-eslint": "^8.13"
8+
"eslint-plugin-jsdoc": "^50.6.1",
9+
"prettier": "3.4.2",
10+
"prettier-eslint": "16.3.0",
11+
"typescript": "5.7.2",
12+
"typescript-eslint": "8.18.1"
1113
}
1214
}

0 commit comments

Comments
 (0)