the current SENTINEL DEBUG output:

127.0.0.1:26379> sentinel debug
 1) "INFO-PERIOD"
 2) "10000"
 3) "PING-PERIOD"
 4) "1000"
 5) "ASK-PERIOD"
 6) "1000"
 7) "PUBLISH-PERIOD"
 8) "2000"
 9) "DEFAULT-DOWN-AFTER"
10) "30000"
11) "DEFAULT-FAILOVER-TIMEOUT"
12) "180000"
13) "TILT-TRIGGER"
14) "2000"
15) "TILT-PERIOD"
16) "30000"
17) "SLAVE-RECONF-TIMEOUT"
18) "10000"
19) "MIN-LINK-RECONNECT-PERIOD"
20) "15000"
21) "ELECTION-TIMEOUT"
22) "10000"
23) "SCRIPT-MAX-RUNTIME"
24) "60000"
25) "SCRIPT-RETRY-DELAY"
26) "30000"

and we can update it with SENTINEL DEBUG key value:

127.0.0.1:26379> sentinel debug INFO-PERIOD 100 PING-PERIOD 100
OK

i guess we need to improve the help message and metion these both

It now occurs to me that all fields of RM_SetCommandInfo are optional.
this means that the same crash will happen when trying to interact with redis that has a module that registered (even an old module that doesn't use RM_SetCommandInfo).
@enjoy-binbin @jhelbaum i suppose this assertion should be removed.

look like yes.

please load some old module, make sure this is the only change that's needed, and push a PR.
thanks.

I would have expected a missing field not to be returned by COMMAND DOCS. Wouldn't that make more sense than returning an empty value?

I suppose you're right. looking at addReplyCommandDocs, i see most fields are indeed skipped when missing,
except for summary, since and group.
I suppose that when it was created maybe we where thinking of native redis commands, and not of modules?

however, i also see it was documented (that these 3 fields are always present).
@guybe7 can you maybe provide some context?

p.s. the module API (RM_SetCommandInfo) doesn't force these to be set (so even on new modules they could be NULL), which will cause COMMAND docs to reply with nil.

for module commands, the group is always "module" (the caller of RM_SetCommandInfo should not be allowed to change that field - @zuiderkwast is that the case?)

regarding modules that don't use RM_SetCommandInfo - i guess we can either reply with NULL or "NA". i think i prefer "NA"

regarding modules that use RM_SetCommandInfo - i think we want to enforce that both "summary" and "since" were provided

considering that RM_SetCommandInfo is optional, and considering all of these are metadata that we're not sure have values in some contexts, i don't think we should make any field mandatory.

so the question IMHO is if COMMAND docs should reply with Nil when this is missing, or avoid adding that entry to the map.

let's avoid adding it to the map, update the docs, and make sure no one relies on the existence of these keys

for module commands, the group is always "module" (the caller of RM_SetCommandInfo should not be allowed to change that field - @zuiderkwast is that the case?)

That seems to be the case, yes.

let's avoid adding it to the map

So, we need another PR regarding the output of COMMAND DOCS?

let's avoid adding it to the map. so just need to handle summary and since? like this

    if (cmd->summary) {
        addReplyBulkCString(c, "summary");
        addReplyBulkCString(c, cmd->summary);
    }

    if (cmd->since) {
        addReplyBulkCString(c, "since");
        addReplyBulkCString(c, cmd->since);
    }

i am testing it.

yep

Regarding the documentation, it says the "keys are always present in the reply". I would have assumed that means they have string values associated with them. If the key is guaranteed to be present, I'm not sure that associating it with a null value is compliant.

Regarding the documentation, it says the "keys are always present in the reply". I would have assumed that means they have string values associated with them. If the key is guaranteed to be present, I'm not sure that associating it with a null value is compliant.

@jhelbaum I think we need to update the docs too then. It is done with a PR to https://github.com/redis/redis-doc/blob/master/commands/command-docs.md

[Edit] It was done already: redis/redis-doc#1780

[Edit again] ... but I see it didn't change the line before the list of fields "The following keys are always present in the reply:", so I guess we can have another PR. :-)