Java: Add PING command #914

acarbonetto · 2024-02-07T17:43:07Z

Issue #, if available:

This PR is dependent to: #917

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

standalone
- ping
- ping with message
cluster
- ping
- ping with message
- ping with route
- ping with message and route

Examples:

// Single commands
RedisClient client = RedisClient.createClient(config).get();
String pingResult = client.ping().get();
String pingResult2 = client.ping("message").get();

// Cluster-mode single commands
RedisClusterClient clusterClient = RedisClusterClient.createClient(config).get();
String pingResult3 = clusterClient.ping(ALL_NODES).get();
String pingResult4 = clusterClient.ping("message", ALL_NODES).get();

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

Yury-Fridlyand · 2024-02-07T18:25:08Z

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

+        throw new RedisException(
+                "Unexpected return type from Redis: got " + className + " expected Map");
+    }
+


Maybe we should make sections/regions - for handlers, for commands for all rest

Sure - but let's leave this for a future update. This will be useful when we have more than 3 commands.

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

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

SanHalacogluImproving · 2024-02-07T22:08:11Z

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

+    }
+
+    @Override
+    public CompletableFuture<String> ping(String str) {


Saw that you added @nonnull annotations for cluster why not here?

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

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

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

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

shachlanAmazon · 2024-02-08T20:15:29Z

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

+     * @return Response as a <code>String</code>
+     * @throws RedisException if there's a type mismatch
+     */
+    protected String handleStringResponse(Response response) {


not necessarily for this PR - this has a very similar structure to the handleArrayResponse.
Consider making a generic function with some refinement of this imaginary code:

protected T handleResponse<T>(Response response) { Object value = handleObjectResponse(response); if (value instanceof T || value == null) { return (T) value; } throw new RedisException( "Unexpected return type from Redis: got " + value.getClass().getSimpleName() + " expected " + T.getType()); } ``` of course, `T.getType()` isn't really possible and this doesn't handle primitives well, but I believe this is possible, and will remove a lot of glue code.

good idea. I've updated to include the generic function - this will reduce code duplication.

shachlanAmazon · 2024-02-08T20:16:27Z

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

        return CreateClient(config, RedisClient::new);
    }

    @Override
-    public CompletableFuture<Object> customCommand(String[] args) {
+    public CompletableFuture<Object> customCommand(@NonNull String[] args) {


correct me if I'm wrong, but @NonNull applies here to the array - the strings themselves might still be null?

Correct... and that would cause a protobuf builder error (NullPointerException).

so how would you solve this?

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

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

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

acarbonetto · 2024-02-08T20:24:01Z

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

@@ -25,12 +26,13 @@ protected RedisClient(ConnectionManager connectionManager, CommandManager comman
     * @param config Redis client Configuration
     * @return A Future to connect and return a RedisClient
     */
-    public static CompletableFuture<RedisClient> CreateClient(RedisClientConfiguration config) {
+    public static CompletableFuture<RedisClient> CreateClient(
+            @NonNull RedisClientConfiguration config) {


Adding for consistency across all methods. @nonnull will return a NullPointerException if the argument is not defined.

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

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

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

Yury-Fridlyand · 2024-02-08T23:26:12Z

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

     */
+    private <T> T handleRedisResponse(Class classType, boolean isNullable, Response response) {


Suggested change

private <T> T handleRedisResponse(Class classType, boolean isNullable, Response response) {

protected <T> T handleRedisResponse(Class<T> classType, boolean isNullable, Response response) throws RedisException {

Thanks @shachlanAmazon for the idea

why not private?

updated in 2cbf558

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

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

acarbonetto requested a review from a team as a code owner February 7, 2024 17:43

Yury-Fridlyand reviewed Feb 7, 2024

View reviewed changes

Yury-Fridlyand approved these changes Feb 7, 2024

View reviewed changes

SanHalacogluImproving reviewed Feb 7, 2024

View reviewed changes

Yury-Fridlyand approved these changes Feb 8, 2024

View reviewed changes

java/integTest/src/test/java/glide/SharedCommandTests.java Show resolved Hide resolved

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 added 9 commits February 8, 2024 11:08

Java: Add PING command

332d782

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

Remove unnecessary handlers

02b56fa

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

Remove OK string

99b36e0

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

Clean up import

2f7760d

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

Spotless

57a5115

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

Add @nonnull annotation

40c9bf3

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

Ping documentation update

c302263

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

Add sharedtests to IT

f78cdb6

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

Fix IT

0b2aa76

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

acarbonetto force-pushed the java/integ_acarbo_add_ping_only branch from 177857c to 0b2aa76 Compare February 8, 2024 19:10

Yury-Fridlyand approved these changes Feb 8, 2024

View reviewed changes

shachlanAmazon approved these changes Feb 8, 2024

View reviewed changes

acarbonetto commented Feb 8, 2024

View reviewed changes

Use genertic response handler

1b62d43

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

Yury-Fridlyand reviewed Feb 8, 2024

View reviewed changes

acarbonetto added 4 commits February 8, 2024 21:30

Update handleRedisResponse

2cbf558

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

Use @timeout annotation

3b968f0

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

Remove unnecessary nonnull

e68c8dd

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

Allow for null response in customCommand

df5d92e

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

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

acarbonetto deleted the java/integ_acarbo_add_ping_only branch February 9, 2024 06:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Java: Add PING command #914

Java: Add PING command #914

acarbonetto commented Feb 7, 2024 •

edited

Loading

Yury-Fridlyand Feb 7, 2024

acarbonetto Feb 7, 2024 •

edited

Loading

SanHalacogluImproving Feb 7, 2024

shachlanAmazon Feb 8, 2024

acarbonetto Feb 9, 2024

shachlanAmazon Feb 8, 2024

acarbonetto Feb 8, 2024

shachlanAmazon Feb 9, 2024

acarbonetto Feb 8, 2024

Yury-Fridlyand Feb 8, 2024

Yury-Fridlyand Feb 8, 2024

acarbonetto Feb 8, 2024

acarbonetto Feb 9, 2024

		*/
		private <T> T handleRedisResponse(Class classType, boolean isNullable, Response response) {

	private <T> T handleRedisResponse(Class classType, boolean isNullable, Response response) {
	protected <T> T handleRedisResponse(Class<T> classType, boolean isNullable, Response response) throws RedisException {

Java: Add PING command #914

Java: Add PING command #914

Conversation

acarbonetto commented Feb 7, 2024 • edited Loading

Examples:

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

acarbonetto commented Feb 7, 2024 •

edited

Loading

acarbonetto Feb 7, 2024 •

edited

Loading