Java: add scan (binary) for cluster commands #1837
Conversation
Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>
| return commandManager | ||
| .submitClusterScan(cursor, options, this::handleArrayResponseBinary) | ||
| .thenApply( | ||
| result -> new Object[] {new NativeClusterScanCursor(result[0].toString()), result[1]}); |
There was a problem hiding this comment.
I think this is fine, but I'd be curious if the Rust layer can ever return non-UTF-8 binary data.
There was a problem hiding this comment.
for the cursor...?
There was a problem hiding this comment.
yeah, I was thinking that too.
I've asked Avi. I'm pretty sure we're good (otherwise our String version would fail).
| @@ -1072,6 +1080,14 @@ public CompletableFuture<Object[]> scan(ClusterScanCursor cursor, ScanOptions op | |||
| result -> new Object[] {new NativeClusterScanCursor(result[0].toString()), result[1]}); | |||
| } | |||
|
|
|||
| @Override | |||
| public CompletableFuture<Object[]> scanBinary(ClusterScanCursor cursor, ScanOptions options) { | |||
There was a problem hiding this comment.
I afraid you need new class ScanOptionsBinary
There was a problem hiding this comment.
The Binary options are useless and don't provide any additional benefit. They are phasing them out.
There was a problem hiding this comment.
There is String matchPattern unfortunately
There was a problem hiding this comment.
protobuf doesn't accept anything but strings, so we cannot sent binary scan options
java/client/src/main/java/glide/api/commands/GenericClusterCommands.java
Outdated
Show resolved
Hide resolved
| @@ -251,6 +251,66 @@ public interface GenericClusterCommands { | |||
| */ | |||
| CompletableFuture<Object[]> scan(ClusterScanCursor cursor); | |||
|
|
|||
| /** | |||
| * Incrementally iterates over the keys in the Cluster. | |||
There was a problem hiding this comment.
The doc is TLDR. I think you just copied it, but it is good to say what is the difference in the functions. Why/When a user should use this one?
There was a problem hiding this comment.
They don't change the documentation between String and GlideString. I think they wanted to consolidate the two APIs at some point...
There was a problem hiding this comment.
Maybe...
But these 2 funcs have 1:1 docs and signatures - really confusing.
On another hand, doc can be changed later. Up to you.
There was a problem hiding this comment.
I'd like them to decide on how to change these docs...
java/client/src/main/java/glide/api/commands/GenericClusterCommands.java
Show resolved
Hide resolved
| * immediately free resources tied to the cursor. Note that this makes the cursor unusable in | ||
| * subsequent calls to <code>SCAN</code>. | ||
| * | ||
| * @see ClusterScanCursor for more details about how to use the cursor. |
There was a problem hiding this comment.
meh - the link isn't necessary. I removed them. ClusterScanCursor is mentioned numerous times in the javadoc.
There was a problem hiding this comment.
Please delete this one too
java/integTest/src/test/java/glide/cluster/ClusterClientTests.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>
| * <p>This command is similar to the <code>SCAN</code> command, but it is designed to work in a | ||
| * Cluster environment. The main difference is that this command uses a {@link ClusterScanCursor} | ||
| * object to manage iterations. For more information about the Cluster Scan implementation, see <a | ||
| * href="https://github.com/aws/glide-for-redis/wiki/General-Concepts#cluster-scan">Cluster |
There was a problem hiding this comment.
The doc linked here has a TODO at the bottom still. Is that something we should address before release?
There was a problem hiding this comment.
I don't know. That's Avi's document? Might we worthwhile taking a look next week.
Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>
java/client/src/main/java/glide/api/commands/GenericClusterCommands.java
Outdated
Show resolved
Hide resolved
| * immediately free resources tied to the cursor. Note that this makes the cursor unusable in | ||
| * subsequent calls to <code>SCAN</code>. | ||
| * | ||
| * @see ClusterScanCursor for more details about how to use the cursor. |
There was a problem hiding this comment.
Please delete this one too
Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>
Signed-off-by: Andrew Carbonetto <andrew.carbonetto@improving.com>
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.