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

Move doc metadata from COMMAND to COMMAND DOCS #10056

Merged
merged 13 commits into from
Jan 11, 2022
Merged

Move doc metadata from COMMAND to COMMAND DOCS #10056

merged 13 commits into from
Jan 11, 2022

Conversation

oranagra
Copy link
Member

@oranagra oranagra commented Jan 5, 2022

Syntax:
COMMAND DOCS [<command name> ...]

Background:
Apparently old version of hiredis (and thus also redis-cli) can't
support more than 7 levels of multi-bulk nesting.

The solution is to move all the doc related metadata from COMMAND to a
new COMMAND DOCS sub-command.

The new DOCS sub-command returns a map of commands (not an array like in COMMAND),
And the same goes for the subcommands field inside it (also contains a map)

Besides that, the remaining new fields of COMMAND (hints, key-specs, and
sub-commands), are placed in the outer array rather than a nested map.
this was done mainly for consistency with the old format.

Other changes:

  • Allow COMMAND INFO with no arguments, which returns all commands, so that we can some day deprecated
    the plain COMMAND (no args)

  • Reduce the amount of deferred replies from both COMMAND and COMMAND
    DOCS, especially in the inner loops, since these create many small
    reply objects, which lead to many small write syscalls and many small
    TCP packets.
    To make this easier, when populating the command table, we count the
    history, args, and hints so we later know their size in advance.
    Additionally, the movablekeys flag was moved into the flags register.

  • Update generate-commands-json.py to take the data from both commands, it
    now executes redis-cli directly, instead of taking input from stdin.

  • Sub-commands in both COMMAND (and COMMAND INFO), and also COMMAND DOCS, show their full name. i.e. CONFIG
    GET will be shown as config|get rather than just get.
    This will be visible both when asking for COMMAND INFO config and COMMAND INFO config|get, but is especially
    important for the later.
    i.e. imagine someone doing COMMAND INFO slowlog|get config|get not being able to distinguish between the two items in
    the array response.

Syntax:
COMMAND DETAILS [<command name> ...]

Background:
Apparently old version of hiredis (and thus also redis-cli) can't
support more than 7 levels of multi-bulk nesting.

The solution is to move all the doc related metadata from COMMAND to a
new COMMAND DETAILS sub-command.

Besides that, the remaining new fields of COMMAND (hints, key-specs, and
sub-commands), are placed in the outer array rather than a nested map.
this was done mainly for consistency with the old format.

Other changes:
---

Reduce the amount of deferred replies from both COMMAND and COMMAND
DETAILS, especially in the inner loops, since these create many small
reply objects, which lead to many small write syscalls and many small
TCP packets.
To make this easier, when populating the command table, we count the
history, args, and hints so we later know their size in advance.
Additionally, the movablekeys flag was moved into the flags register.

Update generate-commands-json.py to take the data from both command, it
now executes redis-cli directly, instead of taking input from stdin.

When a user specifically asks for a sub-command using COMMAND INFO <cmd>,
or COMMAND DETAILS <cmd>, the response will contain the full command
name, rather than just the sub-command name.
i.e. `COMMAND INFO "config|get"` will return "config get" as name,
rather than just "get".
unlike asking for "config" (or asking for all commands), which will
return a section about "get" in a nested block inside "config".
@oranagra oranagra added this to Backlog in 7.0 via automation Jan 5, 2022
@oranagra oranagra moved this from Backlog to In Review in 7.0 Jan 5, 2022
@oranagra oranagra linked an issue Jan 5, 2022 that may be closed by this pull request
Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

👍

src/server.c Outdated Show resolved Hide resolved
src/server.c Outdated Show resolved Hide resolved
utils/generate-commands-json.py Show resolved Hide resolved
src/server.c Show resolved Hide resolved
Co-authored-by: Viktor Söderqvist <viktor@zuiderkwast.se>
src/commands.c Outdated Show resolved Hide resolved
src/server.c Outdated Show resolved Hide resolved
src/server.c Outdated Show resolved Hide resolved
src/server.c Outdated Show resolved Hide resolved
src/server.c Outdated Show resolved Hide resolved
@oranagra
Copy link
Member Author

oranagra commented Jan 6, 2022

@redis/core-team please approve (see the top comment), and also state if you think the new command should be named COMMAND DOCS rather than COMMAND DETAILS (which resembles COMMAND INFO too much)

@oranagra oranagra moved this from In Review to Awaits merge in 7.0 Jan 6, 2022
@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 state:needs-doc-pr requires a PR to redis-doc repository labels Jan 6, 2022
src/server.c Show resolved Hide resolved
@moticless
Copy link
Collaborator

moticless commented Jan 6, 2022

Hi,
I verified that after this commit, data is aggregated and transmitted in large chunks.

  • Run command command from redis-cli and counted tcp packets.
    • Before the change counted 2502 packets from redis-server, total payload of 240K.
    • After the change only 35 packets (yet, first ~30 packets are still small, few hundred bytes each and then 2 packets of size 32k - nonoptimized infra ...), total payload of 79K.
  • Run src/redis-benchmark -n 10000 COMMAND before and after the change:
  throughput summary: 103.92 requests per second
  latency summary (msec):
          avg       min       p50       p95       p99       max
      174.968     8.704   177.407   226.815   246.911   327.167

  throughput summary: 574.05 requests per second
  latency summary (msec):
          avg       min       p50       p95       p99       max
       62.892     1.224    64.639    80.831    83.135    89.663

