Skip to content

sub-command support for ACL CAT and COMMAND LIST. redisCommand always stores fullname#10127

Merged
oranagra merged 29 commits intoredis:unstablefrom
enjoy-binbin:command_fullname
Jan 23, 2022
Merged

sub-command support for ACL CAT and COMMAND LIST. redisCommand always stores fullname#10127
oranagra merged 29 commits intoredis:unstablefrom
enjoy-binbin:command_fullname

Conversation

@enjoy-binbin
Copy link
Contributor

@enjoy-binbin enjoy-binbin commented Jan 17, 2022 •
edited by oranagra
Loading

Summary of changes:

  1. Rename redisCommand->name to redisCommand->declared_name, it is a const char * for native commands and SDS for module commands.
  2. Store the [sub]command fullname in redisCommand->fullname (sds).
  3. List subcommands in ACL CAT
  4. List subcommands in COMMAND LIST
  5. moduleUnregisterCommands now will also free the module subcommands.
  6. RM_GetCurrentCommandName returns full command name

Other changes:

  1. Add addReplyErrorArity and addReplyErrorExpireTime
  2. Remove getFullCommandName function that now is useless.
  3. Some cleanups about fullname since now it is SDS.
  4. Delete populateSingleCommand function from server.h that is useless.
  5. Added tests to cover this change.
  6. Add some module unload tests and fix the leaks
  7. Make error messages uniform, make sure they always contain the full command name and that it's quoted.
  8. Fixes some typos

see the history in #9504, fixes #10124

@enjoy-binbin enjoy-binbin marked this pull request as draft January 17, 2022 17:24
guybe7
guybe7 reviewed Jan 17, 2022
View reviewed changes
guybe7
guybe7 reviewed Jan 17, 2022
View reviewed changes
guybe7
guybe7 reviewed Jan 17, 2022
View reviewed changes
guybe7
guybe7 reviewed Jan 17, 2022
View reviewed changes
guybe7
guybe7 reviewed Jan 18, 2022
View reviewed changes
guybe7
guybe7 reviewed Jan 18, 2022
View reviewed changes
@enjoy-binbin enjoy-binbin marked this pull request as ready for review January 18, 2022 16:54
@enjoy-binbin enjoy-binbin changed the title wip: always store the fullname redisCommand always store the fullname Jan 18, 2022
@enjoy-binbin enjoy-binbin requested a review from guybe7 January 18, 2022 16:58
@enjoy-binbin enjoy-binbin mentioned this pull request Jan 19, 2022
Closed
sundb
sundb reviewed Jan 19, 2022
View reviewed changes
guybe7
guybe7 reviewed Jan 19, 2022
View reviewed changes
guybe7
guybe7 reviewed Jan 19, 2022
View reviewed changes
guybe7
guybe7 reviewed Jan 19, 2022
View reviewed changes
guybe7
guybe7 reviewed Jan 19, 2022
View reviewed changes
guybe7
guybe7 reviewed Jan 19, 2022
View reviewed changes
guybe7
guybe7 reviewed Jan 19, 2022
View reviewed changes
guybe7
guybe7 reviewed Jan 19, 2022
View reviewed changes
guybe7
guybe7 reviewed Jan 19, 2022
View reviewed changes
@oranagra
Copy link
Member

oranagra commented Jan 19, 2022

@redis/core-team listing sub-commands in ACL CAT and COMMAND LIST.
complementary act for the sub-command work in 7.0.
if you see any reason not to do that, please speak.

@guybe7
Copy link
Collaborator

guybe7 commented Jan 20, 2022

@enjoy-binbin i'm done reviewing the lat batch of changes

@enjoy-binbin
Copy link
Contributor Author

enjoy-binbin commented Jan 20, 2022

@guybe7 thank you very much for the review.. sorry for not doing it perfectly, i will take a deep look tomorrow

guybe7
guybe7 requested changes Jan 21, 2022
View reviewed changes
@enjoy-binbin
Copy link
Contributor Author

enjoy-binbin commented Jan 21, 2022 •
edited
Loading

trigger a full CI in this branch: https://github.com/enjoy-binbin/redis/actions/runs/1729975896

oranagra
oranagra reviewed Jan 22, 2022
View reviewed changes
@oranagra
Copy link
Member

oranagra commented Jan 22, 2022

@enjoy-binbin gave it another top to bottom review, and last round of comments.
there's one real issue here, one that existed before your PR, but i just found it now (iterating on the subcommands list instead of dict).
all other comments are just some small suggestions for improvements.
i also added some details to the top comment.
please also go over all the earlier unresolved comments make sure they're resolved and mark them as such.
then let's merge this.

@enjoy-binbin
Copy link
Contributor Author

enjoy-binbin commented Jan 22, 2022

@oranagra thanks for the review / patience. I checked all the comments.
I admit this PR getting bigger and bigger with each small change, with too much cleanup mixed in...
will avoid doing this in the future, or at least split the PR (avoid irrelevant changes)

@oranagra
Copy link
Member

oranagra commented Jan 22, 2022

a little unrelated cleanup is ok.. and in this case much of it is NOT unrelated.
in retrospect, i'm not sure we should have made all the error messages uniform (inducing many changes in the tests)

we could even consider not to rename name to fullname, but i figured it could be dangerous for some merge branches / forks, and once we did that, many of these lines where already modified anyway...

oranagra
oranagra reviewed Jan 22, 2022
View reviewed changes
yossigo
yossigo approved these changes Jan 23, 2022
View reviewed changes
Copy link
Collaborator

@yossigo yossigo left a comment

Choose a reason for hiding this comment

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

LGTM (based on top comment, did not CR).

@oranagra oranagra merged commit 23325c1 into redis:unstable Jan 23, 2022
@enjoy-binbin enjoy-binbin deleted the command_fullname branch January 23, 2022 08:07
@oranagra oranagra mentioned this pull request Jan 23, 2022
Merged
5 tasks
@oranagra oranagra mentioned this pull request Apr 24, 2022
Merged
oranagra pushed a commit that referenced this pull request Apr 25, 2022
@guybe7
Fix regression not aborting transaction on error, and re-edit some er…
df78776 
…ror responses (#10612)

1. Disk error and slave count checks didn't flag the transactions or counted correctly in command stats (regression from #10372  , 7.0 RC3)
2. RM_Call will reply the same way Redis does, in case of non-exisitng command or arity error
3. RM_WrongArtiy will consider the full command name
4. Use lowercase 'u' in "unknonw subcommand" (to align with "unknown command")

Followup work of #10127
enjoy-binbin pushed a commit to enjoy-binbin/redis that referenced this pull request Jul 31, 2023
@guybe7 @enjoy-binbin
Fix regression not aborting transaction on error, and re-edit some er…
bea3d70 
…ror responses (redis#10612)

1. Disk error and slave count checks didn't flag the transactions or counted correctly in command stats (regression from redis#10372  , 7.0 RC3)
2. RM_Call will reply the same way Redis does, in case of non-exisitng command or arity error
3. RM_WrongArtiy will consider the full command name
4. Use lowercase 'u' in "unknonw subcommand" (to align with "unknown command")

Followup work of redis#10127
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sundb sundb sundb left review comments

@yossigo yossigo yossigo approved these changes

@oranagra oranagra oranagra approved these changes

@guybe7 guybe7 guybe7 approved these changes

Assignees

No one assigned

Labels

approval-needed Waiting for core team approval to be merged release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus

Projects

Redis 7.0
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Iterating all the commands

5 participants

@enjoy-binbin @oranagra @guybe7 @sundb @yossigo