Valkey-8: add BY/GET support for SORT/RO in cluster mode by shohamazon · Pull Request #2252 · valkey-io/valkey-glide

shohamazon · 2024-09-09T11:25:58Z

No description provided.

Yury-Fridlyand · 2024-09-10T20:55:35Z

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

+     * @apiNote When in cluster mode, both <code>key</code> and the patterns specified in {@link
+     *     SortOptions#byPattern} and {@link SortOptions#getPatterns} must hash to the same slot. The
+     *     use of {@link SortOptions#byPattern} and {@link SortOptions#getPatterns} in cluster mode is
+     *     supported since Valkey version 8.0.


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

Yury-Fridlyand · 2024-09-10T20:58:12Z

java/integTest/src/test/java/glide/cluster/CommandTests.java

Please add sross slot test there

Yury-Fridlyand · 2024-09-10T21:00:19Z

node/src/Commands.ts

+ *     slot as the key, and this is supported only since Valkey version 8.0.
 */
-export type SortOptions = SortBaseOptions & {
+export type SortOptions = {


According to new starndards introduced by Avi, this should an interface

Yury-Fridlyand · 2024-09-10T21:00:39Z

node/src/GlideClient.ts

    GlideString,
-    PubSubMsg,
-    ReadFrom, // eslint-disable-line @typescript-eslint/no-unused-vars
+    PubSubMsg, // eslint-disable-line @typescript-eslint/no-unused-vars


Please revert

Yury-Fridlyand · 2024-09-10T21:00:46Z

node/src/GlideClusterClient.ts

    GlideString,
-    PubSubMsg,
-    ReadFrom, // eslint-disable-line @typescript-eslint/no-unused-vars
+    PubSubMsg, // eslint-disable-line @typescript-eslint/no-unused-vars


Yury-Fridlyand · 2024-09-10T21:00:56Z

node/src/Transaction.ts

    GlideString,
-    HashDataType,
-    ReadFrom, // eslint-disable-line @typescript-eslint/no-unused-vars
+    HashDataType, // eslint-disable-line @typescript-eslint/no-unused-vars


Yury-Fridlyand · 2024-09-10T21:02:22Z

node/tests/SharedTests.ts

                    await client.sinter(
                        [Buffer.from(key1), Buffer.from(key2)],
-                        { decoder: Decoder.Bytes },
+                        {


why? linter? why?

node/src/GlideClusterClient.ts

acarbonetto · 2024-09-11T16:57:17Z

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

     * This command is routed depending on the client's {@link ReadFrom} strategy.
     *
     * @since Valkey 7.0 and above.
+     * @see <a href="https://valkey.io/commands/sort_ro/">valkey.io</a> for details.


404 not found

acarbonetto · 2024-09-11T16:57:23Z

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

     * This command is routed depending on the client's {@link ReadFrom} strategy.
     *
     * @since Valkey 7.0 and above.
+     * @see <a href="https://valkey.io/commands/sort_ro/">valkey.io</a> for details.


404 not found

acarbonetto · 2024-09-11T16:58:08Z

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

+     *     use of {@link SortOptions#byPattern} and {@link SortOptions#getPatterns} in cluster mode is
+     *     supported since Valkey version 8.0.
+     * @since Valkey 7.0 and above.
+     * @see <a href="https://valkey.io/commands/sort_ro/">valkey.io</a> for details.


404 not found
I'm not sure we should be added a reference here that doesn't exist. It kind of looks bad. Maybe just add a link to sort?

I dont know why this page doesnt exist

acarbonetto · 2024-09-11T16:58:16Z

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

+     *     same slot. The use of {@link SortOptionsBinary#byPattern} and {@link
+     *     SortOptionsBinary#getPatterns} in cluster mode is supported since Valkey version 8.0.
+     * @since Valkey 7.0 and above.
+     * @see <a href="https://valkey.io/commands/sort_ro/">valkey.io</a> for details.


yipin-chen · 2024-09-11T20:26:53Z

java/integTest/src/test/java/glide/SharedCommandTests.java

+    @MethodSource("getClients")
+    public void sort_with_pattern(BaseClient client) {
+        if (client instanceof GlideClusterClient) {
+            assumeTrue(SERVER_VERSION.isGreaterThanOrEqualTo("7.9.0"), "This feature added in version 8");


I know Valkey 7.9 is 8 (while in RC). I am just wondering whether we need to update the test to use "8.0.0" or just leave "7.9.0" as is.

I have an issue to change that once valkey 8 is released

Signed-off-by: Shoham Elias <shohame@amazon.com>

shohamazon force-pushed the sort-valkey-8 branch 2 times, most recently from 8ad4757 to 8b95058 Compare September 9, 2024 13:44

shohamazon added python 🐍 Python wrapper node 🐢 Node.js wrapper java ☕ issues and fixes related to the java client Valkey-8 labels Sep 9, 2024

shohamazon force-pushed the sort-valkey-8 branch 2 times, most recently from daeef61 to c03ac99 Compare September 10, 2024 14:43

shohamazon added the Release blocker 🛡️ Can't release without. label Sep 10, 2024

shohamazon marked this pull request as ready for review September 10, 2024 15:01

shohamazon requested a review from a team as a code owner September 10, 2024 15:01

shohamazon requested review from Yury-Fridlyand and acarbonetto September 10, 2024 15:06

Yury-Fridlyand reviewed Sep 10, 2024

View reviewed changes

shohamazon force-pushed the sort-valkey-8 branch from c03ac99 to 9b7ba33 Compare September 11, 2024 14:56

shohamazon mentioned this pull request Sep 11, 2024

Valkey 8 support #2033

Closed

7 tasks

acarbonetto approved these changes Sep 11, 2024

View reviewed changes

yipin-chen reviewed Sep 11, 2024

View reviewed changes

shohamazon force-pushed the sort-valkey-8 branch 2 times, most recently from e75d3d0 to e4f14f2 Compare September 12, 2024 11:23

Valkey-8: add BY/GET support for SORT/RO in cluster mode

4569e52

Signed-off-by: Shoham Elias <shohame@amazon.com>

shohamazon force-pushed the sort-valkey-8 branch from e4f14f2 to 4569e52 Compare September 12, 2024 11:36

shohamazon merged commit 783a9f0 into valkey-io:main Sep 12, 2024

shohamazon deleted the sort-valkey-8 branch September 12, 2024 11:57

Conversation

shohamazon commented Sep 9, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yipin-chen Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yipin-chen Sep 11, 2024 •

edited

Loading