src/server.c Outdated Show resolved Hide resolved
src/server.c Outdated Show resolved Hide resolved
@zuiderkwast
Copy link
Contributor

decide if we want that sub-command named COMMAND DOCS or COMMAND DETAILS

I would assume COMMAND DOCS would give the full docs as on the documentation web page. Thus, I prefer DETAILS (or possibly something else... EXTRAINFO, MOREINFO, DOCINFO, MISC...?)

@yossigo
Copy link
Member

yossigo commented Jan 10, 2022

@oranagra

decide if we want that sub-command named COMMAND DOCS or COMMAND DETAILS (considering we decided that the guideline to choose what's there is if it's not normally needed by clients). also note that having COMMAND INFO and COMMAND DETAILS is confusing (they both seem similar)

How about COMMAND METAINFO to indicate it is related but extends INFO?

maybe we can consider the sub-command nested reply to always contains the parent command name? (not just when explicitly asking for a sub-command)? if we do that, probably use space as a separator, and not |

IIUC, this means the structure of the reply will be the same (in terms of levels) but the command will include all subcommands, whereas a subcommand will only list its parent command and itself (but no siblings). Is that what you mean? If so, think this makes sense.

@oranagra
Copy link
Member Author

@yossigo i don't think what you described is what i meant.
here's the current output:

oran@Oran-laptop:~/work/redis2$ src/redis-cli command info config
1)  1) "config"
    2) (integer) -2
   ...
   10) 1)  1) "resetstat"
       2)  1) "get"
           2) (integer) -3
   ...
       5)  1) "set"
   ...

oran@Oran-laptop:~/work/redis2$ src/redis-cli command info "config|get"
1)  1) "config|get"
    2) (integer) -3
   ...

oran@Oran-laptop:~/work/redis2$ src/redis-cli command details config
1) "config"
2)  1) "summary"
    2) "A container for server configuration commands"
   ...
    9) "subcommands"
   10)  1) "rewrite"
   ...
        5) "get"
        6)  1) "summary"
            2) "Get the values of configuration parameters"
   ...
        7) "set"
   ...

oran@Oran-laptop:~/work/redis2$ src/redis-cli command details "config|get"
1) "config|get"
2)  1) "summary"
    2) "Get the values of configuration parameters"
   ...

now here are the alternatives:

  1. always use the full command name (i.e. even in the top most example, you'll see config|get instead of just get)
  2. never use the full command name (i.e. if you explicitly ask for config|get, you'll just see get in the response, and can't distinguish between slowlog|get and config|get).
  3. maybe what you suggested, which is that if someone asks for config|get we reply with config's metadata too, but without other sub-commands (i think this is more complicated at both the implementation side, and also for the reader)

@oranagra
Copy link
Member Author

for the record, this was discussed in a core-team meeting and got a green light to merge.
deciding on COMMAND DOCS and always returning the full command names for sub-commands (e.g. config|get)

@zuiderkwast
Copy link
Contributor

Why the pipe in "config|get"? A space would look much nicer but I guess there is some context where it doesn't work...?

@oranagra
Copy link
Member Author

some context on the pipe char.
it was first introduced by Salvatore in version 6.0 for ACL, as a way to set ACL rules on sub-commands (e.g. +config|set).
later in #9504 (unreleased), Guy added that syntax in many places (even COMMAND INFO, and the output of INFO commandstats).

so it looks like we're already "invested" in having the pipe char as a separator when a single string refers to a sub-command's full name.

src/server.c Outdated Show resolved Hide resolved
@oranagra oranagra changed the title Move doc metadata from COMMAND to COMMAND DETAILS Move doc metadata from COMMAND to COMMAND DOCS Jan 11, 2022
src/commands/command-docs.json Outdated Show resolved Hide resolved
@oranagra oranagra merged commit 3204a03 into redis:unstable Jan 11, 2022
@oranagra oranagra deleted the command_details branch January 11, 2022 15:16
@oranagra oranagra moved this from Awaits merge to Done in 7.0 Jan 11, 2022
oranagra pushed a commit that referenced this pull request Feb 22, 2022
…9934)

There are scenarios where it results in many small objects in the reply list,
such as commands heavily using deferred array replies (`addReplyDeferredLen`).
E.g. what COMMAND command and CLUSTER SLOTS used to do (see #10056, #7123),
but also in case of a transaction or a pipeline of commands that use just one differed array reply.

We used to have to run multiple loops along with multiple calls to `write()` to send data back to
peer based on the current code, but by means of `writev()`, we can gather those scattered
objects in reply list and include the static reply buffer as well, then send it by one system call,
that ought to achieve higher performance.

In the case of TLS,  we simply check and concatenate buffers into one big buffer and send it
away by one call to `connTLSWrite()`, if the amount of all buffers exceeds `NET_MAX_WRITES_PER_EVENT`,
then invoke `connTLSWrite()` multiple times to avoid a huge massive of memory copies.

Note that aside of reducing system calls, this change will also reduce the amount of
small TCP packets sent.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 state:needs-doc-pr requires a PR to redis-doc repository
Projects
Archived in project
7.0
Done
Development

Successfully merging this pull request may close these issues.

Make sure the new output of COMMAND doesn't break old redis-cli
7 participants