Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
oranagra merged 29 commits into from
Jan 23, 2022
Merged

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

oranagra merged 29 commits into from
Jan 23, 2022

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 and others added 2 commits January 22, 2022 23:24
@enjoy-binbin
Merge remote-tracking branch 'upstream/unstable' into command_fullname
2c2ee32
@enjoy-binbin @oranagra @guybe7
review from oran: iter subcommand dict and test cleanups
a8af348 
Co-authored-by: Oran Agra <oran@redislabs.com>
Co-authored-by: guybe7 <guy.benoish@redislabs.com>
@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
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