Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Java: Add INFO() commands to java client #920

Merged
merged 7 commits into from
Feb 12, 2024

Conversation

acarbonetto
Copy link
Collaborator

@acarbonetto acarbonetto commented Feb 8, 2024

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
    • info
    • info with sections
  • cluster
    • info
    • info with routing
    • info with sections
    • info with sections & routing

note: info() command returns different sections in Redis version 6 vs version 7. IT tests have added the Redis version system variable to help test results expect version-specific results.

Examples:

// Single commands
RedisClient client = RedisClient.createClient(config).get();
String infoResult = client.info().get();
String infoResultWithOptions = client.info(InfoOptions.builder().section(EVERYTHING).build()).get();

// Cluster-mode single commands
RedisClusterClient clusterClient = RedisClusterClient.createClient(config).get();
String infoResult = clusterClient.info(ALL_NODES).get();
String infoResultWithOptions = client.info(InfoOptions.builder().section(EVERYTHING).build(), 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/integTest/build.gradle Outdated Show resolved Hide resolved
@acarbonetto acarbonetto self-assigned this Feb 8, 2024
@acarbonetto acarbonetto added the java issues and fixes related to the java client label Feb 8, 2024
@acarbonetto acarbonetto changed the title Java: Add info() commands to java client Java: Add INFO() commands to java client Feb 8, 2024
@acarbonetto acarbonetto force-pushed the java/integ_acarbo_add_info_only branch from 82d8d78 to 8380ba4 Compare February 8, 2024 21:26
@acarbonetto acarbonetto force-pushed the java/integ_acarbo_add_info_only branch from ac70a01 to 9c78c96 Compare February 9, 2024 07:06
Signed-off-by: Andrew Carbonetto <[email protected]>
Signed-off-by: Andrew Carbonetto <[email protected]>
Signed-off-by: Andrew Carbonetto <[email protected]>
Signed-off-by: Andrew Carbonetto <[email protected]>
Signed-off-by: Andrew Carbonetto <[email protected]>
@acarbonetto acarbonetto force-pushed the java/integ_acarbo_add_info_only branch from 9c78c96 to ce22c12 Compare February 11, 2024 08:36
*
* @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 with a <code>String</code> containing the information for
Copy link
Collaborator

@barshaul barshaul Feb 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without routing the info request is routed to multi-nodes (all primaries), so the return value is a map of strings. should be documented

Copy link
Collaborator

@barshaul barshaul Feb 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add an example, so it would be clearer that getMulti needs to be called in these functions by default

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this:

    /**
     * Get information and statistics about the Redis server. DEFAULT option is assumed. The command
     * will be routed to all primaries.
     *
     * @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 with a <code>Map{@literal <String, String>}</code> with each address
     *     as the key and its corresponding value is the information for the node.
     * @example
     *     <p><code>
     *     {@literal Map<String, String>} routedInfoResult = clusterClient.info().get().getMultiValue();
     *     </code>
     */

*
* @see <a href="https://redis.io/commands/info/">redis.io</a> for details.
* @param options - A list of {@link InfoOptions.Section} values specifying which sections of
* information to retrieve. When no parameter is provided, the {@link
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be mentioned that it's being sent to all primaries and that the return type is a map

3) "92d73b6eb847604b63c7f7cbbf39b148acdd1318"
4) (empty array)
*/
// Extracting first slot key
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[0])[2])[2] is the node ID == "92d73b6eb847604b63c7f7cbbf39b148acdd1318", why do you need it? why do you need to run cluster slots at all?

If you want to have a routing of a primary you can simply do new SlotKeyRoute("foo", PRIMARY);, we're covering all slots when we create the cluster so it would simply send the request to the primary that holds the slot of crc16("foo")

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just testing that we can do this. It's maybe not the most efficient command.

Copy link
Collaborator

@barshaul barshaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, please fix the comments

@Override
public CompletableFuture<ClusterValue<String>> info() {
return commandManager.submitNewCommand(
Info, new String[0], response -> ClusterValue.of(handleObjectOrNullResponse(response)));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is null accepted here?

Suggested change
Info, new String[0], response -> ClusterValue.of(handleObjectOrNullResponse(response)));
Info, new String[0], response -> ClusterValue.of(handleObjectResponse(response)));

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose that makes sense.

Signed-off-by: Andrew Carbonetto <[email protected]>
@acarbonetto acarbonetto merged commit e6a0de5 into valkey-io:main Feb 12, 2024
11 checks passed
@acarbonetto acarbonetto deleted the java/integ_acarbo_add_info_only branch February 23, 2024 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
java issues and fixes related to the java client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants