Allow configuring signaled shutdown flags#10594
Conversation
46985d6 to
8c2a5c4
Compare
8c2a5c4 to
fd8a33d
Compare
|
@oranagra I've ended up updating createEnumConfig to support |
fd8a33d to
1f1e82e
Compare
oranagra
left a comment
There was a problem hiding this comment.
neat! i had a feeling this is not that complicate to re-use the enum code.
i feel we're missing some tests. maybe one about shutdown, but also some around the config.
I think we may want to expose this to modules too (see #10285), maybe that can be done later.
|
regarding the module configs, i think that all we need to do is add a new |
|
@eduardobr sorry for hogging your PR, i thought it may be better to judge this whole idea of enum flags in one piece. |
| if (!names || values != matches) { | ||
| sdsfree(names); | ||
| return sdsnew("unknown"); | ||
| } |
There was a problem hiding this comment.
@oranagra Now that you've removed configEnumGetNameOrUnknown, maybe we can panic here instead of returning "unkonwn".
There was a problem hiding this comment.
i'm having second thoughts.. i wonder why it was there in the first place.
maybe to serve the INFO command, but still how come we expected a state in which the integer doesn't have a corresponding enum?
maybe the thought is that panic should be used to protect further data corruption due to a bug, or hidden consecutive bugs that are hard to find without panicking earlier. but for CONFIG GET or INFO, there's no need for that (panic causes data loss).
i'm unsure.
|
@yossigo i thought that letting the user provide overlapping bits is a feature. i think the current code will handle it somewhat correctly. |
|
we discussed this in a core-team meeting, and agreed to proceed. what i'm gonna do next is remove the the module API code i added to this PR, and merge it. |
… spaces for MULTI_ARG configs parsing Currently redis-server looks for arguments that start with `--`, and anything in between them is considered arguments for the config. like: `src/redis-server --shutdown-on-sigint nosave force now --port 6380` MULTI_ARG configs behave differently for CONFIG command, vs the command line argument for redis-server. i.e. CONFIG command takes one bulk string with spaces in it, while the command line takes an argv array with multiple values. In this PR, in config.c, if `argc > 1` we can take them as is, and if the config is a `MULTI_ARG` and `argc == 1`, we will split it by spaces. So both of these will be the same: ``` redis-server --shutdown-on-sigint nosave force now --port 6380 redis-server --shutdown-on-sigint "nosave force now" --port 6380 ``` And for MULTI_ARG, now we will check that it has at least one arg, we can return `wrong number of arguments` before `config->interface.set`. ``` old: src/redis-server --shutdown-on-sigint >>> 'shutdown-on-sigint' argument(s) must be one of the following: default, save, nosave, now, force new: src/redis-server --shutdown-on-sigint >>> 'shutdown-on-sigint' wrong number of arguments ``` This is mentioned in redis#10594 (comment) Co-authored-by: Oran Agra <[email protected]>
The SHUTDOWN command has various flags to change it's default behavior, but in some cases establishing a connection to redis is complicated and it's easier for the management software to use signals. however, so far the signals could only trigger the default shutdown behavior. Here we introduce the option to control shutdown arguments for SIGTERM and SIGINT. New config options: `shutdown-on-sigint [nosave | save] [now] [force]` `shutdown-on-sigterm [nosave | save] [now] [force]` Implementation: Support MULTI_ARG_CONFIG on createEnumConfig to support multiple enums to be applied as bit flags. Co-authored-by: Oran Agra <[email protected]>
The SHUTDOWN command has various flags to change it's default behavior,
but in some cases establishing a connection to redis is complicated and it's easier
for the management software to use signals. however, so far the signals could only
trigger the default shutdown behavior.
Here we introduce the option to control shutdown arguments for SIGTERM and SIGINT.
New config options:
shutdown-on-sigint [nosave | save] [now] [force]shutdown-on-sigterm [nosave | save] [now] [force]Implementation:
Support MULTI_ARG_CONFIG on createEnumConfig to support multiple enums to be applied as bit flags.
background
As discussed on #10525
On replicas using RDB, it can be acceptable to not save the latest data on shutdown, especially if we know it will never become a master.
Sometimes the periodic SAVE in background is enough and the blocking SAVE on shutdown undesired.
Also, if guaranteed durability is desired AOF could be used instead.