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

Make shutdown command more intuitively with syntax #13153

Open
wants to merge 3 commits into
base: unstable
Choose a base branch
from

Conversation

LiiNen
Copy link
Contributor

@LiiNen LiiNen commented Mar 18, 2024

This PR contains fix about missing syntax hint in #9872, which is added from Redis 7.0.

--

The point is that we cannot use ABORT with others.

ABORT cancels an ongoing shutdown and cannot be combined with other flags.

But refer to redis.io, Syntax of command SHUTDOWN is below:
SHUTDOWN [NOSAVE | SAVE] [NOW] [FORCE] [ABORT]

Now in redis-cli of unstable branch, hint of shutdown does not seem properly like below:
image

When using abort with other flags, of course it is syntax error, but it seems like that we can use it together.

// src/db.c

if ((abort && flags != SHUTDOWN_NOFLAGS) ||
    (flags & SHUTDOWN_NOSAVE && flags & SHUTDOWN_SAVE))
{
    /* Illegal combo. */
    addReplyErrorObject(c,shared.syntaxerr);
    return;
}

image

--

What have been fixed

So, I wrote this minor fix PR to let us know right syntax properly:
image

When using one of nosave, save, now, and force, then abort will be disappeared in hint:
image

Contrary, when using abort, then all of other flags will be disappeared:
image

If I missed naming rule of argument in src/commands/shutdown.json, then please let me know.

Best regards,

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.

I believe we wrote it like that to avoid complex-looking syntax. In the docs, we described the it in text instead:

"ABORT cancels an ongoing shutdown and cannot be combined with other flags."

A that time, we didn't have the fancy auto-complete of options in redis-cli that we have now.

src/db.c Outdated Show resolved Hide resolved
Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
@LiiNen
Copy link
Contributor Author

LiiNen commented Mar 18, 2024

I have updated PR with your comment, to make it sense. Thanks for your comment.
BTW, if it is merged to unstable or to latest stable version, is it better for me to write PR also in redis-doc (https://github.com/redis/redis-doc/pulls)?

@zuiderkwast
Copy link
Contributor

BTW, if it is merged to unstable or to latest stable version, is it better for me to write PR also in redis-doc (https://github.com/redis/redis-doc/pulls)?

The JSON files from Redis are used automatically by redis.io for the syntax of each command, but all the written text documentation is in redis-doc. A PR in redis-doc is always welcome if you want to improve some text.

@LiiNen
Copy link
Contributor Author

LiiNen commented Mar 18, 2024

Thanks for your reply. I'll write doc PR after merged, if needed :)

@CLAassistant
Copy link

CLAassistant commented Mar 24, 2024

CLA assistant check
All committers have signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants