Currently CONFIG SET operates on a single configuration parameter at a time, which works well because most configuration parameters are independent. In other cases parameters do relate to each other but there's little or no harm in performing two discrete CONFIG SET operations (e.g. replica-announce-ip and replica-announce-port).

In more rare cases, parameters are both inter-dependent and modifying them involves a validation step that would fail if they're not modified atomically. For example, tls-cert-file and tls-key-file generally need to be changed together and validation will fail if they are not.

The proposed way to solve this is to extend CONFIG SET to support an arbitrary number of keyword/value pairs. In most cases processing will be the same, but this will enable special handling of the cases where interdependency exists.

Another alternative is to leverage MULTI/EXEC for this purpose, but this might imply atomicity which we may not want to guarantee.

Comment From: madolson

We also would probably want to be able to pre-validate all of the configs are valid before applying any of them.

Comment From: bjosv

I looked into this a bit, and to pre-validate the configs we could either - split the standardConfig's interface function set(..) into a validate(..) + set(..), and run validate on configs before set - or add a dry-run flag to set(..) to do type- and config specific checks on all given configs before setting them.

One obstacle I have seen is that we still have some configs that requires special handling, and as I see it this makes it a bit harder to do the validation part. One option could be to incorporate these configs info the standardConfig structure by breaking out each configs set- and get-handling to own functions (rewrite is already its own function), and add a new config type to point out each configs setfn, getfn, rewritefn

As a try-out/PoC to better show my intentions I have created this branch The addition to standardConfig is at the end of the diff.

Having a single config structure would help making the multi-parameter solution a bit cleaner I think. Any thoughts or comments if this is something I should continue to pursue? Or do you have other ideas? It seems that we lack some tests that would show if error output changes, and I think that could be a good starting point for this

Comment From: yossigo

@bjosv thanks for looking into this! Had a quick look and it looks good to me so far. Do you see any pros and cons for set + validate vs dry-run?

Once concern I have is if we can really always treat multiple keywords as a single-keyword with multiple arguments. That might break for Sentinel for example, but a lot of the Sentinel config is already pretty awkward and perhaps we should just fork it out.

@madolson Any thoughts about this?

Comment From: madolson

@bjosv I like the start of it. It's been on my list for a long while to try to cleanup up the remaining configs and standardize it more widely. We might want to consider re-thinking how validate/update work to see if we can make some of the "special" configs more normalized.

@yossigo I think validate + set is a much better option than a dry-run if we can make it work. I am standing behind my belief that I dislike sentinel and want to replace it with cluster mode, so I'm pretty indifferent to sentinel being hard to manage.

Comment From: JoaoSerras

any update on this matter?

Comment From: madolson

@JoaoSerras No, unless you ant to help :)

Comment From: yoav-steinberg

With regards to the validation is see a few possible directions here and I'd like some feedback: 1. We can apply each config value one after the other. For each value we store the result and then return a multi-bulk response with all the results. This means we don't guarantee all values will be set, but we do guarantee whatever is applied will be applied atomically. This is by far the easiest and most maintainable solution, and resolves the core issue of setting multiple config values atomically.

  1. We can add a verification stage to the multi-arg config set where we verify each config has a valid value and once we see all are ok we run the set functions on all the values under the assumptions they will succeed. This has a few drawbacks:
  2. Some configs do system calls or other complex stuff that might pass basic validation, like that the sting looks like a valid IP address, but then fail on actual apply.

  3. If we pass basic validation but fail on actual apply, what do we do: do we crash the server under the assumptions that validation should always work? If not how do we report to the user what failed? How to we treat this partially applied state.

    1. Having a validation phase makes the code much more complex. If we take the dry-run approach we need to add a flag to only do validation to all setter functions. If we take the validation-function approach we need to pull validation code out of existing settters and move them into lots of new validation functions. And adding new configs will always be more complex.

  4. The last option I can think of is to run the getters on all sepecified config values and back them up. Then attempt to apply the new settings and if one fails then restore the values of everything up to that failure and report an error. This requires for the restore process to succeed which isn't necessarily guaranteed either. So issue ii above also applies here.

Comment From: oranagra

i think we're looking too much into this. looking at Yossi's top comment, for most configs there's no need for all of this, and this solution comes to make it possible to solve the few that do.

i think we can take option 3 for for most cases (seems easy enough and genetic). but in all that text i still didn't see any plan of how we handle the tls-key-file problem. i.e. if we apply the general configs one by one, we would still call tlsConfigure twice.

a simple way around it could be to let the update callback of the tls configs set some dirty flag, and then apply the configs when the loop ends. in theory maybe we could have done a similar things with MULTI-EXEC (in call()), except for the fact we would do that after already sending the reply.

Comment From: yoav-steinberg

Ok, sorry for not noticing that before. What we can do is fix the code so that: 1. We backup values before calling the setter. 2. we call the setter with update set to 0. 3. If calling the setter fails we restore all backed up values by calling the setter again (with update 0). And return an error. 4. We keep a set unique function pointers to all the update_fn functions for the relevant values. 5. Once all setters are called we call all the update_fn functions to apply them. 6. If a call to the update_fn function fails we restore all backed up values by calling the setters again (with update 0) and then call all the update_fn functions we called up until now to revert the changes. Then we return an error.