Skip to content

Conversation

@enjoy-binbin
Copy link
Contributor

@enjoy-binbin enjoy-binbin commented May 26, 2022

There are some commands that has the wrong key specs.
This PR adds a key-spec related check in generate-command-code.py.
Check if the index is valid, or if there is an unused index.

The check result will look like:

[root]# python utils/generate-command-code.py
Processing json files...
Linking container command to subcommands...
Checking all commands...
command: RESTORE_ASKING may have unused key_spec
command: RENAME may have unused key_spec
command: PFDEBUG may have unused key_spec
command: WATCH key_specs missing flags
command: LCS arg: key2 key_spec_index error
command: RENAMENX may have unused key_spec
Error: There are errors in the commands check, please check the above logs.

The following commands have been fixed according to the check results:

  • RESTORE ASKING: add missing arguments section (and history section)
  • RENAME: newkey's key_spec_index should be 1
  • PFDEBUG: add missing arguments (and change the arity from -3 to 3)
  • WATCH: add missing key_specs flags: RO, like EXIST (it allow you to know the key exists, or is modified, but doesn't "leak" the data)
  • LCS: key2 key_spec_index error, there is only one key-spec
  • RENAMENX: newkey's key_spec_index should be 1

This was mention in #9656 (review)

@enjoy-binbin
Copy link
Contributor Author

with the new generate-command-code.py, before this PR, we will get:

[root@binblog redis]# python utils/generate-command-code.py 
Processing json files...
Linking container command to subcommands...
Checking all commands...
command: RESTORE_ASKING may have unused key_spec
command: RENAME may have unused key_spec
command: PFDEBUG may have unused key_spec
command: WATCH key_specs missing flags
command: LCS arg: key2 key_spec_index error
command: RENAMENX may have unused key_spec
Generating commands.c...
All done, exiting.

so we need to check the following commands:

  • LCS: key2 key_spec_index error
  • RENAME: have unused key_spec
  • RENAMENX: have unused key_spec
  • WATCH: key_specs missing flags?
  • SUNSUBSCRIBE / SPUBLISH / SSUBSCRIBE are NOT_KEY, i suppose they are ok
  • RESTORE-ASKING: missing arguments, should it be like RESTORE?
  • PFDEBUG: missing arguments, this is a debug command, so i suppose it is ok

the changes in py files can undo, test script (can keep it of not), and some formats

@enjoy-binbin
Copy link
Contributor Author

now we will exit in generate-command-code if we got error, like:
https://github.com/redis/redis/runs/6608468386?check_suite_focus=true#step:3:551

Processing json files...
Linking container command to subcommands...
Checking all commands...
command: PFDEBUG may have unused key_spec
Error: There are errors in the command check, please check the above logs.

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.

LGTM

@oranagra oranagra merged commit 2a099d4 into redis:unstable May 27, 2022
@enjoy-binbin enjoy-binbin deleted the fix_command_key_spec branch May 27, 2022 10:03
enjoy-binbin added a commit to enjoy-binbin/redis that referenced this pull request Jul 31, 2023
There are some commands that has the wrong key specs.
This PR adds a key-spec related check in generate-command-code.py.
Check if the index is valid, or if there is an unused index.

The check result will look like:
```
[root]# python utils/generate-command-code.py
Processing json files...
Linking container command to subcommands...
Checking all commands...
command: RESTORE_ASKING may have unused key_spec
command: RENAME may have unused key_spec
command: PFDEBUG may have unused key_spec
command: WATCH key_specs missing flags
command: LCS arg: key2 key_spec_index error
command: RENAMENX may have unused key_spec
Error: There are errors in the commands check, please check the above logs.
```

The following commands have been fixed according to the check results:
- RESTORE ASKING: add missing arguments section (and history section)
- RENAME: newkey's key_spec_index should be 1
- PFDEBUG: add missing arguments (and change the arity from -3 to 3)
- WATCH: add missing key_specs flags: RO, like EXIST (it allow you to know the key exists, or is modified, but doesn't "leak" the data)
- LCS: key2 key_spec_index error, there is only one key-spec
- RENAMENX: newkey's key_spec_index should be 1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants