Java: Add `FUNCTION LIST` command. #1452

Yury-Fridlyand · 2024-05-23T19:53:52Z

Issue #, if available:
N/A

Description of changes:
https://redis.io/docs/latest/commands/function-list/

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Some highlights in this PR:

data conversion (value_conversion.rs)
new functions for standalone and cluster clients' interfaces (ScriptingAndFunctions*Commands.java)
implementations for both client types (RedisClient.java & RedisClusterClient.java)
response handling (BaseClient.java)
transaction update (BaseTransaction.java)
new options (not for user use, but to avoid client classes being polluted by them, moved to a another place - FunctionListOptions.java)
UT (java/client/src/test/...)
transaction IT (TransactionTestUtilities.java)
command IT (CommandTests.java)
IT tools (TestUtilities.java)

Some details on how response conversion works:

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

* Add `FUNCTION LIST` command. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * Tests for the God of Tests. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * I HATE YOU SPOTLESS Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * Ruuuuuuuuuuuuuuuuuust Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * Add `@since`. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * Update examples. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * Clean up value conversion code. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> * Address PR comments. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> --------- Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

glide-core/src/client/value_conversion.rs

aaron-congo · 2024-05-23T22:59:09Z

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

+
+    /**
+     * Returns information about the functions and libraries.<br>
+     * The command will be routed to all primary nodes.


Just wanted to double check, are we sure this is correct? Unlike FUNCTION FLUSH and FUNCTION LOAD, which have an ALL_SHARDS request policy (see here), FUNCTION LIST does not have any request policy (see here)

On a related note, unlike FUNCTION LOAD and FUNCTION FLUSH, this command is not in cluster_routing.rs, I'm assuming we'll have to add it?

Oh right, going to add it

amazon-contributing/redis-rs#148

please close this

Updated & closed.

I think we still need to update redis-rs#cluster_routing.rs to map this command to RANDOM, since the default is FirstKey

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

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

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> Co-authored-by: Aaron <69273634+aaron-congo@users.noreply.github.com>

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

glide-core/src/client/value_conversion.rs

acarbonetto · 2024-05-27T20:53:30Z

glide-core/src/client/value_conversion.rs

@@ -5,7 +5,7 @@ use redis::{
    cluster_routing::Routable, from_owned_redis_value, Cmd, ErrorKind, RedisResult, Value,
 };

-#[derive(Clone, Copy)]
+#[derive(Clone, Copy, Debug)]


It is required for debug logging. I decided to keep it.

acarbonetto · 2024-05-27T20:56:09Z

glide-core/src/client/value_conversion.rs

+           3) "engine"
+           4) "LUA"
+           5) "functions"
+           6) 1) 1) "name"


is this a set?

flags is a set

acarbonetto · 2024-05-27T21:02:20Z

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

+
+    /**
+     * Returns information about the functions and libraries.<br>
+     * The command will be routed to all primary nodes.


please close this

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

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

java/integTest/src/test/java/glide/TransactionTestUtilities.java

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

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

jonathanl-bq

Approved with some minor comments on the Rust side, though, I skimmed through the Java side briefly too.

glide-core/src/client/value_conversion.rs

jonathanl-bq · 2024-05-28T20:30:07Z

glide-core/src/client/value_conversion.rs

    }
 }

+/// Similar to [`convert_array_to_map`], but converts a map to a map.


Kind of confusingly worded. I think this is a little more clear:

Suggested change

/// Similar to [`convert_array_to_map`], but converts a map to a map.

/// Similar to [`convert_array_to_map`], but converts an array of value pairs to a map.

Input argument is always a map
https://github.com/amazon-contributing/redis-rs/blob/e28a97877fc353497b35a0cd033bf47cb199b47e/redis/src/types.rs#L325

okay, but saying "map to a map" sounds like you're just running the identity function (which you are not). What does this actually do?

It converts keys and values inside the map. So input is a map and output is a map too.

okay, I feel like the comment could describe it as such. Otherwise, the function looks like it just takes a map and returns it, based on the description. That being said, if you really want to, you can leave the comment as is. I don't think it'll matter much in the long run for such a minor documentation nit.

Updated, thank you.

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

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

aaron-congo · 2024-05-28T21:25:22Z

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

+
+    /**
+     * Returns information about the functions and libraries.<br>
+     * The command will be routed to all primary nodes.


I think we still need to update redis-rs#cluster_routing.rs to map this command to RANDOM, since the default is FirstKey

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

aaron-congo

LGTM but we will need to merge the change to redis-rs first

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

glide-core/src/client/value_conversion.rs

barshaul

This PR is too large to be properly reviewed, and there are some parts which can be easily moved to different PRs - for example the value conversion logic.
You can have PRs that are being rebased over others, but we need to have smaller PRs in order to make sure we're not missing anything.

valkey-io#1452. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

Yury-Fridlyand · 2024-05-29T17:26:45Z

This PR is too large to be properly reviewed, and there are some parts which can be easily moved to different PRs - for example the value conversion logic. You can have PRs that are being rebased over others, but we need to have smaller PRs in order to make sure we're not missing anything.

Please see #1489

Yury-Fridlyand · 2024-05-30T00:53:33Z

Suspended: waiting for #1489 and for amazon-contributing/redis-rs#148

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

Review addressed in #1489 which is already merged

* Add `FUNCTION LIST` command. (#277) Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> Co-authored-by: Aaron <69273634+aaron-congo@users.noreply.github.com>

Yury-Fridlyand added 4 commits May 9, 2024 20:13

Add FUNCTION LOAD command.

2f672a6

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

Fix IT on redis 6.

ce72b3b

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

Merge remote-tracking branch 'upstream/main' into java/integ_yuryf_flist

93c4a18

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

Yury-Fridlyand added the java issues and fixes related to the java client label May 23, 2024

Yury-Fridlyand requested review from aaron-congo, barshaul and shohamazon May 23, 2024 19:53

Yury-Fridlyand requested a review from a team as a code owner May 23, 2024 19:53

aaron-congo reviewed May 23, 2024

View reviewed changes

Apply suggestions from code review

f58cf63

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com> Co-authored-by: Aaron <69273634+aaron-congo@users.noreply.github.com>

Yury-Fridlyand commented May 27, 2024

View reviewed changes

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

acarbonetto reviewed May 27, 2024

View reviewed changes

Yury-Fridlyand commented May 27, 2024

View reviewed changes

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

Yury-Fridlyand added 5 commits May 27, 2024 22:11

Address PR comments.

3bd823e

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

Address PR comments.

b0f78cb

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

Merge remote-tracking branch 'upstream/main' into java/integ_yuryf_flist

90e1c2a

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

Fix rust code docs.

9af9d4f

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

Merge function signatures by adding withCode argument.

d248ff1

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

jonathanl-bq approved these changes May 28, 2024

View reviewed changes

aaron-congo reviewed May 28, 2024

View reviewed changes

Yury-Fridlyand added 2 commits May 28, 2024 14:41

Rework response conversion recursion.

3cc2446

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

Address PR comments.

fe7d2b7

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

jonathanl-bq approved these changes May 28, 2024

View reviewed changes

aaron-congo approved these changes May 28, 2024

View reviewed changes

Update rust docs.

a855672

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

barshaul reviewed May 29, 2024

View reviewed changes

glide-core/src/client/value_conversion.rs Outdated Show resolved Hide resolved

barshaul reviewed May 29, 2024

View reviewed changes

glide-core/src/client/value_conversion.rs Outdated Show resolved Hide resolved

barshaul previously requested changes May 29, 2024

View reviewed changes

Yury-Fridlyand added a commit to Bit-Quill/valkey-glide that referenced this pull request May 29, 2024

Update response conversion for FUNCTION LIST according to comments in

a0ba60e

valkey-io#1452. Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

Yury-Fridlyand mentioned this pull request May 29, 2024

Core: Add response conversion for FUNCTION LIST command #1489

Merged

Yury-Fridlyand marked this pull request as draft May 30, 2024 00:53

Yury-Fridlyand requested a review from barshaul June 1, 2024 02:19

Merge remote-tracking branch 'upstream/main' into java/integ_yuryf_flist

f9e0450

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

Yury-Fridlyand marked this pull request as ready for review June 5, 2024 02:10

Fix merge error.

1da0ca0

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

Yury-Fridlyand merged commit 200d68f into valkey-io:main Jun 5, 2024
46 checks passed

Yury-Fridlyand deleted the java/integ_yuryf_flist branch June 5, 2024 02:26

Cole-Greer mentioned this pull request Jul 1, 2024

Python: Add FUNCTION LIST command. #1738

Merged

Yury-Fridlyand mentioned this pull request Jul 25, 2024

Node: Add FUNCTION LIST #2019

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Java: Add `FUNCTION LIST` command. #1452

Java: Add `FUNCTION LIST` command. #1452

Yury-Fridlyand commented May 23, 2024 •

edited

Loading

aaron-congo May 23, 2024

Yury-Fridlyand May 23, 2024

Yury-Fridlyand May 23, 2024

acarbonetto May 27, 2024

Yury-Fridlyand May 28, 2024

aaron-congo May 28, 2024

Yury-Fridlyand May 28, 2024

Yury-Fridlyand May 28, 2024

acarbonetto May 27, 2024

Yury-Fridlyand May 27, 2024

acarbonetto May 27, 2024

Yury-Fridlyand May 27, 2024

acarbonetto May 27, 2024

jonathanl-bq left a comment

jonathanl-bq May 28, 2024

Yury-Fridlyand May 28, 2024

jonathanl-bq May 28, 2024

Yury-Fridlyand May 28, 2024

jonathanl-bq May 28, 2024

Yury-Fridlyand May 29, 2024

aaron-congo May 28, 2024

aaron-congo left a comment

barshaul left a comment •

edited

Loading

Yury-Fridlyand commented May 29, 2024

Yury-Fridlyand commented May 30, 2024

	/// Similar to [`convert_array_to_map`], but converts a map to a map.
	/// Similar to [`convert_array_to_map`], but converts an array of value pairs to a map.

Java: Add FUNCTION LIST command. #1452

Java: Add FUNCTION LIST command. #1452

Conversation

Yury-Fridlyand commented May 23, 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

jonathanl-bq 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

aaron-congo left a comment

Choose a reason for hiding this comment

barshaul left a comment • edited Loading

Choose a reason for hiding this comment

Yury-Fridlyand commented May 29, 2024

Yury-Fridlyand commented May 30, 2024

Java: Add `FUNCTION LIST` command. #1452

Java: Add `FUNCTION LIST` command. #1452

Yury-Fridlyand commented May 23, 2024 •

edited

Loading

barshaul left a comment •

edited

Loading