LCS + mset + msetnx #1698

aws-talbenjo · 2024-06-27T15:56:40Z

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.

Yury-Fridlyand · 2024-06-28T03:32:08Z

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

+        if (keyValueMap.isEmpty()) {
+            throw new IllegalArgumentException("empty map");
+        }
+


You can omit this. We have tests where intentionally send to redis an empty map

Not sure I follow. This isn't for testing purpose, if we remove this and get an empty map with mset we'll recieve
NoSuchElementException so isn't his necessary?

if map is empty - submit command with 0 args. But track arg type to avoid issues.

@Yury-Fridlyand so basically you want the below code to be enclosed within:

if (!keyValueMap.isEmpty()) { ArgType firstValue = keyValueMap.keySet().iterator().next(); .. } else { // submit empty array return commandManager.submitNewCommand(MSet, new String[] {}, this::handleStringResponse); }

Yury-Fridlyand · 2024-06-28T03:32:43Z

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

-    public CompletableFuture<String> mset(@NonNull Map<String, String> keyValueMap) {
-        String[] args = convertMapToKeyValueStringArray(keyValueMap);
-        return commandManager.submitNewCommand(MSet, args, this::handleStringResponse);
+    public <ArgType> CompletableFuture<String> mset(@NonNull Map<ArgType, ArgType> keyValueMap) {


Better to duplicate code - as I understand, all APIs with String would be removed later

Since the functions signature differ only in template input Java doesn't allow such overload. I.E this kind of overloading isn't permitted:
someType foo(Map<String,String> ...);
someType foo(Map<GlideString,GlideString> ...);

That's why i had to resort to templating.

Yes, Java's "type erasure" thingi

Yury-Fridlyand · 2024-06-28T03:33:23Z

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

@@ -263,7 +263,7 @@ public interface StringBaseCommands {
     * assert result.equals("OK"));
     * }</pre>
     */
-    CompletableFuture<String> mset(Map<String, String> keyValueMap);
+    <ArgType> CompletableFuture<String> mset(Map<ArgType , ArgType> keyValueMap);


If you decide to not duplicate - describe what is ArgType in the doc.
But this will be inconsistend with all other APIs

IIUC, we can't really duplicate these functions. Java's type erasure does not differentiate between:

void foo(Map<GlideString, GlideString> m)

and

void foo(Map<String, String> m)

Both have the same signature of void foo(Map)
The options are:

Add custom method name msetBinary or

Generalize the method (we could also be using Map<?, ?>

With the second approach, the user gets the "feel" of overloading: he can still call the method with either Map<String, String> or Map<GlideString, GlideString> (anything else will throw)

Yury-Fridlyand · 2024-06-28T03:33:34Z

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

+            return commandManager.submitNewCommand(
+                    MSet, (GlideString[]) args, this::handleStringResponse);
+        } else {
+            throw new IllegalArgumentException("Expected String or GlideString");


If you decide to not duplicate - add a test for this case

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

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

Yury-Fridlyand · 2024-07-01T05:07:34Z

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

+        if (keyValueMap.isEmpty()) {
+            throw new IllegalArgumentException("empty map");
+        }
+


if map is empty - submit command with 0 args. But track arg type to avoid issues.

Yury-Fridlyand · 2024-07-01T05:07:52Z

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

-        String[] args = convertMapToKeyValueStringArray(keyValueMap);
-        return commandManager.submitNewCommand(MSetNX, args, this::handleBooleanResponse);
+    public <ArgType> CompletableFuture<Boolean> msetnx(@NonNull Map<ArgType, ArgType> keyValueMap) {
+        if (keyValueMap.isEmpty()) {


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

…io#1693) * Added support for decr decrby del hdel pfadd pfcount pfmerge --------- Co-authored-by: Ubuntu <ubuntu@ip-172-31-26-174.us-west-2.compute.internal>

- Java transaction: Hash + List commands are now binary compatible + using the leaked ptr approach if required - Code cleanup

Yury-Fridlyand

Can't approve nor reject.
You're lucky that jacoco is still disabled. It will alert for non-100% code coverage by tests.

Yury-Fridlyand · 2024-07-02T04:53:12Z

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

@@ -272,15 +274,17 @@ public interface StringBaseCommands {
     * @apiNote When in cluster mode, all keys in <code>keyValueMap</code> must map to the same hash
     *     slot.
     * @see <a href="https://redis.io/commands/msetnx/">redis.io</a> for details.
-     * @param keyValueMap A key-value map consisting of keys and their respective values to set.
+     * @param keyValueMap A key-value map consisting of keys and their respective values to set. Note


Suggested change

* @param keyValueMap A key-value map consisting of keys and their respective values to set. Note

* @param keyValueMap A key-value map consisting of keys and their respective values to set.

Yury-Fridlyand · 2024-07-02T04:53:39Z

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

@@ -336,8 +332,7 @@ public boolean isBinarySafeOutput() {
     * }</pre>
     */
    public <ArgType> T customCommand(ArgType[] args) {
-        ArgsArray commandArgs = buildArgs(args);
-        protobufTransaction.addCommands(buildCommand(CustomCommand, commandArgs));
+        protobufTransaction.addCommands(buildCommand(CustomCommand, newArgsBuilder().add(args)));


Rebase issue? PLease clean up branch before merging

Ubuntu added 3 commits June 27, 2024 12:46

Support for LCS

59ef80e

Support mset

78b03f6

mset fixed

1cfa3c8

aws-talbenjo requested a review from a team as a code owner June 27, 2024 15:56

Yury-Fridlyand reviewed Jun 28, 2024

View reviewed changes

Ubuntu added 4 commits June 30, 2024 08:28

Merge branch 'main' into tal-ls-2

eeebcc0

mset tests + doc

04d3eb8

Support for msetnx

6f6377b

nit: spotlessApply

5123052

aws-talbenjo changed the title ~~LCS + mset~~ LCS + mset + msetnx Jun 30, 2024

Wrong handler for msetnx

4f10f79

Yury-Fridlyand added the java issues and fixes related to the java client label Jun 30, 2024

Yury-Fridlyand reviewed Jul 1, 2024

View reviewed changes

Ubuntu and others added 5 commits July 1, 2024 09:03

doc change for generics in mset, msetnx

bb29ba9

Merge branch 'main' into tal-ls-2

3d12fb6

Added support for decr decrby del hdel pfadd pfcount pfmerge (valkey-…

0fe9721

…io#1693) * Added support for decr decrby del hdel pfadd pfcount pfmerge --------- Co-authored-by: Ubuntu <ubuntu@ip-172-31-26-174.us-west-2.compute.internal>

- Java transaction: Hash + List commands (valkey-io#1744)

8167b60

- Java transaction: Hash + List commands are now binary compatible + using the leaked ptr approach if required - Code cleanup

nit: reapply spotless

b1073ad

Yury-Fridlyand reviewed Jul 2, 2024

View reviewed changes

aws-talbenjo marked this pull request as draft July 4, 2024 10:43

alon-arenberg closed this Jul 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LCS + mset + msetnx #1698

LCS + mset + msetnx #1698

aws-talbenjo commented Jun 27, 2024

Yury-Fridlyand Jun 28, 2024

aws-talbenjo Jun 30, 2024

Yury-Fridlyand Jul 1, 2024

eifrah-aws Jul 4, 2024

Yury-Fridlyand Jul 4, 2024

Yury-Fridlyand Jun 28, 2024

aws-talbenjo Jun 30, 2024

eifrah-aws Jul 1, 2024

Yury-Fridlyand Jun 28, 2024 •

edited

Loading

eifrah-aws Jul 4, 2024 •

edited

Loading

Yury-Fridlyand Jun 28, 2024

Yury-Fridlyand Jul 1, 2024

Yury-Fridlyand Jul 1, 2024

Yury-Fridlyand left a comment

Yury-Fridlyand Jul 2, 2024

Yury-Fridlyand Jul 2, 2024

	* @param keyValueMap A key-value map consisting of keys and their respective values to set. Note
	* @param keyValueMap A key-value map consisting of keys and their respective values to set.

LCS + mset + msetnx #1698

LCS + mset + msetnx #1698

Conversation

aws-talbenjo commented Jun 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yury-Fridlyand Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

eifrah-aws Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yury-Fridlyand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yury-Fridlyand Jun 28, 2024 •

edited

Loading

eifrah-aws Jul 4, 2024 •

edited

Loading