don't throw some options on config rewrite

[Dieken]

Those options will be thrown without this patch:
include, rename-command, min-slaves-to-write, min-slaves-max-lag,
appendfilename.

I found Redis throws "appendfilename" on config rewrite during playing Redis sentinel,
then I write a script to figure out other thrown options:

$ cat ~/check-redis-config.c.sh
perl -ne 'print if /void loadServerConfigFromString/ .. /^}/' config.c | perl -lne 'print $1 if /^ {8}\S.*strcasecmp.*,\s*"([^"]+)/' | sort > loadServerConfigFromString.txt

perl -ne 'print if /void configSetCommand/ .. /^}/' config.c | perl -lne 'print $1 if /^ {4}\S.*strcasecmp.*,\s*"([^"]+)/'  | sort > configSetCommand.txt

perl -ne 'print if /void configGetCommand/ .. /^}/' config.c | perl -lne 'print $1 if /^ {4}(?:\S*config_get_\w+\(|\S.*stringmatch[^"]+)\s*"([^"]+)/' | sort > configGetCommand.txt

{
perl -ne 'print if /int rewriteConfig\(/ .. /^}/' config.c | perl -lne 'print $1 if /^ {4}\S*rewriteConfig[^"]+"([^"]+)/'
perl -ne 'print if /int rewriteConfig\(/ .. /^}/' config.c | perl -lne 'print lc($1) if !/"/ && /^ {4}(?:\S.* )?rewriteConfig(\S+)Option/'
} | sort > rewriteConfig.txt

[Dieken]

Oh, bad markdown rendering... but you still can select by dragging and copy the script snippet correctly.

I don't fix missing options for "CONFIG GET" command, that's not urgent to me. And IMHO, the config.c
file requires refactoring, it's hard to maintain options in four places: load/set/get/rewrite.

[Dieken]

@antirez , any comment?

[antirez]

Thank you @Dieken, this sounds like a great idea, and thanks for the smart grepping of not-covered options.

I'm reviewing the code right now in order to merge.

[antirez]

Btw, the original code that I wrote is not working as I expected, don't know why but I imagined that the implementation took care of not touching the options it did not understand. This is important as one thing is not rewriting unknown options and another thing is to remove them entirely ;-) So I'm reading your message as a bug report as well...

[antirez]

Hi again @Dieken, I merged your patch and modified the code in order to never touch configuration directive it does not understand. I also specified in the example redis.conf that if users want to use includes in order to override stuff, it is better to put includes at the end.

Now I would like your feedback about this: my feeling is that for this work to be complete, we need every option to be able to override the past options.

For example save works chaining new save points. I would like to be able to reset the chain with:

save ""

Similarly bind binds only selected addresses, but it should be possible to override that later with:

bind *

And so forth. The same for:

salveof no one

That should be able to override previous slaveof commands. Does this sound sane to you as well? Thanks.

[Dieken]

@antirez, thanks for your merging!

Yes, to make config rewrite work, all options must be able to override past values. save/bind/rename-command/include are special case because they can have multiple values, they need a syntax to clear all previous values and then accumulate new values, I suggest to use consistent COMMAND "" to reset to default value: no save point, any bind address, no rename-command, no includes(maybe forbidden rewriting rename-command and include now, I'm not sure).

For those single value option, it's enough to just write default value (currently default value isn't written out).

And I suggest to separate rewritten options into separate file, for example, use a "rewritten-config path" in redis.conf to tell redis where to store rewritten options, this has two benefits:

• somebody uses puppet or some other tool to manage redis.conf, self-modification will mess it, I already saw a guy in redis mailing list complaining it.
• greatly simplify the logic in src/config.c, redis can dump all current config without comment into a separate file, totally file overwriting. Actually current src/config.c totally breaks the correspondence between comment and multiple value options such as "sentinel" because redis doesn't care the second word following "sentinel".
  • With the overriding behavior, users can set default values before "rewritten-config path" and non-rewritten-able values after "rewritten-config path". // not accurate, it still can be rewritten, just restore after restart.

[Dieken]

The latest code can't correctly rewrite "save" option:

$ ./src/redis-cli config get save

1. "save"
2. "900 1 300 10 60 10000"

$ ./src/redis-cli config set save ""
OK

$ ./src/redis-cli config rewrite
OK

$ grep ^save redis.conf
save 900 1
save 300 10
save 60 10000

$ ./src/redis-cli config get save

1. "save"
2. ""

In-place rewriting is so tricky, easier to just dump all options into separate file:-)

[Dieken]

Not directly related to the bug above, just an advice, it's better to refactor src/config.c to have a data structure like this:

struct option {
     char* name;
     void (*load)(....);
     void  (*set)(...);
     void (*get)(...);
     void (*rewrite)(...);
};

then declare an array of option structure, by this way it's easier to maintain four code paths and don't have to worry "unknown option" for a code path, usually "unknown option" means something it doesn't consider carefully and may hide a bug.

[antirez]

@Dieken thanks! Good catch, fixed. About the table you proposed, this was planned since... a lot of time, and it is a good approach IMHO. Fields may be set to NULL when something is not provided, or to generic methods implementing what is good for 80% of options. So this would definitely simplify the code.

Soon or later (hopefully sooner) will find the time to do it. Perhaps instead of just a fixed structure, we could have an hash table, so that there is no need to specify everything in a pedantic way, but higher level methods could set the right stuff, like:

addConfigOption("timeout",REDIS_OPTION_INTEGER);
addConfigOptionBoundChecks("timeout",0,3600);

And so forth. You end with a DSL that allows, for most things, to just specify the "type" of the option, and to refine over it. For special options you could use addConfigOptionMethod(... method name ..., function pointer) in order to handle some option with an ad-hoc function.

Thanks again for your fixes and interesting inputs!

Salvatore

[antirez]

p.s. sorry the fix is still not pushed, testing it and pushing it in a few.

[Dieken]

Agree, your design is more flexible.

And I tested the new patch, increase save point, decrease save point, clear save points, all work, thanks!