Java: Cluster SCAN request #1737

jduo · 2024-06-30T23:40:03Z

Issue #, if available:
N/A

Description of changes:
Support cluster SCAN commands.

This is a port of the Python part of #1623 for Java.

API and implementation notes:

ClusterScanCursor is an interface end users should work with. It is in a package exposed in the module.
NativeClusterScanCursor is an implementation of the above that is in an internal package. It wraps Rust cursor strings.
RedisClusterClient intercepts the result from Rust to wrap the cursor string in a NativeClusterScanCursor
The caller is responsible for closing ClusterScanCursor. If they don't, it'll get cleaned up at GC-time, but this is non-deterministic.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

java/client/src/main/java/glide/api/models/commands/scan/ScanOptions.java

CHANGELOG.md

glide-core/src/request_type.rs

java/client/src/main/java/glide/api/RedisClient.java

java/client/src/main/java/glide/api/RedisClusterClient.java

java/client/src/main/java/glide/api/models/commands/scan/ClusterScanCursor.java

java/client/src/main/java/glide/api/commands/GenericClusterCommands.java

java/client/src/main/java/glide/api/models/commands/scan/ClusterScanCursor.java

java/client/src/main/java/glide/api/models/commands/scan/ScanOptions.java

Signed-off-by: Andrew Carbonetto <[email protected]>

Notes: - ClusterScanCursor is an interface end users should work with. It is in a package exposed in the module. - NativeClusterScanCursor is an implementation of the above that is in an internal package. It wraps Rust cursor strings. - RedisClusterClient intercepts the result from Rust to wrap the cursor string in a NativeClusterScanCursor - The caller is responsible for closing ClusterScanCursor. If they don't, it'll get cleaned up at GC-time, but this is non-deterministic.

Look for the "finished" string

The same hash could potentially be reused.

* Validate that either the initial cursor is used, or an unfinished cursor. * Test-by-reference for the initial cursor instead of evaluating its string. * Clean-up unfinished cursors if they get re-used in scan. * Throw errors if a finished cursor is passed to scan.

- The cursor object manages updating cursor handles and internal state - It auto-cleans at the end of the data - The user can also dispose of the cursor early

When using the ScanOptions type, pass native names through to the rust layer when using cluster scans, and enum names when using standalone scans.

Also flushall before each test to avoid flakiness from leftover state

For consistency with Python

Move access to this to an internal detail interface. The user shouldn't need this string themselves.

resolve conflicts

jonathanl-bq

Signing off on the Rust portion.

java/src/lib.rs

java/client/src/main/java/glide/api/commands/GenericClusterCommands.java

java/client/src/main/java/glide/api/logging/Logger.java

java/client/src/main/java/glide/ffi/resolvers/ObjectTypeResolver.java

Yury-Fridlyand

Nice!
I didn't review tests yet, but I trust you.

java/client/src/main/java/glide/api/commands/GenericCommands.java

Yury-Fridlyand · 2024-07-03T01:55:34Z

java/client/src/main/java/glide/api/logging/Logger.java

