Skip to content
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.

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

Refresh COMMAND, add topics: command-args, key-specs #1697

Merged
merged 28 commits into from
Jan 24, 2022

Conversation

guybe7
Copy link
Contributor

@guybe7 guybe7 commented Dec 1, 2021

Add new COMMAND command features implemented in redis/redis#9656 and it's predecessor PRs (key-specs, and sub-commands)
Plus some other cleanup and improvements.

TODO:

  • Document COMMAND LIST
  • Write the key-specs topic
  • Write the command hints
  • Document key-spec-index (new field for command argument)
  • update the list of command flags

commands/command.md Outdated Show resolved Hide resolved
commands/command.md Outdated Show resolved Hide resolved
commands/command.md Outdated Show resolved Hide resolved
commands/command.md Outdated Show resolved Hide resolved
commands/command.md Outdated Show resolved Hide resolved
commands/command.md Outdated Show resolved Hide resolved
commands/command.md Outdated Show resolved Hide resolved
commands/command-list.md Outdated Show resolved Hide resolved
commands.json Outdated Show resolved Hide resolved
commands/command-list.md Outdated Show resolved Hide resolved
commands/command.md Outdated Show resolved Hide resolved
commands/command.md Outdated Show resolved Hide resolved
topics/command-info.md Outdated Show resolved Hide resolved
topics/command-info.md Outdated Show resolved Hide resolved
topics/command-info.md Outdated Show resolved Hide resolved
topics/command-info.md Outdated Show resolved Hide resolved
topics/command-info.md Outdated Show resolved Hide resolved
topics/command-info.md Outdated Show resolved Hide resolved
guybe7 and others added 2 commits December 5, 2021 10:17
Co-authored-by: Itamar Haber <itamar@redis.com>
Co-authored-by: Itamar Haber <itamar@redis.com>
@guybe7 guybe7 added to-be-merged should probably be merged soon waiting-for-upstream waiting for a redis PR to be merged labels Dec 15, 2021
Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

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

please run this though a spell checker.

topics/key-specs.md Outdated Show resolved Hide resolved
topics/key-specs.md Outdated Show resolved Hide resolved
topics/key-specs.md Outdated Show resolved Hide resolved
oranagra pushed a commit to redis/redis that referenced this pull request Dec 15, 2021
Delete the hardcoded command table and replace it with an auto-generated table, based
on a JSON file that describes the commands (each command must have a JSON file).

These JSON files are the SSOT of everything there is to know about Redis commands,
and it is reflected fully in COMMAND INFO.

These JSON files are used to generate commands.c (using a python script), which is then
committed to the repo and compiled.

The purpose is:
* Clients and proxies will be able to get much more info from redis, instead of relying on hard coded logic.
* drop the dependency between Redis-user and the commands.json in redis-doc.
* delete help.h and have redis-cli learn everything it needs to know just by issuing COMMAND (will be
  done in a separate PR)
