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.

Sign up for GitHub

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

Jump to bottom

Sentinel Mode: do not add "save" config during config rewrite #7945

Merged
merged 1 commit into from
Oct 22, 2020
Merged

Sentinel Mode: do not add "save" config during config rewrite #7945

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/config.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1335,6 +1335,12 @@ void rewriteConfigSaveOption(struct rewriteConfigState *state) {
int j;
sds line;

/* In Sentinel mode we don't need to rewrite the save parameters */
if (server.sentinel_mode) {
rewriteConfigMarkAsProcessed(state,"save");
return;
}
Comment on lines +1338 to +1342
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@hwware hwware Oct 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@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.


/* Note that if there are no save parameters at all, all the current
* config line with "save" will be detected as orphaned and deleted,
* resulting into no RDB persistence as expected. */
Expand Down