Added script to gather command info and updated base_routing #143

aaron-congo · 2024-04-29T17:15:07Z

Issue #, if available:
N/A

Description of changes:

Adds a script that analyzes json info from the valkey commands directory and categorizes commands based on their RouteBy mapping
Updates cluster_routing.rs#base_routing with the info obtained from the script
- Includes adding a RouteBy::SecondArgAfterKeyCount mapping
Changes some incorrect mappings based on the script output
- WAITAOF was mapped to Random but is now mapped to AllPrimaries
- BITOP was mapped to Undefined but is now mapped to SecondArg
- MOVE was mapped to Undefined but is now mapped to FirstKey

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 · 2024-04-30T04:19:55Z

redis/src/cluster_routing.rs

+        | b"XINFO STREAM"
+        | b"ZDIFF"
+        | b"ZINTER"
+        | b"ZINTERCARD"
+        | b"ZMPOP"


@aaron-congo @Yury-Fridlyand
Are you intending to add all the missing commands that have 2nd arg as a key or some specific commands?

Lets make it exhaustive (if you already touching this code) - please implement a script that parses all the jsons in $REDIS_ROOT/src/commands/ and filters the commands that have "key" object at index 1 of "arguments" block, per example of ZDIFF:
"arguments": [ { "name": "numkeys", "type": "integer" }, { "name": "key", "type": "key", "key_spec_index": 0, "multiple": true }, { "name": "withscores", "token": "WITHSCORES", "type": "pure-token", "optional": true } ]
Run this script on the ValKey (https://github.com/valkey-io/valkey/releases/tag/7.2.5) source, so not tot touch the licensed Redis.

Please add this script to the PR - it will form the basis for automatically generate all these hard-coded cases during the compilation in the future

I've added SINTERCARD to SecondArgAfterKeyCount and BZMPOP to ThirdArgAfterKeyCount

ikolomi · 2024-04-30T06:25:01Z

@aaron-congo when you open PRs, please try to provide some info on the change in the PR description (not automatically ignoring it). For this instance for example - why this PR is needed?

shohamazon · 2024-04-30T06:41:57Z

redis/src/cluster_routing.rs

+        | b"ZINTER"
+        | b"ZINTERCARD"
+        | b"ZMPOP"
+        | b"ZUNION" => RouteBy::SecondArg,


Suggested change

| b"ZUNION" => RouteBy::SecondArg,

| b"LMPOP"

| b"BLMPOP"

| b"ZUNION" => RouteBy::SecondArg,

and also Yurri's additions
please check if FCALL should be there too

I added the commands Yury requested, LMPOP and ZUNION are now routed to SecondArgAfterKeyCount and BLMPOP is mapped to ThirdArgAfterKeyCount. FCALL is under the Uncategorized section of the script since the key argument is optional. For now I've left it out of the cluster_routing.rs file

From the discussion below I've realized cluster_routing can handle optional keys if there is a numkeys argument before the optional key, so I've updated the script to categorize these cases and moved FCALL to ThirdArgAfterKeyCount

ikolomi · 2024-04-30T07:13:01Z

redis/src/cluster_routing.rs

+        | b"XINFO STREAM"
+        | b"ZDIFF"
+        | b"ZINTER"
+        | b"ZINTERCARD"
+        | b"ZMPOP"


@aaron-congo @Yury-Fridlyand
Are you intending to add all the missing commands that have 2nd arg as a key or some specific commands?

Lets make it exhaustive (if you already touching this code) - please implement a script that parses all the jsons in $REDIS_ROOT/src/commands/ and filters the commands that have "key" object at index 1 of "arguments" block, per example of ZDIFF:
"arguments": [ { "name": "numkeys", "type": "integer" }, { "name": "key", "type": "key", "key_spec_index": 0, "multiple": true }, { "name": "withscores", "token": "WITHSCORES", "type": "pure-token", "optional": true } ]
Run this script on the ValKey (https://github.com/valkey-io/valkey/releases/tag/7.2.5) source, so not tot touch the licensed Redis.

Please add this script to the PR - it will form the basis for automatically generate all these hard-coded cases during the compilation in the future

Yury-Fridlyand · 2024-04-30T15:41:18Z

@ikolomi I like your idea, but keep in mind that these commands already merged on GLIDE repo and doesn't work properly in transaction. Do we have enough time before GLIDE release to implement that?

Yury-Fridlyand · 2024-04-30T16:25:30Z

As I see that, a script (aka build script) should parse valkey specs and generate a piece of rust code on compilation stage. Does this meet your expectations @ikolomi?

aaron-congo · 2024-04-30T16:31:26Z

As I see that, a script (aka build script) should parse valkey specs and generate a piece of rust code on compilation stage. Does this meet your expectations @ikolomi?

Or is the script supposed to just print out the commands that have "key" in position 2 so that they can be manually added from the script output to the file here?

ikolomi · 2024-05-01T06:46:52Z

@aaron-congo @Yury-Fridlyand
I meant to just make the script to detect such commands, not integrate into the build process - what i am lacking in this PR is the understanding of the process - did you manually checked the selected commands?
So like i said, if we already touching this part, lets have a automatic detection process, such script should not be difficult to implement - just json parsing

scripts/get_command_info.py

ikolomi · 2024-05-06T13:32:30Z

scripts/get_command_info.py

+import os
+from os.path import join
+
+"""Valkey command categorizer


This is a very good work!

I am a bit confused - Dont we want to align the RouteBy::SecondArg case with its output? If I understand correctly the SecondArg commands should be the following 2 sets:

============================ Category: SecondArg commands Description: Commands with their first key argument at position 2 List of commands in this category: BITOP MEMORY USAGE OBJECT ENCODING OBJECT FREQ OBJECT IDLETIME OBJECT REFCOUNT PFDEBUG XGROUP CREATE XGROUP CREATECONSUMER XGROUP DELCONSUMER XGROUP DESTROY XGROUP SETID XINFO CONSUMERS XINFO GROUPS XINFO STREAM ============================ Category: SecondArgAfterKeyCount commands Description: Commands with their first key argument at position 2, after a numkeys argument List of commands in this category: LMPOP SINTERCARD ZDIFF ZINTER ZINTERCARD ZMPOP ZUNION

So per the script's output - both @Yury-Fridlyand and @shohamazon suggestions on FCALL and BZMPOP are not SecondArg?

In addition, please leverage your awesome script to complete the list - it seems that some commands are missing (e.g. MEMORY USAGE)...

Please merge when done

@ikolomi

FCALL is being categorized by the script into ThirdArgAfterKeyCount, the signature is
FCALL function numkeys [key [key ...]] [arg [arg ...]]. The key arg is optional, so if a key is provided it will be at position 3. Is ThirdArgAfterKeyCount still the correct category even though the key arg is optional? If not, what category should it be?

BZMPOP is being categorized by the script into ThirdArgAfterKeyCount, I think this is correct as its signature is BZMPOP timeout numkeys key [key ...] <MIN | MAX> [COUNT count]

MEMORY USAGE is in the SecondArgs category, you can see it in the output snippet you posted above. I just checked and the output number of commands matches the number of json files

There's just a couple things that I still need to do to complete this PR:

I'm not sure how to distinguish between commands that are mapped to RouteBy::Random vs RouteBy::Undefined based on their json. For example, AUTH is mapped to Random, how should the script determine this based on its json info (here)?

Once I add logic to map to the random category after resolving the above question, I'll have to add the outputted commands to their categories in cluster_routing.rs

For FCALL we need another category, I think. In scope of current PR we can leave a TODO note only.
@barshaul what do you think?

Bar answered my question about the random category on slack, she said to leave them in the uncategorized group since there isn't a good way to classify them based on their json. I've added logic to put the commands with optional key arguments in the Uncategorized section of the output. @ikolomi I've also added the commands to cluster_routing.rs, so this PR should be good to merge once it has approvals.

@aaron-congo @Yury-Fridlyand re. FCALL - we already have a fallback if key count == 0, meaning that if no keys are passed, we will route the request to a random node. So please add FCALL to be mapped to ThirdArgAfterKeyCount

# cluster_routing.rs RouteBy::ThirdArgAfterKeyCount => { let key_count = r .arg_idx(2) .and_then(|x| std::str::from_utf8(x).ok()) .and_then(|x| x.parse::<u64>().ok())?; if key_count == 0 { Some(RoutingInfo::SingleNode(SingleNodeRoutingInfo::Random)) } else { r.arg_idx(3).map(|key| RoutingInfo::for_key(cmd, key)) } }

since cluster_routing can handle optional keys if there is a numkeys argument before the optional key, I've updated the script to categorize these cases and have updated FCALL to map to ThirdArgAfterKeyCount, this PR should be good to merge now once it has approvals

Merge when #143 (comment) is completed

…ts as Uncategorized

…RouteBy values

barshaul · 2024-05-09T08:01:46Z

Hey @aaron-congo, please fix the tests

…r in this category

acarbonetto · 2024-05-09T17:24:26Z

@barshaul @shohamazon would you be able to re-run the workflow?

shohamazon · 2024-05-09T17:25:53Z

@barshaul @shohamazon would you be able to re-run the workflow?

ofc

Yury-Fridlyand reviewed Apr 30, 2024

View reviewed changes

shohamazon reviewed Apr 30, 2024

View reviewed changes

shohamazon approved these changes Apr 30, 2024

View reviewed changes

ikolomi previously requested changes Apr 30, 2024

View reviewed changes

aaron-congo commented May 3, 2024

View reviewed changes

scripts/get_command_info.py Outdated Show resolved Hide resolved

Yury-Fridlyand reviewed May 3, 2024

View reviewed changes

scripts/get_command_info.py Show resolved Hide resolved

ikolomi reviewed May 6, 2024

View reviewed changes

aaron-congo added 2 commits May 6, 2024 09:58

Add script to categorize commands into their RouteBy categories

2c3f721

Remove copyright note

476b443

aaron-congo force-pushed the main branch from 890a5b2 to 476b443 Compare May 6, 2024 16:59

aaron-congo added 2 commits May 7, 2024 13:44

Change logic to categorize commands with optional key or slot argumen…

be0b2b5

…ts as Uncategorized

Update cluster_routing.rs with missing commands

7e375d7

aaron-congo changed the title ~~Add base_routing for sorted set commands~~ Added script to gather command info and updated base_routing May 7, 2024

Yury-Fridlyand approved these changes May 7, 2024

View reviewed changes

Yury-Fridlyand mentioned this pull request May 8, 2024

Java: Update transaction handling and IT valkey-io/valkey-glide#1284

Merged

aaron-congo added 2 commits May 8, 2024 13:03

optional keys are only categorized if they follow a numkeys arg

0b4caab

Update base_routing with some commands that were mapped to incorrect …

335cad2

…RouteBy values

barshaul approved these changes May 9, 2024

View reviewed changes

Remove MOVE and BITOP from test for Undefined since they are no longe…

15c3679

…r in this category

shohamazon merged commit e28a978 into amazon-contributing:main May 9, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added script to gather command info and updated base_routing #143

Added script to gather command info and updated base_routing #143

aaron-congo commented Apr 29, 2024 •

edited

Loading

Yury-Fridlyand Apr 30, 2024

ikolomi Apr 30, 2024

aaron-congo May 7, 2024

ikolomi commented Apr 30, 2024

shohamazon Apr 30, 2024

aaron-congo May 7, 2024

aaron-congo May 8, 2024

ikolomi Apr 30, 2024

Yury-Fridlyand commented Apr 30, 2024

Yury-Fridlyand commented Apr 30, 2024

aaron-congo commented Apr 30, 2024

ikolomi commented May 1, 2024

ikolomi May 6, 2024

aaron-congo May 6, 2024 •

edited

Loading

Yury-Fridlyand May 6, 2024

aaron-congo May 7, 2024 •

edited

Loading

barshaul May 8, 2024 •

edited

Loading

aaron-congo May 8, 2024

barshaul commented May 9, 2024

acarbonetto commented May 9, 2024

shohamazon commented May 9, 2024

Added script to gather command info and updated base_routing #143

Added script to gather command info and updated base_routing #143

Conversation

aaron-congo commented Apr 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikolomi commented Apr 30, 2024

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 commented Apr 30, 2024

Yury-Fridlyand commented Apr 30, 2024

aaron-congo commented Apr 30, 2024

ikolomi commented May 1, 2024

Choose a reason for hiding this comment

aaron-congo May 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaron-congo May 7, 2024 • edited Loading

Choose a reason for hiding this comment

barshaul May 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barshaul commented May 9, 2024

acarbonetto commented May 9, 2024

shohamazon commented May 9, 2024

aaron-congo commented Apr 29, 2024 •

edited

Loading

aaron-congo May 6, 2024 •

edited

Loading

aaron-congo May 7, 2024 •

edited

Loading

barshaul May 8, 2024 •

edited

Loading