* redis.io should stop using commands.json and learn everything from Redis (ultimately one of the release
  artifacts should be a large JSON, containing all the information about all of the commands, which will be
  generated from COMMAND's reply)
* the byproduct of this is:
  * module commands will be able to provide that info and possibly be more of a first-class citizens
  * in theory, one may be able to generate a redis client library for a strictly typed language, by using this info.

### Interface changes

#### COMMAND INFO's reply change (and arg-less COMMAND)

Before this commit the reply at index 7 contained the key-specs list
and reply at index 8 contained the sub-commands list (Both unreleased).
Now, reply at index 7 is a map of:
- summary - short command description
- since - debut version
- group - command group
- complexity - complexity string
- doc-flags - flags used for documentation (e.g. "deprecated")
- deprecated-since - if deprecated, from which version?
- replaced-by - if deprecated, which command replaced it?
- history - a list of (version, what-changed) tuples
- hints - a list of strings, meant to provide hints for clients/proxies. see #9876
- arguments - an array of arguments. each element is a map, with the possibility of nesting (sub-arguments)
- key-specs - an array of keys specs (already in unstable, just changed location)
- subcommands - a list of sub-commands (already in unstable, just changed location)
- reply-schema - will be added in the future (see #9845)

more details on these can be found in redis/redis-doc#1697

only the first three fields are mandatory 

#### API changes (unreleased API obviously)

now they take RedisModuleCommand opaque pointer instead of looking up the command by name

- RM_CreateSubcommand
- RM_AddCommandKeySpec
- RM_SetCommandKeySpecBeginSearchIndex
- RM_SetCommandKeySpecBeginSearchKeyword
- RM_SetCommandKeySpecFindKeysRange
- RM_SetCommandKeySpecFindKeysKeynum

Currently, we did not add module API to provide additional information about their commands because
we couldn't agree on how the API should look like, see #9944.

### Somehow related changes
1. Literals should be in uppercase while placeholder in lowercase. Now all the GEO* command
   will be documented with M|KM|FT|MI and can take both lowercase and uppercase

### Unrelated changes
1. Bugfix: no_madaory_keys was absent in COMMAND's reply
2. expose CMD_MODULE as "module" via COMMAND
3. have a dedicated uint64 for ACL categories (instead of having them in the same uint64 as command flags)

Co-authored-by: Itamar Haber <itamar@garantiadata.com>
hwware pushed a commit to hwware/redis that referenced this pull request Dec 20, 2021
Delete the hardcoded command table and replace it with an auto-generated table, based
on a JSON file that describes the commands (each command must have a JSON file).

These JSON files are the SSOT of everything there is to know about Redis commands,
and it is reflected fully in COMMAND INFO.

These JSON files are used to generate commands.c (using a python script), which is then
committed to the repo and compiled.

The purpose is:
* Clients and proxies will be able to get much more info from redis, instead of relying on hard coded logic.
* drop the dependency between Redis-user and the commands.json in redis-doc.
* delete help.h and have redis-cli learn everything it needs to know just by issuing COMMAND (will be
  done in a separate PR)
* redis.io should stop using commands.json and learn everything from Redis (ultimately one of the release
  artifacts should be a large JSON, containing all the information about all of the commands, which will be
  generated from COMMAND's reply)
* the byproduct of this is:
  * module commands will be able to provide that info and possibly be more of a first-class citizens
  * in theory, one may be able to generate a redis client library for a strictly typed language, by using this info.

### Interface changes

#### COMMAND INFO's reply change (and arg-less COMMAND)

Before this commit the reply at index 7 contained the key-specs list
and reply at index 8 contained the sub-commands list (Both unreleased).
Now, reply at index 7 is a map of:
- summary - short command description
- since - debut version
- group - command group
- complexity - complexity string
- doc-flags - flags used for documentation (e.g. "deprecated")
- deprecated-since - if deprecated, from which version?
- replaced-by - if deprecated, which command replaced it?
- history - a list of (version, what-changed) tuples
- hints - a list of strings, meant to provide hints for clients/proxies. see redis#9876
- arguments - an array of arguments. each element is a map, with the possibility of nesting (sub-arguments)
- key-specs - an array of keys specs (already in unstable, just changed location)
- subcommands - a list of sub-commands (already in unstable, just changed location)
- reply-schema - will be added in the future (see redis#9845)

more details on these can be found in redis/redis-doc#1697

only the first three fields are mandatory 

#### API changes (unreleased API obviously)

now they take RedisModuleCommand opaque pointer instead of looking up the command by name

- RM_CreateSubcommand
- RM_AddCommandKeySpec
- RM_SetCommandKeySpecBeginSearchIndex
- RM_SetCommandKeySpecBeginSearchKeyword
- RM_SetCommandKeySpecFindKeysRange
- RM_SetCommandKeySpecFindKeysKeynum

Currently, we did not add module API to provide additional information about their commands because
we couldn't agree on how the API should look like, see redis#9944.

### Somehow related changes
1. Literals should be in uppercase while placeholder in lowercase. Now all the GEO* command
   will be documented with M|KM|FT|MI and can take both lowercase and uppercase

### Unrelated changes
1. Bugfix: no_madaory_keys was absent in COMMAND's reply
2. expose CMD_MODULE as "module" via COMMAND
3. have a dedicated uint64 for ACL categories (instead of having them in the same uint64 as command flags)

Co-authored-by: Itamar Haber <itamar@garantiadata.com>
Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

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

only reviewed the tips.
slightly changed the meaning of all_succeeded and one_succeeded.
taking the liberty to apply my changes to make it easier for future reviewers.

topics/command-tips.md Outdated Show resolved Hide resolved
topics/command-tips.md Outdated Show resolved Hide resolved
topics/command-tips.md Outdated Show resolved Hide resolved
topics/command-tips.md Outdated Show resolved Hide resolved
topics/command-tips.md Outdated Show resolved Hide resolved
topics/command-tips.md Outdated Show resolved Hide resolved
topics/command-tips.md Outdated Show resolved Hide resolved
topics/command-tips.md Outdated Show resolved Hide resolved
Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

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

I haven't had time to review comand-arguments.md and key-specs.md.
i trust that the information there is ok as a first version, so if someone else reviewed it, we can merge this PR.
and even if no one else reviewed it, i rather merge it as is than keep it waiting (we can always incrementally improve it in future PRs)
@itamarhaber ?

commands/command.md Outdated Show resolved Hide resolved
Comment on lines 28 to 34
- nested @array-reply of [ACL categories][ta]
- nested @array-reply of [command tips][tb]
- nested @array-reply of [key-specs][td]
- nested @array-reply of subcommands
[ta]: /topics/acl
[tb]: /topics/command-tips
[td]: /topics/key-specs
Copy link
Member

Choose a reason for hiding this comment

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

no sure about this markdown, is it sane?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

The syntax is ok, but there should be a \n before these just for safety (i.e. regular md is ok with that, but the current site's implementation is more sensitive).

Copy link
Member

Choose a reason for hiding this comment

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

@itamarhaber what do you mean? an empty line between the - nested list and the [ta] thing?
i must admit i don't understand any of this.. what does it do?

anyway, and you add a suggestion to fix it, or even just push a fix?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, will push the fixes.

commands/command.md Outdated Show resolved Hide resolved
commands/command.md Outdated Show resolved Hide resolved
commands/command.md Outdated Show resolved Hide resolved
commands/command.md Outdated Show resolved Hide resolved
commands/command.md Outdated Show resolved Hide resolved
topics/command-arguments.md Outdated Show resolved Hide resolved
topics/command-tips.md Outdated Show resolved Hide resolved
topics/command-tips.md Outdated Show resolved Hide resolved
topics/command-tips.md Outdated Show resolved Hide resolved
topics/command-tips.md Outdated Show resolved Hide resolved
topics/command-tips.md Outdated Show resolved Hide resolved
topics/command-tips.md Outdated Show resolved Hide resolved
topics/command-tips.md Outdated Show resolved Hide resolved
Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

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

ok with me..
i think the last remaining real issue is to decide if we really want to remove block_keyword

topics/command-tips.md Outdated Show resolved Hide resolved
topics/command-tips.md Outdated Show resolved Hide resolved
topics/command-tips.md Outdated Show resolved Hide resolved
topics/command-tips.md Outdated Show resolved Hide resolved
topics/command-tips.md Outdated Show resolved Hide resolved
@oranagra
Copy link
Member

@itamarhaber the redis PR was merged.
please merge when ready (and then do some sanity check on the live version)

@oranagra oranagra removed the waiting-for-upstream waiting for a redis PR to be merged label Jan 20, 2022
@oranagra
Copy link
Member

@itamarhaber it is impossible to review your last change, can you please sum up what you changed?
is it mainly about wording? also added / removed some information? moved things around?

@itamarhaber
Copy link
Member

itamarhaber commented Jan 23, 2022

@oranagra edits are grammatic, syntactic, and formatting mostly.

No content has been intentionally added/removed - I think it can be merged.

@oranagra
Copy link
Member

ok. go merge.
just please confirm that it has all the recent changes (if applicable):
redis/redis#10147
redis/redis#10104
redis/redis#10127
redis/redis#10122
redis/redis#10056

@itamarhaber
Copy link
Member

@oranagra thanks for the list - reviewed.

The only thing that bothers me a little is that we have new dangling topics (topics/command-tips, topics/key-specs, topics/command-arguments) without a reference from the documentation view of the website (separate PR needed). I'm not sure it is actually needed, but if so I was thinking about adding Command runtime introspection section after the cluster's.

@oranagra
Copy link
Member

@itamarhaber i do agree we need to bind these topics somehow in the general docs section, same as we should for "programability".
feel free to merge this one and do that in another PR.

@oranagra oranagra merged commit 06f3598 into redis:master Jan 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
to-be-merged should probably be merged soon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants