Configuration should be immutable

[radekg]

Expected behavior

Configuration should be immutable. Being able to update the configuration of the Pulsar instance by using provided set methods is not safe for the program. It also isn't consistent. Some changes have an effect, some do not.

Actual behavior

The current implementation is mutable.

System configuration

Pulsar version: 1.14

I would like to suggest Typesafe Config library: https://github.com/typesafehub/config.
It's written in Java, supports simple values, as well as lists (no sets), env variable expansion and more.

There would be a number of changes required in unit tests. Some of the unit tests mutate a single configuration instance. Preferably, all tests should run on a clean environment.

I have done some preliminary work on this but I'd need merge master into it and make sure all tests pass.

Generally, if Typesafe Config is an acceptable option, I'd be happy to take this one.

[merlimat]

Typesafe config looks like a very nice written library.

For broker config, I'd be ok to use it to populate the ServiceConfiguration. I'd like to maintain to have a single point to have to update when adding a new config variable. Currently, ServiceConfiguration is a pojo, and you just need to add a new variable with default value and example in conf/broker.conf.

Alternatively, what about having ServiceConfiguration with only getters and adding a ServiceConfigurationBuilder?

[radekg]

Pushed the branch to remote but no pr, just so you can have a look. The most interesting points:

• ServiceConfiguration wraps and instance of Typesafe Config: https://github.com/radekg/pulsar/blob/7f58a69a2b51c1b3dfca7e03438e9cd1ec5a3afe/pulsar-broker-common/src/main/java/com/yahoo/pulsar/broker/ServiceConfiguration.java
• pulsar-broker-common/src/main/java/com/yahoo/pulsar/broker/ServiceConfigurationLoader.java is gone, not required
• config takes the following form: https://github.com/radekg/pulsar/blob/7f58a69a2b51c1b3dfca7e03438e9cd1ec5a3afe/pulsar-broker-common/src/test/resources/broker.conf
• config init: https://github.com/yahoo/pulsar/compare/master...radekg:immutable-config?expand=1#diff-b9da9463a373b0b19d516884fb70c189R33
• an example of setting up configuration for a test: https://github.com/yahoo/pulsar/compare/master...radekg:immutable-config?expand=1#diff-0a160247a547f35234439402e845f463R83 (it can be set up from the resource, this is what the final PR would be)
• overriding config from a base test class: https://github.com/yahoo/pulsar/compare/master...radekg:immutable-config?expand=1#diff-00841fa6f00fe82dab598394fc501144R107

This isn't a finished PR, tests need much more work. Also, the validation of minValue and maxValue isn't implemented but would be rather straightforward to implement.

To comply with what Typesafe Config needs, we would have to add reference.conf file to the resources.

Have a look, let me know what you think.

[merlimat]

For config file, would it be possible to maintain the same java properties format? That would keep it consistent with the other config files (client, bookkeeper, ..). It seems the only change is right now the pulsar {....} enclosing namespace.

I think that having to specify the config var name (either a string or a constant), makes a it bit more complicated to configure programmatically, since you have to lookup for the list of allowed variables, while with setters it's easy to discover all the options.

[merlimat]

But other than the 2 notes above, it seems nice to combine multiple config sources

[radekg]

@merlimat I'd suggest using the hocon format across all configuration. The reason is that if someone builds software where Pulsar is only one of the components using Typesafe Config, they can single hocon file for all their components. We can keep the properties look and feel but, at least, I'd suggest prefixing all settings with pulsar. to give them a namespace.

As for having to know the property names, yes, this is slightly inconvenient but, I think, you are looking from the perspective of unit tests. Preferably, all configuration variants would come as a separate .conf file. Combined with a reference.conf, everything is covered. However, if that is not enough, we can always implement a builder which would construct the necessary properties. The base is what it is, how config properties are constructed, we can change that to a friendlier looking version.

Shall I come up with a full PR?

[merlimat]

I would rather not break the config file compatibility unless is really necessary. Also, the other concern is that bookkeeper.conf and zookeeper.conf will still be in Java properties format and that cannot be easily changed.

[ivankelly]

This seems to have gone quiet. Closing. Please reopen if you want this work to continue.