Sentinel Mode: do not add "save" config during config rewrite #7945
Conversation
| /* In Sentinel mode we don't need to rewrite the save parameters */ | ||
| if (server.sentinel_mode) { | ||
| rewriteConfigMarkAsProcessed(state,"save"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
i'm not an expert of the config rewrite (didn't deal with it in the past).
it looks to me that a slightly cleaner way would be to just add the condition on the call to rewriteConfigSaveOption.
i.e. since the original config file isn't expected to have the save config, we don't need to rewriteConfigMarkAsProcessed.
@hwware do you care to test it, or correct me if i'm wrong?
There was a problem hiding this comment.
Hi @oranagra, yes both these two ways will work. the reason i added the check in the function is I was thinking about consistency, since some other functions such as https://github.com/redis/redis/blob/unstable/src/config.c#L1400 the check condition added inside the function. But for sentinel mode the codition is simplier indeed, I agree if we add check in the calling place of rewriteConfigSaveOption may make code looks more consise.
There was a problem hiding this comment.
but just let you know there are also some trivial difference here, if we put the check when we calling the rewriteConfigSaveOption, it won't touch any existing "save" parameters in sentinel configuration, however if we added the check inside the function and if we mistakenly put "save " inside sentinel config, this will be overwritten by blank. In my opinion both way works since we need to make sure the "save" willl not be re-added in the config, if user mistakenly put it in sentinel conf, we can igore it or not ignore it both seems ok to me.. how do you think? @oranagra
There was a problem hiding this comment.
@hwware good point about the fact that there's a precedence that does that, but as you pointed out, it's a slightly different scenario (slaveof does have a purpose there, and might indeed needs to be removed, unlike here which is a pointless line).
If it was about removing or not removing a line that the user might have put there, I would have said not to remove it.
But indeed as you pointed out, this config wasn't put there by the user, but rather by our bug.
So if this were a new config invented by the next version of Redis, I would go with my suggested way (cleaner code), but since we need to clean up junk from old config files we generated, let's take your way.
…is#7945) Previous code would have added default redis save parameters to the config file on rewrite, which would have been silently ignored when the config file is loaded. The new code avoids adding this, and also actively removes these lines If added by a previous config rewrite. (cherry picked from commit 0f370f9)
…is#7945) Previous code would have added default redis save parameters to the config file on rewrite, which would have been silently ignored when the config file is loaded. The new code avoids adding this, and also actively removes these lines If added by a previous config rewrite. (cherry picked from commit 0f370f9)
Previous code would have added default redis save parameters to the config file on rewrite, which would have been silently ignored when the config file is loaded. The new code avoids adding this, and also actively removes these lines If added by a previous config rewrite. (cherry picked from commit 0f370f9)
…is#7945) Previous code would have added default redis save parameters to the config file on rewrite, which would have been silently ignored when the config file is loaded. The new code avoids adding this, and also actively removes these lines If added by a previous config rewrite.
…is#7945) Previous code would have added default redis save parameters to the config file on rewrite, which would have been silently ignored when the config file is loaded. The new code avoids adding this, and also actively removes these lines If added by a previous config rewrite. (cherry picked from commit 0f370f9)
In current sentinel code, if we do sentinel flushconfig, some redundant configuration for RDB save will be added in sentinel conf, for example:
requirepass 1234
sentinel myid 4cc73269c70ebf64cc8fb18a1a37898044c30463
sentinel deny-scripts-reconfig yes
#Generated by CONFIG REWRITE
protected-mode no
port 26379
save 3600 1
save 300 100
save 60 10000
Although this configuration has no effect in Sentinel mode, it may make user confused and make sentinel configuration not clean enough.