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

acarbonetto · 2024-02-02T04:57:00Z

Issue #, if available:

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

standalone
- get
- set
- set with options
- ping
- ping with message
- info
- info with sections
cluster
- get
- set
- set with options
- ping
- ping with message
- info
- info with routing
- info with sections
- info with sections and routing

Examples:

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

// Cluster-mode single commands
RedisClusterClient clusterClient = RedisClusterClient.createClient(config).get();
ClusterValue<String> infoResult = clusterClient.info(ALL_NODES).get();
String infoForNode = infoResult.getMultiValue().get("<node address>");

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

Looking forward for IT

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

java/client/src/main/java/glide/api/models/configuration/RequestRoutingConfiguration.java

java/client/src/test/java/glide/api/RedisClientTest.java

java/client/src/test/java/glide/api/RedisClusterClientTest.java

java/client/src/test/java/glide/managers/CommandManagerTest.java

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

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

jduo · 2024-02-02T20:36:44Z

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

+            return Ok.INSTANCE;
+        }
+        String className = (value == null) ? "null" : value.getClass().getSimpleName();
+        throw new RedisException(


If this was non-null, should this log the actual value's toString() rather than just the class name?

Better not to expose user data to our logging?

jduo · 2024-02-02T23:07:31Z

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

+ */
+public class Ok {
+
+    public static Ok INSTANCE = new Ok();


This should be static final, not just static.

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

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

barshaul · 2024-02-04T09:52:31Z

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

+ *
+ * @see <a href="https://redis.io/commands/?group=server">Server Management Commands</a>
+ */
+public interface ClusterServerCommands {


I think ClusterManagementCommands describes the commands type better

ServerManagementClusterCommands or ServerClusterCommands.
We are going to have about 30 (15 for standalone and 15 for cluster mode) command interfaces to define all the types of commands. We can call them all ___Commands and ___ClusterCommands.
e.g. StringCommands and StringClusterCommands.

barshaul · 2024-02-04T09:54:42Z

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

+import java.util.concurrent.CompletableFuture;
+
+/**
+ * Server Management Commands interface.


Why is it an interface? Why the commands aren't being implemented here? I can't think of two cluster clients that will implement it in a different way.
Also, The 'commands' folder name should be renamed to async commands, as these APIs won't serve sync clients

The big win is because we don't need to define javadocs in multiple files.

Another big win is that for non-routed calls that never take a route (like get, set) we can define the signature in an interface once, and all implementations afterwards must abide by the signature.

Finally, it also allows us to spread the javadocs over multiple files split by "type", which makes organizing and maintaining the calls easier.

barshaul · 2024-02-04T09:58:57Z

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

+     *
+     * @see <a href="https://redis.io/commands/info/">redis.io</a> for details. {@link
+     *     InfoOptions.Section#DEFAULT} option is assumed.
+     * @return Response from Redis cluster containing a <code>String</code>.


Please use the documentation from python/node. Since in Java we use overloading, you'll need to adjust the documentation for each command function.
From python:

"""Get information and statistics about the Redis server. See https://redis.io/commands/info/ for details. Args: sections (Optional[List[InfoSection]]): A list of InfoSection values specifying which sections of information to retrieve. When no parameter is provided, the default option is assumed. route (Optional[Route]): The command will be routed to all primaries, unless `route` is provided, in which case the client will route the command to the nodes defined by `route`. Defaults to None. Returns: TClusterResponse[str]: If a single node route is requested, returns a string containing the information for the required sections. Otherwise, returns a dict of strings, with each key containing the address of the queried node and value containing the information regarding the requested sections. """

so, for info() with no args in cluster mode usage, the command will always be routed to all primaries and the response will always be a dict of strings. Same for info(InfoOptions options). However, when route can be provided we need to use the ClusterValue type as the response is dependent on the user input (single node routing vs multi node). The API signature and documentation shall reflect all of that. Please rewrite the documentation and the signature of all functions

Updated all commands in 8388c49

barshaul · 2024-02-04T10:20:50Z

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

+     * Ping the Redis server.
+     *
+     * @see <a href="https://redis.io/commands/ping/">redis.io</a> for details.
+     * @return Response from Redis containing a <code>String</code>.


same - please follow the functions' documentation we have in python/node throughout the PR

Updated all commands in 8388c49

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

barshaul · 2024-02-04T11:00:58Z

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

+     * @param key The key to store.
+     * @param value The value to store with the given key.
+     * @param options The Set options.
+     * @return Response from Redis containing a <code>String</code> or <code>null</code> response. The


"Response from Redis containing a String or null response." I think it doesn't describe it well, it doesn't return a response that contains a string - it returns a string.

Again, follow the doc from python/node:

If the value is successfully set, return OK. If value isn't set because of only_if_exists or only_if_does_not_exist conditions, return None. If return_old_value is set, return the old value as a string.

"Response from Redis" is another way of saying that its wrapped by a CompletableFuture.
The rest of the comment applies and we will reorder to match the node/python as much as applicable.

barshaul · 2024-02-04T11:07:58Z

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

@@ -19,7 +26,7 @@ private ClusterValue() {}

    /**
     * Get per-node value.<br>
-     * Check with {@link #hasMultiData()} prior to accessing the data.
+     * Asserts if {@link #hasMultiData()} is false.


So it would raise an assertion error? I would expect it to throw some client error, the user shouldn't know he needs to also expect assertion errors in addition to the client errors

It raises a RuntimeException. The user should always check hasMultiData() first to avoid the error.

barshaul · 2024-02-04T12:27:28Z

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

+
+    /** Configuration of value lifetime. */
+    @Builder
+    public static final class TimeToLive {


Please keep the names we use in python/node (Expiry) - we want to maintain uniform naming conventions across the languages

Changed field name to expiry of type Expiry:

private final Expiry expiry;

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

barshaul · 2024-02-04T12:29:02Z

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

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


minor: count => value

I think it should be Long, as users will probably use Duration objects that will be converted to long values (e.g. https://docs.oracle.com/javase/8/docs/api/java/time/Duration.html toSeconds, toMillis)

I'd prefer to keep count - consistent with Node. I think count is more meaningful, rather than value which is quite ambiguous.

barshaul · 2024-02-04T12:35:40Z

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

+    /** Types of value expiration configuration. */
+    @RequiredArgsConstructor
+    @Getter
+    public enum TimeToLiveType {


TimeToLiveType => ExpiryType

Updated to ExpiryType

shachlanAmazon · 2024-02-04T08:52:09Z

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

- * union-like type which can store single-value or multi-value retrieved from Redis. The
- * multi-value, if defined, contains the routed value as a Map<String, Object> containing a cluster
- * node address to cluster node value.
+ * Represents a Response object from a Redis server with cluster-mode enabled. The response type may


this isn't a response, and certainly not a capital-R Response - this is the returned value. A response might be an error, which AFAIS isn't represented here.

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

shachlanAmazon · 2024-02-04T09:37:59Z

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

- * union-like type which can store single-value or multi-value retrieved from Redis. The
- * multi-value, if defined, contains the routed value as a Map<String, Object> containing a cluster
- * node address to cluster node value.
+ * Represents a Response object from a Redis server with cluster-mode enabled. The response type may


how are the changes here related to this PR? can they be moved to another PR?

They could be split into a separate PR, actually.

Reverted. Will raise a new PR with these changes.

shachlanAmazon · 2024-02-04T11:16:55Z

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

+    }
+
+    /** Configuration of value lifetime. */
+    @Builder


the builder pattern doesn't suit this - only 2 values, and both must be set.

This enforces a user to set count (aka value) even when expiration type is keepttl. Could be confusing.

not if you're using static factories -

static TimeToLive keep_existing() static TimeToLive seconds(Duration seconds) static TimeToLive milliseconds(Duration milliseconds)

etc.

fixed in d082f32

Builder pattern for SetOptions now looks like:

SetOptions options = SetOptions.builder().expiry(UnixSeconds(100500L)).build(); SetOptions options = SetOptions.builder().expiry(Milliseconds(2000L)).build(); SetOptions options = SetOptions.builder().returnOldValue(true).conditionalSet(ONLY_IF_DOES_NOT_EXIST).expiry(KeepExisting()).build();

shachlanAmazon · 2024-02-04T11:25:24Z

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

+    public <T> CompletableFuture<T> submitNewCommand(
+            RequestType requestType,
+            String[] arguments,
+            Optional<Route> route,


minor: it's weird that route is optional in 2 senses - it's both an optional parameter due to functional overloading, and also a value of type Optional. Why would this function be called with Optional.empty(), instead of just calling the other overload?

The reasoning is that one call is used for the standalone client, while the other method is used for the cluster client.
In the end, we merge into a single call with an Optional route to save on duplicating lines of code, but you are right, we could have implemented it the other way around.

shachlanAmazon · 2024-02-04T12:39:18Z

java/client/src/test/java/glide/api/RedisClientTest.java

+    @Test
+    public void ping_returns_success() {
+        // setup
+        CompletableFuture<String> testResponse = mock(CompletableFuture.class);


you need to test all the commands with a real server

given that you'll test all of the commands with a real server, what's the benefit of adding mock tests?

Main advantage is that it's faster/easier/cheaper to run unit tests and provides a good white-box testing platform. Using mocks allows us to test all variants independently (one injected dependency at a time).

If you don't want to maintain this long term, we could reduce/remove the unit tests, but I don't recommend that.

java/client/src/test/java/glide/api/RedisClusterClientTest.java

shachlanAmazon · 2024-02-04T12:44:57Z

java/client/src/test/java/glide/api/RedisClusterClientTest.java

        assertAll(
                () -> assertTrue(value.hasSingleData()), () -> assertEquals(data, value.getSingleValue()));
    }

    @Test
    @SneakyThrows
-    public void custom_command_with_multi_node_route_returns_multi_value() {
+    public void customCommand_with_multi_node_route_returns_multi_value() {


do these test renames belong in this PR? if not, please move them to a separate PR.

shachlanAmazon · 2024-02-04T12:45:16Z

java/client/src/test/java/glide/api/RedisClusterClientTest.java

        var commandManager = new TestCommandManager(null);

        var data = Map.of("key1", "value1", "key2", "value2");
        var client = new TestClient(commandManager, data);

-        var value = client.customCommand(new String[0], SimpleRoute.ALL_NODES).get();
+        var value = client.customCommand(ALL_NODES, "Test").get();


do these call changes belong in this PR? If not, please move them to another PR.

shachlanAmazon · 2024-02-04T12:46:26Z

java/client/src/test/java/glide/api/RedisClusterClientTest.java

+
+    @SneakyThrows
+    @Test
+    public void customCommand_success() {


what does this test? how is it different from customCommand_returns_success?

Removing this. We don't need both.

barshaul · 2024-02-04T12:56:27Z

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

+            if (expiry.type != TimeToLiveType.KEEP_EXISTING) {
+                if (expiry.count == null) {
+                    throw new RequestException(
+                            "Set command received expiry type " + expiry.type + ", but count was not set.");


but count was not set => but value was not set

I'd prefer to keep count, which is consistent with Node client.

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

barshaul · 2024-02-04T13:11:49Z

java/client/src/test/java/glide/ExceptionHandlingTests.java

@@ -84,7 +83,7 @@ public void channel_is_closed_when_disconnected_on_command() {
        var channelHandler = new TestChannelHandler(callbackDispatcher);
        var commandManager = new CommandManager(channelHandler);

-        var future = commandManager.submitNewCommand(createDummyCommand(), r -> null);
+        var future = commandManager.submitNewCommand(CustomCommand, new String[0], r -> null);
        callbackDispatcher.completeRequest(null);
        var exception = assertThrows(ExecutionException.class, future::get);
        // a ClosingException thrown from CallbackDispatcher::completeRequest and then


custom command with empty args list shouldn't close the client (e.g. raise ClosingException)

The ClosingException is just from a mock. It's not necessarily from the empty args list.
A closing exception may be returned from any command call, and then we close the client.

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

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

Signed-off-by: Yury-Fridlyand <[email protected]>

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

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

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

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

Yury-Fridlyand

Couple minor non-functional things

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

Yury-Fridlyand · 2024-02-07T02:06:12Z

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

+        if (expiry != null) {
+            optionArgs.add(expiry.type.redisApi);
+            if (expiry.type != KEEP_EXISTING) {
+                if (expiry.count == null) {


I think you can omit this check with new constructors

I was thinking the same thing, but does it hurt to keep it..?

Maybe turn into an assert?

It is ok to keep it

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

java/client/src/test/java/glide/api/RedisClusterClientTest.java

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

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

acarbonetto · 2024-02-08T05:03:50Z

Closing PR in favour of:

acarbonetto requested a review from a team as a code owner February 2, 2024 04:57

acarbonetto self-assigned this Feb 2, 2024

acarbonetto requested a review from Yury-Fridlyand February 2, 2024 04:57

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

Yury-Fridlyand reviewed Feb 2, 2024

View reviewed changes

SanHalacogluImproving reviewed Feb 2, 2024

View reviewed changes

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

Yury-Fridlyand reviewed Feb 2, 2024

View reviewed changes

jduo reviewed Feb 2, 2024

View reviewed changes

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

jduo reviewed Feb 2, 2024

View reviewed changes

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

jduo reviewed Feb 2, 2024

View reviewed changes

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

jduo reviewed Feb 2, 2024

View reviewed changes

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

Yury-Fridlyand force-pushed the java/dev_acarbo_add_getsetpinginfo branch from 6785dc6 to 26ee2c3 Compare February 3, 2024 00:58

Yury-Fridlyand mentioned this pull request Feb 3, 2024

Java: Add transaction commands #895

Merged

Yury-Fridlyand approved these changes Feb 3, 2024

View reviewed changes

Yury-Fridlyand changed the title ~~Java: Add commands for GET/SET/INFO/PING~~ Java: Add GET, SET, INFO and PING commands Feb 3, 2024

barshaul reviewed Feb 4, 2024

View reviewed changes

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

barshaul reviewed Feb 4, 2024

View reviewed changes

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

barshaul reviewed Feb 4, 2024

View reviewed changes

shachlanAmazon reviewed Feb 4, 2024

View reviewed changes

barshaul reviewed Feb 4, 2024

View reviewed changes

java/client/src/main/java/glide/managers/CommandManager.java Outdated Show resolved Hide resolved

barshaul reviewed Feb 4, 2024

View reviewed changes

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

acarbonetto and others added 12 commits February 6, 2024 08:56

Fix Redis versioned tests

6c8e04b

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

Remove redis 6 issue with LatencyStats

4262a4b

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

Remove redis 6 issue with LatencyStats

8c22212

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

Remove EVERYTHING info() test

e96c284

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

spotless

59e7b8f

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

Fix IT.

ee90545

Signed-off-by: Yury-Fridlyand <[email protected]>

cleanup

210094c

Signed-off-by: Yury-Fridlyand <[email protected]>

Minor fixes.

677c98c

Signed-off-by: Yury-Fridlyand <[email protected]>

Remove OK object

70cd993

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

Revert change to customCommand

2dd1a03

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

Rename command files

4a56401

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

Add ping(route) to Cluster client

42b0498

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

acarbonetto force-pushed the java/dev_acarbo_add_getsetpinginfo branch from ca28051 to 42b0498 Compare February 6, 2024 17:01

Yury-Fridlyand reviewed Feb 6, 2024

View reviewed changes

acarbonetto added 4 commits February 6, 2024 15:01

Clean up javadocs; and remove extra test

b780bfc

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

Fix CI run

40520a7

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

Revert documentation changes in ClusterValue

55a0097

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

Change SetOptions.expiry to use constructors

d082f32

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

Yury-Fridlyand approved these changes Feb 7, 2024

View reviewed changes

acarbonetto added 7 commits February 6, 2024 18:16

Update cluster mode javadocs

8388c49

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

Update javadocs to be consistent with Node client

a96658b

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

minor update

189badb

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

minor update

fcd13df

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

remove check in SetOptions

dc43db8

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

Update OK statis string

247ffdb

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

Clean upRedisClusterClientTest.java

cb8cac4

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

Yury-Fridlyand approved these changes Feb 7, 2024

View reviewed changes

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

acarbonetto closed this Feb 8, 2024

Yury-Fridlyand deleted the java/dev_acarbo_add_getsetpinginfo branch February 8, 2024 23:42

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

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

Conversation

acarbonetto commented Feb 2, 2024 • edited by Yury-Fridlyand Loading

Examples:

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barshaul Feb 4, 2024 • edited Loading

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barshaul Feb 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acarbonetto commented Feb 8, 2024

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

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

acarbonetto commented Feb 2, 2024 •

edited by Yury-Fridlyand

Loading

barshaul Feb 4, 2024 •

edited

Loading

acarbonetto Feb 7, 2024 •

edited

Loading

barshaul Feb 4, 2024 •

edited

Loading