redis-cli generates command help tables from the results of COMMAND #10043

jhelbaum · 2022-01-03T10:51:25Z

This is a followup to #9656 and implements the following step mentioned in that PR:

When possible, extract all the help and completion tips from COMMAND DOCS (Redis 7.0 and up)
If COMMAND DOCS fails, use the static help.h compiled into redis-cli.
Supplement additional command names from COMMAND (pre-Redis 7.0)

The last step is needed to add module command and other non-standard commands.

This PR does not change the interactive hinting mechanism, which still uses only the param strings to provide somewhat unreliable and inconsistent command hints (see #8084). That task is left for a future PR.

oranagra · 2022-01-03T12:30:49Z

thanks for your PR.. started looking at it..
i now realize that deleting help.h would mean that new redis-cli will be unable to provide any hints when connected to old servers (in the past it just used to provide the wrong hings).
one alternative is to keep the old help.h just for use in old servers
but that would probably complicate the code, and also, i suppose it'll mean we'll have to keep the old help.h forever.
anyone has any other ideas before we embrace that "limitation"?

jhelbaum · 2022-01-03T13:41:09Z

Right - that would mean keeping the existing help.h mechanism as a fallback for when COMMAND returns the old format. Basically, keep all the code this PR deletes, behind a conditional. We could keep it for a predefined transition period, until the old server version is no longer supported. But sure, it would complicate the code.

oranagra · 2022-01-03T15:14:50Z

ok. i don't like it.. but just wanted to raise it.

oranagra

@jhelbaum i did a sloppy initial review (only looking at the diff, without being familiar of the code that uses it).
can't afford to spend much more time on that right now, so i figured raw feedback is better than nothing.

src/redis-cli.c

guybe7 · 2022-01-04T15:47:42Z

for the record, i guess it's not so bad if a new redis-cli doesn't show hints when running with an old redis server (I would imagine the more common case is an old redis-cli with an new redis-server)

oranagra · 2022-01-04T15:49:40Z

i'm not sure that's right.. someone managing many redis servers, will maybe update his redis-cli but keep using it for both old and new servers.

oranagra · 2022-01-12T07:15:40Z

@jhelbaum FYI, #10056 was merged.

jhelbaum · 2022-01-12T18:42:41Z

Can redis-cli assume that COMMAND INFO and COMMAND DOCS return the commands in the same order? It would simplify the code.

oranagra · 2022-01-12T19:11:14Z

currently that's the case, and i can't think of any reason why the implementation could be changed in a way that'll break that.
i.e. both commands recursively run on the same dict.
that said, it seems like an invalid assumption to make (each command has it's own documented semantics, but they have nothing to do with each other, formally)

if-else style Co-authored-by: Oran Agra <oran@redislabs.com>

jhelbaum · 2022-01-14T01:06:29Z

COMMAND INFO is not necessary for this purpose. The code now only calls COMMAND DOCS, which has all the relevant information for the help strings.

oranagra · 2022-01-25T15:19:13Z

@jhelbaum forgive me for the luck of focus. what's the status here?
ready for merge?

jhelbaum · 2022-01-25T15:58:30Z

As far as I'm concerned, it's ready. I don't get the sense that it's had a thorough code review, though. I also don't know if there are any tests that are usually run on redis-cli?

…

On Tue, Jan 25, 2022 at 5:19 PM Oran Agra ***@***.***> wrote: @jhelbaum <https://github.com/jhelbaum> forgive me for the luck of focus. what's the status here? ready for merge? — Reply to this email directly, view it on GitHub <#10043 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AH5PZMMIUZWP4QOM7NY3XBDUX25QBANCNFSM5LE7HCOQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

oranagra · 2022-01-25T16:41:37Z

I'll give it another review.. just wasn't sure there are still changes pending or it's ready from your side.
there are some tests in tests/integration/redis-cli.tcl but I i'm not sure if the infrastructure can serve for our purpose here.
and also, i suppose manual testing is enough for this feature, not sure we need regression tests.

src/redis-cli.c

jhelbaum · 2022-01-26T08:56:55Z

Another thought: Presumably in a separate PR, would anyone object to breaking this up into multiple files? 8800 lines is a bit much much for one source file.

oranagra · 2022-01-26T09:31:11Z

i agree. but i don't see a point in just breaking it up in a PR that does only that..
we should do some major revamp in it some day, maybe even split it into multiple tools, or multiple units bundled in one tool.
i don't have a concrete idea. just agree that what we currently have is a mess.

src/redis-cli.c

…unstable

jhelbaum · 2022-02-04T00:41:31Z

Okay, I've reinstated the static help.h file for use with pre-7.0 servers, along with the code that generates the help tables from it.

I still can't figure out why some of the tests are timing out. Any leads in investigating the problem would be appreciated.

Testing integration/redis-cli
[TIMEOUT]: clients state report follows.
sock55d1e5c59510 => (IN PROGRESS) Interactive CLI: INFO response should be printed raw

oranagra

just a quick skim though the recent changes.

utils/generate-command-help.rb

src/redis-cli.c

oranagra · 2022-02-04T10:35:54Z

I can't figure out why the test hangs on GH actions (it passes locally).
@yossigo maybe you can figure it out?

jhelbaum · 2022-02-04T11:59:56Z

It's been broken since at least 896dad5

oranagra · 2022-02-04T14:27:57Z

it was actually easy to reproduce locally if using the same make line as the one the CI uses.
the test hang because redis-cli crashed on SIGABRT because realloc was called with a bad address.

oranagra · 2022-02-04T14:38:34Z

p.s. zrealloc uses realloc, but has this in smalloc.c:

#define realloc(ptr,size) je_realloc(ptr,size)

so when you call realloc from redis-cli.c, it uses libc allocator.
but if you debug this with make valgrind or any system that doesn't use jemalloc by default, then both use libc, which is why it didn't reproduce locally for you.

jhelbaum · 2022-02-04T14:41:22Z

Okay, I've learned something. Thanks.

…

On Fri, Feb 4, 2022, 16:38 Oran Agra ***@***.***> wrote: p.s. zrealloc uses realloc, but has this in smalloc.c: #define realloc(ptr,size) je_realloc(ptr,size) so when you call realloc from redis-cli.c, it uses libc allocator. but if you debug this with make valgrind or any system that doesn't use jemalloc by default, then both use libc, which is why it didn't reproduce locally for you. — Reply to this email directly, view it on GitHub <#10043 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AH5PZMOG2ZROREBE3VS4F63UZPQHNANCNFSM5LE7HCOQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

oranagra · 2022-02-05T10:43:42Z

Maybe we should automatically hide suggestions based on version?
I.e. Send HELLO or INFO at startup, and use the version to hide irrelevant suggestions?
or maybe we should remove commands and args that are newer than 6.2 from help.h when generating it?
The first option is nicer, but not sure how much effort we wanna invest in old version support (since in versions newer than 7, it'll happen automatically since we use command DOCS)

oranagra

for the record, some tests i did (with valgrind).

run it against latest server and against old server.
added to both servers some commands that don't exist in help.h, and verified they show in help and in completion hints.
run it with and without -3
verified the help @<cat> is working

the only problem i could spot (which may not be an issue), is that when using help.h (on old server) we show the since field, but when using COMMAND DOCS (new server) we don't.
it's logical since when we use COMMAND DOCS we show only the ones actually supported.
but since the users can't distinguish between these modes, i now think it'll be nicer to show it in both modes.

tests/support/util.tcl

Because SENTINEL DEBUG missing summary in its json file, with the change in redis#10043, the following assertion will fail. ``` [redis]# src/redis-cli -p 26379 redis-cli: redis-cli.c:678: cliInitCommandHelpEntry: Assertion `reply->type == 1' failed. ``` This commit add the summary and complexity for SENTINEL DEBUG, which introduced in redis#9291, and also improved the help message.

…ry (#10250) Fix redis-cli with sentinel crash due to SENTINEL DEBUG missing summary Because SENTINEL DEBUG missing summary in its json file, with the change in #10043, the following assertion will fail. ``` [redis]# src/redis-cli -p 26379 redis-cli: redis-cli.c:678: cliInitCommandHelpEntry: Assertion `reply->type == 1' failed. ``` This commit add the summary and complexity for SENTINEL DEBUG, which introduced in #9291, and also improved the help message.

If summary or since is empty, we used to return NULL in COMMAND DOCS. Currently all redis commands will have these two fields. But not for module command, summary and since are optional for RM_SetCommandInfo. With the change in redis#10043, if a module command doesn't have the summary or since, redis-cli will crash (see redis#10250). In this commit, COMMAND DOCS avoid adding summary or since when they are missing.

If summary or since is empty, we used to return NULL in COMMAND DOCS. Currently all redis commands will have these two fields. But not for module command, summary and since are optional for RM_SetCommandInfo. With the change in #10043, if a module command doesn't have the summary or since, redis-cli will crash (see #10250). In this commit, COMMAND DOCS avoid adding summary or since when they are missing.

The following usage will output an empty newline: ``` > redis-cli help set empty line ``` The reason is that in interactive mode, we have called `cliInitHelp`, which initializes help. When using `redis-cli help xxx` or `redis-cli help ? xxx`, we can't match the command due to empty `helpEntries`, so we output an empty newline. In this commit, we will call `cliInitHelp` to init the help. Note that in this case, we need to call `cliInitHelp` (COMMAND DOCS) every time, which i think is acceptable. So now the output will look like: ``` [redis]# src/redis-cli help get GET key summary: Get the value of a key since: 1.0.0 group: string [redis]# ``` Fixes #10378 This PR also fix a redis-cli crash when using `--ldb --eval`: ``` [root]# src/redis-cli --ldb --eval test.lua test 1 Lua debugging session started, please use: quit -- End the session. restart -- Restart the script in debug mode again. help -- Show Lua script debugging commands. * Stopped at 1, stop reason = step over -> 1 local num = redis.call('GET', KEYS[1]); redis-cli: redis-cli.c:718: cliCountCommands: Assertion `commandTable->element[i]->type == 1' failed. Aborted ``` Because in ldb mode, `COMMAND DOCS` or `COMMAND` will return an array, only with one element, and the type is `REDIS_REPLY_STATUS`, the result is `<error> Unknown Redis Lua debugger command or wrong number of arguments`. So if we are in the ldb mode, and init the Redis HELP, we will get the wrong response and crash the redis-cli. In ldb mode we don't initialize HELP, help is only initialized after the lua debugging session ends. It was broken in #10043

oranagra reviewed Jan 3, 2022

View reviewed changes

oranagra added the 7.0-must-have label Jan 3, 2022

jhelbaum and others added 9 commits January 14, 2022 00:58

Build redis-cli command help from output of COMMAND

ac49422

Remove obsolete help.h file

cb543ea

Update src/redis-cli.c

fe4ff48

if-else style Co-authored-by: Oran Agra <oran@redislabs.com>

Remove obsolete file.

bdd28a1

Update reference to help.h

628abcc

Style review fixes

1d27992

Adding type checks for COMMAND reply elements

14911f1

Support help table format from COMMAND DOCS

f79e1cb

redis-cli command help: Add support for RESP3

bb63d64

jhelbaum force-pushed the cli-help-strings branch from 58be967 to bb63d64 Compare January 14, 2022 01:01

jhelbaum added 2 commits January 17, 2022 17:32

Change helpIndex from int to pointer

008304f

Support fallback help info from COMMAND output.

b8ef071

jhelbaum commented Jan 25, 2022

View reviewed changes

src/redis-cli.c Outdated Show resolved Hide resolved

oranagra reviewed Jan 26, 2022

View reviewed changes

src/redis-cli.c Outdated Show resolved Hide resolved

src/redis-cli.c Outdated Show resolved Hide resolved

src/redis-cli.c Outdated Show resolved Hide resolved

src/redis-cli.c Outdated Show resolved Hide resolved

jhelbaum and others added 5 commits February 3, 2022 02:47

Merge branch 'unstable' of https://github.com/redis/redis into redis-…

5d68541

…unstable

Merge branch 'redis-unstable' into cli-help-strings

4e758a2

Remove generate-command-help.rb

aaadc23

Use help.h for pre-7.0 servers

1d1ad6f

Merge branch 'redis:unstable' into cli-help-strings

e35a683

oranagra reviewed Feb 4, 2022

View reviewed changes

utils/generate-command-help.rb Show resolved Hide resolved

src/redis-cli.c Outdated Show resolved Hide resolved

jhelbaum added 2 commits February 4, 2022 13:55

Minor fix

b26478c

Restore utils/generate-command-help.rb

5b4474e

fix sigabrt due to mixing allocators

7c065f1

oranagra reviewed Feb 5, 2022

View reviewed changes

tests/support/util.tcl Outdated Show resolved Hide resolved

oranagra added 2 commits February 5, 2022 16:10

CR fixes

18e0f57

undo an unneeded change to an otherwise unmodified file

73f14a2

oranagra approved these changes Feb 5, 2022

View reviewed changes

oranagra merged commit 5b17909 into redis:unstable Feb 5, 2022

jhelbaum deleted the cli-help-strings branch February 6, 2022 13:48

enjoy-binbin mentioned this pull request Feb 7, 2022

Fix redis-cli with sentinel crash due to SENTINEL DEBUG missing summary #10250

Merged

enjoy-binbin mentioned this pull request Feb 7, 2022

COMMAND DOCS avoid adding summary/since if they don't exist #10252

Merged

enjoy-binbin mentioned this pull request Mar 4, 2022

Initialize help when using redis-cli help or redis-cli ? #10382

Merged

redis-cli generates command help tables from the results of COMMAND #10043

redis-cli generates command help tables from the results of COMMAND #10043

Uh oh!

Conversation

jhelbaum commented Jan 3, 2022 • edited by oranagra Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oranagra commented Jan 3, 2022

Uh oh!

jhelbaum commented Jan 3, 2022

Uh oh!

oranagra commented Jan 3, 2022

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

guybe7 commented Jan 4, 2022

Uh oh!

oranagra commented Jan 4, 2022

Uh oh!

oranagra commented Jan 12, 2022

Uh oh!

jhelbaum commented Jan 12, 2022

Uh oh!

oranagra commented Jan 12, 2022

Uh oh!

jhelbaum commented Jan 14, 2022

Uh oh!

oranagra commented Jan 25, 2022

Uh oh!

jhelbaum commented Jan 25, 2022 via email

Uh oh!

oranagra commented Jan 25, 2022

Uh oh!

Uh oh!

jhelbaum commented Jan 26, 2022

Uh oh!

oranagra commented Jan 26, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jhelbaum commented Feb 4, 2022

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

oranagra commented Feb 4, 2022

Uh oh!

jhelbaum commented Feb 4, 2022

Uh oh!

oranagra commented Feb 4, 2022

Uh oh!

oranagra commented Feb 4, 2022

Uh oh!

jhelbaum commented Feb 4, 2022 via email

Uh oh!

oranagra commented Feb 5, 2022

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

jhelbaum commented Jan 3, 2022 •

edited by oranagra

Loading