Skip to content

Command info module API #10108

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 31 commits into from
Feb 4, 2022
Merged

Command info module API #10108

oranagra merged 31 commits into from
Feb 4, 2022

Conversation

@zuiderkwast
Copy link
Contributor

@zuiderkwast zuiderkwast commented Jan 13, 2022 •
edited by oranagra
Loading

Adds RM_SetCommandInfo, allowing modules to provide the following command info:

  • summary
  • complexity
  • since
  • history
  • hints
  • arity
  • key specs
  • args

This information affects the output of COMMAND, COMMAND INFO and COMMAND DOCS, Cluster, ACL and is used to filter commands with the wrong number of arguments before the call reaches the module code.

The recently added API functions for key specs (never released) are removed.

This work is based on some commits in #9656 and then rewritten to the declarative style as discussed in #9944.

Fixes #9944.

A minimalist example would look like so:

    RedisModuleCommand *mycmd = RedisModule_GetCommand(ctx,"mymodule.mycommand");
    RedisModuleCommandInfo mycmd_info = {
        .version = REDISMODULE_COMMAND_INFO_VERSION,
        .arity = -5,
        .summary = "some description",
    };
    if (RedisModule_SetCommandInfo(mycmd, &mycmd_info) == REDISMODULE_ERR)
        return REDISMODULE_ERR;

Notes:

  • All the provided information (including strings) is copied, not keeping references to the API input data.
  • The version field is actually a static struct that contains the sizes of the the structs used in arrays, so we can extend these in the future and old version will still be able to take the part they can support.

zuiderkwast
zuiderkwast commented Jan 13, 2022
View reviewed changes
zuiderkwast
zuiderkwast commented Jan 13, 2022
View reviewed changes
zuiderkwast
zuiderkwast commented Jan 13, 2022
View reviewed changes
zuiderkwast
zuiderkwast commented Jan 13, 2022
View reviewed changes
guybe7
guybe7 reviewed Jan 17, 2022
View reviewed changes
@zuiderkwast zuiderkwast mentioned this pull request Jan 18, 2022
Merged
zuiderkwast added a commit to zuiderkwast/redis that referenced this pull request Jan 18, 2022
@zuiderkwast
Disable keyspec module API in 7.0 RC1
01c6977 
The keyspec API is not yet released and there is a plan to change it
in redis#10108, which is going to be included in RC2. Therefore, we hide
it in RC1 to avoid introducing a breaking change in RC2.
guybe7
guybe7 reviewed Jan 20, 2022
View reviewed changes
guybe7
guybe7 reviewed Jan 20, 2022
View reviewed changes
guybe7
guybe7 reviewed Jan 20, 2022
View reviewed changes
guybe7
guybe7 reviewed Jan 20, 2022
View reviewed changes
guybe7
guybe7 reviewed Jan 20, 2022
View reviewed changes
guybe7
guybe7 reviewed Jan 20, 2022
View reviewed changes
guybe7
guybe7 reviewed Jan 20, 2022
View reviewed changes
@oranagra
Copy link
Member

oranagra commented Feb 3, 2022

@redis/core-team please approve the new module API

@oranagra oranagra added 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 labels Feb 3, 2022
@zuiderkwast
Add more keyspecs tests
0a3a67a 
Two new tests:

1. A command without keyspecs. Check that the legacy triple remains and that a
   keyspec is autogenerated from the legacy triple.

2. A command with only a keyword-based keyspec. This spec cases the legacy
   triple to be overwritten with (0,0,0) and sets movablekeys.

In all tests, check movablekeys, which is overridden based on keyspecs.
@yossigo
Copy link
Collaborator

yossigo commented Feb 3, 2022

@zuiderkwast @oranagra I've pushed a private commit with an alternative here:
yossigo@13219d3

This seems a bit more straight forward to me. Callers need to pass an extra REDISMODULE_COMMANDINFO_VERSION argument to RM_SetCommandInfo(), but that is pretty much aligned with how other APIs work. The upside is we can avoid the inline function and name mangling.

