Java: Add GET & SET commands #919

acarbonetto · 2024-02-07T22:48:14Z

Issue #, if available:

Description of changes:

This PR is dependent on: #917

This PR adds get/set/ping/info calls to the standalone, and cluster-mode:

standalone
- get
- set
- set with options
cluster
- get
- set
- set with options

Examples:

// Single commands
RedisClient client = RedisClient.createClient(config).get();
Ok setResult = client.set("key", "value").get(); // returns "OK"
String getResult = client.get("key").get(); // returns "value"

// Cluster-mode single commands
RedisClusterClient clusterClient = RedisClusterClient.createClient(config).get();
Ok setResult = clusterClient.set("key", "value").get(); // returns "OK"
String getResult = clusterClient.get("key").get(); // returns "value"

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/BaseClient.java

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

acarbonetto · 2024-02-09T05:56:41Z

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

@@ -31,6 +31,6 @@ public static CompletableFuture<RedisClient> CreateClient(RedisClientConfigurati

    @Override
    public CompletableFuture<Object> customCommand(String[] args) {
-        return commandManager.submitNewCommand(CustomCommand, args, this::handleObjectResponse);
+        return commandManager.submitNewCommand(CustomCommand, args, this::handleObjectOrNullResponse);


Renamed function handleObjectOrNullResponse() to be consistent with how sometimes we allow for null and sometimes not.

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

shachlanAmazon · 2024-02-09T11:46:40Z

java/client/src/main/java/glide/api/models/commands/SetOptions.java

+     * If <code>conditionalSet</code> is not set the value will be set regardless of prior value
+     * existence. If value isn't set because of the condition, command will return <code>null</code>.
+     */
+    private final ConditionalSet conditionalSet;


this should be an optional, in order to properly represent that this value can be not set.
It's better to make types explicit than rely on enum being null, which is a (weird) language specific ability.

Objects in the @Builder pattern are all optional unless defaulted (@Builder.Default). If not set, the value will be null. I dislike using nulls, but as part of a Builder, we can contain the pattern and trust Lombok.
Unfortunately, Lombok doesn't play well with Optionals (actually, the Lombok developers actively dislike the Optional pattern). We would have to create our own Builders to work with Optionals, or we can stick with what Lombok provides and use nulls if the value isn't set prior.

shachlanAmazon · 2024-02-09T11:46:49Z

java/client/src/main/java/glide/api/models/commands/SetOptions.java

+    private final boolean returnOldValue;
+
+    /** If not set, no expiry time will be set for the value. */
+    private final Expiry expiry;


shachlanAmazon · 2024-02-09T11:47:54Z

java/client/src/main/java/glide/api/models/commands/SetOptions.java

+         * The amount of time to live before the key expires. Ignored when {@link
+         * ExpiryType#KEEP_EXISTING} type is set.
+         */
+        private Long count;


wouldn't a Duration type be more representative of the data?

Generally, yes. This object is not being used to calculate duration, but to map to string for a Set command.
But this just a map to the Set command (which takes a number/Integer). Documentation says "Integer", but Bar commented that Long is more appropriate.
see: https://redis.io/commands/set/

shachlanAmazon · 2024-02-09T11:55:42Z

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

+     * @param value The value to store with the given <code>key</code>.
+     * @return Response from Redis containing <code>"OK"</code>.
+     */
+    CompletableFuture<String> set(String key, String value);


shouldn't the arguments be marked with @NonNull?

The annotation is an implementation detail. It doesn't do anything in the interface.
I can add it for "documentation" purposes?

shachlanAmazon · 2024-02-09T11:56:44Z

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

+    @ParameterizedTest
+    @MethodSource("getClients")
+    public void set_requires_a_value(BaseClient client) {
+        assertThrows(NullPointerException.class, () -> client.set("SET", null));


minor - the null exception tests can be in UTs, not ITs.

San can move these to the next PR?

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

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

acarbonetto requested a review from a team as a code owner February 7, 2024 22:48

Yury-Fridlyand approved these changes Feb 8, 2024

View reviewed changes

acarbonetto mentioned this pull request Feb 8, 2024

Java: Add GET, SET, INFO and PING commands #894

Closed

acarbonetto self-assigned this Feb 8, 2024

acarbonetto added the java issues and fixes related to the java client label Feb 8, 2024

acarbonetto force-pushed the java/integ_acarbo_add_getset_only branch from 0a10654 to 65c893a Compare February 8, 2024 21:10

Yury-Fridlyand approved these changes Feb 8, 2024

View reviewed changes

java/client/src/main/java/glide/api/BaseClient.java Outdated Show resolved Hide resolved

java/client/src/main/java/glide/api/RedisClusterClient.java Outdated Show resolved Hide resolved

acarbonetto commented Feb 9, 2024

View reviewed changes

acarbonetto added 10 commits February 8, 2024 22:27

Update commandmanager to remove optional<route> argument

a69e5ce

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

Java: Add GET & SET commands

0aefec1

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

Move IT tests to SharedCommandTests

8b73360

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

Spotless

3cca0b9

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

Add nonnull check to API

7540d6b

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

Fix from merge

2803616

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

Update handleRedisResponse

037808c

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

Use @timeout annotation

143d4f5

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

Remove extra @nonnull annotations

5279a89

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

Merge main

b445dc6

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

acarbonetto force-pushed the java/integ_acarbo_add_getset_only branch from e17fec0 to b445dc6 Compare February 9, 2024 06:38

acarbonetto added 4 commits February 8, 2024 22:40

Add NonNull

9642149

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

Add back IT tests

47f14a2

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

Remove extra tests

9991ef2

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

Add DELME tests

c0e379a

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

shachlanAmazon approved these changes Feb 9, 2024

View reviewed changes

Yury-Fridlyand approved these changes Feb 9, 2024

View reviewed changes

Yury-Fridlyand mentioned this pull request Feb 9, 2024

Java: Add ExamplesApp and Update benchmarkingApp to both have Java/Glide-for-redis client #896

Merged

Minor: clean tests; throw exception

90e13f9

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

acarbonetto merged commit 8f35f5a into valkey-io:main Feb 9, 2024
11 checks passed

acarbonetto deleted the java/integ_acarbo_add_getset_only branch February 9, 2024 20:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Java: Add GET & SET commands #919

Java: Add GET & SET commands #919

acarbonetto commented Feb 7, 2024

acarbonetto Feb 9, 2024 •

edited

Loading

shachlanAmazon Feb 9, 2024

acarbonetto Feb 9, 2024

shachlanAmazon Feb 9, 2024

shachlanAmazon Feb 9, 2024

acarbonetto Feb 9, 2024

shachlanAmazon Feb 9, 2024

acarbonetto Feb 9, 2024

shachlanAmazon Feb 9, 2024

acarbonetto Feb 9, 2024

Java: Add GET & SET commands #919

Java: Add GET & SET commands #919

Conversation

acarbonetto commented Feb 7, 2024

Examples:

acarbonetto Feb 9, 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

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

acarbonetto Feb 9, 2024 •

edited

Loading