+     */
+    public static void log(
+            @NonNull Level level, @NonNull String logIdentifier, @NonNull Throwable throwable) {
+        // TODO: Add the corresponding API to Python and Node.js.


It is better to post such TODO as a GH task

Yury-Fridlyand · 2024-07-03T01:56:51Z

java/client/src/main/java/glide/api/models/Script.java

@@ -35,7 +37,11 @@ public <T> Script(T code, Boolean binarySafeOutput) {
    /** Drop the linked script from glide-rs <code>code</code>. */
    @Override
    public void close() throws Exception {
-        dropScript(hash);
+


Do we need to synchronise this?

java/client/src/main/java/glide/api/models/commands/scan/ClusterScanCursor.java

java/client/src/main/java/glide/api/models/commands/scan/ScanOptions.java

java/client/src/main/java/glide/managers/CommandManager.java

python/python/tests/test_async_client.py

Signed-off-by: Andrew Carbonetto <[email protected]>

acarbonetto · 2024-07-03T04:42:16Z

java/client/src/main/java/glide/api/models/commands/scan/ClusterScanCursor.java

+
+                @Override
+                public void releaseCursorHandle() {
+                    // Ignore.


It overrides the interface and doesn't destroy the instance object.
That way James has smartly prevented anyone from destroying the statically created INITIAL_CURSOR_INSTANCE.

* Added Scan logic to Glide-core * added scan to python * added changes to changelog * fixed comments * changed from hash to nanoid * fixed typing * fixed comments * Stubbing for cluster scan * Update ScanOptions Signed-off-by: Andrew Carbonetto <[email protected]> * ClusterScan implementation Notes: - ClusterScanCursor is an interface end users should work with. It is in a package exposed in the module. - NativeClusterScanCursor is an implementation of the above that is in an internal package. It wraps Rust cursor strings. - RedisClusterClient intercepts the result from Rust to wrap the cursor string in a NativeClusterScanCursor - The caller is responsible for closing ClusterScanCursor. If they don't, it'll get cleaned up at GC-time, but this is non-deterministic. * Make cursor termination behave as Python does Look for the "finished" string * Unrelated: avoid double-dropping of scripts The same hash could potentially be reused. * Better state management of cursors * Validate that either the initial cursor is used, or an unfinished cursor. * Test-by-reference for the initial cursor instead of evaluating its string. * Clean-up unfinished cursors if they get re-used in scan. * Throw errors if a finished cursor is passed to scan. * Update design for an iterative cursor - The cursor object manages updating cursor handles and internal state - It auto-cleans at the end of the data - The user can also dispose of the cursor early * Change the API to match Python * Unit tests * Fix integration tests When using the ScanOptions type, pass native names through to the rust layer when using cluster scans, and enum names when using standalone scans. * Integration test for cluster scan options * Spotless * Add more integration tests Also flushall before each test to avoid flakiness from leftover state * Move population of the ScanOptions protobuf to CommandManager * Rename clusterScan() method to scan() For consistency with Python * Add javadoc comments * Spotless * Abstract the cursor string from the public API Move access to this to an internal detail interface. The user shouldn't need this string themselves. * Spotless * Revert accidental changes resolve conflicts * Address review comments * Get protocol string constants from Rust instead of hard-coding them * Add integration testing for the Stream type filter * Spotless * Update scan documentation Signed-off-by: Andrew Carbonetto <[email protected]> --------- Signed-off-by: Andrew Carbonetto <[email protected]> Co-authored-by: avifenesh <[email protected]> Co-authored-by: Andrew Carbonetto <[email protected]>

Yury-Fridlyand added the java issues and fixes related to the java client label Jul 1, 2024

jduo mentioned this pull request Jul 1, 2024

Cluster Scan - glide-core and python #1623

Merged

acarbonetto reviewed Jul 1, 2024

View reviewed changes

java/client/src/main/java/glide/api/models/commands/scan/ScanOptions.java Outdated Show resolved Hide resolved

jduo marked this pull request as ready for review July 2, 2024 00:30

jduo requested a review from a team as a code owner July 2, 2024 00:30

acarbonetto reviewed Jul 2, 2024

View reviewed changes

jduo force-pushed the java/dev_jduo_cluster_scan branch 3 times, most recently from b695e4a to dbaabe3 Compare July 2, 2024 18:33

avifenesh and others added 21 commits July 2, 2024 15:15

Added Scan logic to Glide-core

4385ced

added scan to python

1620543

added changes to changelog

164d6bb

fixed comments

2a52e77

changed from hash to nanoid

9bfadcd

fixed typing

e6e3cf6

fixed comments

f0c7be1

Stubbing for cluster scan

396aefa

Update ScanOptions

398dc82

Signed-off-by: Andrew Carbonetto <[email protected]>

Make cursor termination behave as Python does

d3767ea

Look for the "finished" string

Unrelated: avoid double-dropping of scripts

81df50a

The same hash could potentially be reused.

Update design for an iterative cursor

24d94f9

- The cursor object manages updating cursor handles and internal state - It auto-cleans at the end of the data - The user can also dispose of the cursor early

Change the API to match Python

6cd05a7

Unit tests

7c80cd4

Fix integration tests

8fc7d62

When using the ScanOptions type, pass native names through to the rust layer when using cluster scans, and enum names when using standalone scans.

Integration test for cluster scan options

7c6e05d

Spotless

693363c

Add more integration tests

92d7f60

Also flushall before each test to avoid flakiness from leftover state

Move population of the ScanOptions protobuf to CommandManager

4bb2cc7

jduo added 8 commits July 2, 2024 15:15

Rename clusterScan() method to scan()

277e57e

For consistency with Python

Add javadoc comments

da2631c

Spotless

d18f07d

Abstract the cursor string from the public API

9b890ed

Move access to this to an internal detail interface. The user shouldn't need this string themselves.

Spotless

3894f71

Revert accidental changes

ef3f3d6

resolve conflicts

Address review comments

c0af505

Get protocol string constants from Rust instead of hard-coding them

7948b57

jduo force-pushed the java/dev_jduo_cluster_scan branch from 998042d to 7948b57 Compare July 2, 2024 22:15

Add integration testing for the Stream type filter

180a509

jonathanl-bq approved these changes Jul 2, 2024

View reviewed changes

java/src/lib.rs Outdated Show resolved Hide resolved

java/src/lib.rs Show resolved Hide resolved

Spotless

0d001e8

acarbonetto reviewed Jul 3, 2024

View reviewed changes

java/client/src/main/java/glide/api/commands/GenericClusterCommands.java Show resolved Hide resolved

acarbonetto reviewed Jul 3, 2024

View reviewed changes

Yury-Fridlyand reviewed Jul 3, 2024

View reviewed changes

Update scan documentation

4110295

Signed-off-by: Andrew Carbonetto <[email protected]>

acarbonetto approved these changes Jul 3, 2024

View reviewed changes

Elen-Ghulam approved these changes Jul 3, 2024

View reviewed changes

Yury-Fridlyand approved these changes Jul 3, 2024

View reviewed changes

acarbonetto merged commit 867cd9b into valkey-io:main Jul 3, 2024
17 checks passed

acarbonetto deleted the java/dev_jduo_cluster_scan branch July 3, 2024 05:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Java: Cluster SCAN request #1737

Java: Cluster SCAN request #1737

jduo commented Jun 30, 2024

jonathanl-bq left a comment

Yury-Fridlyand left a comment

Yury-Fridlyand Jul 3, 2024

Yury-Fridlyand Jul 3, 2024

acarbonetto Jul 3, 2024

Java: Cluster SCAN request #1737

Java: Cluster SCAN request #1737

Conversation

jduo commented Jun 30, 2024

jonathanl-bq left a comment

Choose a reason for hiding this comment

Yury-Fridlyand left a comment

Choose a reason for hiding this comment

Yury-Fridlyand Jul 3, 2024

Choose a reason for hiding this comment

Yury-Fridlyand Jul 3, 2024

Choose a reason for hiding this comment

acarbonetto Jul 3, 2024

Choose a reason for hiding this comment