@oranagra
Copy link
Member

oranagra commented Feb 3, 2022

it is slightly more verbose to the user to use, but i guess that's a drop in the ocean compared to the declarative part of the command metadata, and indeed more inline with other APIs (in which we ask the user to explicitly set a version field in a struct), and of course avoids the static function and special naming.
so anyway, i'm in favor of what Yossi proposed.

oranagra
oranagra reviewed Feb 3, 2022
View reviewed changes
@zuiderkwast
Copy link
Contributor Author

zuiderkwast commented Feb 3, 2022 •
edited
Loading

@zuiderkwast @oranagra I've pushed a private commit with an alternative here: yossigo@13219d3

This seems a bit more straight forward to me. Callers need to pass an extra REDISMODULE_COMMANDINFO_VERSION argument to RM_SetCommandInfo(), but that is pretty much aligned with how other APIs work. The upside is we can avoid the inline function and name mangling.

@yossigo OK. I agree it's more strait forward. In that case, isn't it better to put the version field back inside the info struct, like in the other APIs, and make it a struct pointer? The version macro can expand to a pointer to a static constant struct defined in redismodule.h. WDYT?

One more reason to get rid of the RM_SetCommandInfo_ trick is that this trick doesn't work:

if (RedisModule_SetCommandInfo != NULL) {
    RedisModule_SetCommandInfo(cmd, info);
}

@oranagra
Copy link
Member

oranagra commented Feb 3, 2022

putting this as a first first member of the info struct means we can't extend it with more size fields.

@zuiderkwast
Copy link
Contributor Author

zuiderkwast commented Feb 3, 2022

putting this as a first first member of the info struct means we can't extend it with more size fields.

We can if it's a pointer to a version struct.

@yossigo
Copy link
Collaborator

yossigo commented Feb 3, 2022

@zuiderkwast I thought about that, but wasn't very happy with defining a static variable in redismodule.h. We'd need to silence errors about unused definition, and we'd rely on the toolchain to make sure it doesn't really end up in every object file. This version makes it completely ephemeral and the API is relatively clean (pass a VERSION macro like everywhere else).

@oranagra
Copy link
Member

oranagra commented Feb 3, 2022

we actually already have these.
see RedisModuleEvent_ReplicationRoleChanged and friends.

@zuiderkwast
Copy link
Contributor Author

zuiderkwast commented Feb 3, 2022 •
edited
Loading

Exactly, the events are all static const. They end up in every object file, in modules which don't use events. Also, all the API functions are global pointers. Most of them are not used by most of the modules.

@yossigo
Copy link
Collaborator

yossigo commented Feb 3, 2022

@zuiderkwast @oranagra Good points, so the pointer approach makes sense.

zuiderkwast
zuiderkwast commented Feb 3, 2022
View reviewed changes
itamarhaber
itamarhaber approved these changes Feb 3, 2022
View reviewed changes
Copy link
Member

@itamarhaber itamarhaber left a comment

Choose a reason for hiding this comment

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

LGTM && !CR

@oranagra oranagra merged commit 0a82fe8 into redis:unstable Feb 4, 2022
@zuiderkwast zuiderkwast deleted the command-info-module-api branch February 6, 2022 19:25
@oranagra oranagra mentioned this pull request Feb 28, 2022
Merged
@yoav-steinberg yoav-steinberg mentioned this pull request Apr 8, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@guybe7 guybe7 guybe7 approved these changes

@yossigo yossigo yossigo approved these changes

@itamarhaber itamarhaber itamarhaber approved these changes

@oranagra oranagra oranagra approved these changes

+2 more reviewers

@MeirShpilraien MeirShpilraien MeirShpilraien left review comments

@madolson madolson madolson approved these changes

Reviewers whose approvals may not affect merge requirements

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.

Module API for providing command infromation

7 participants

@zuiderkwast @oranagra @yossigo @itamarhaber @guybe7 @MeirShpilraien @madolson