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.

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

Dynamic updates and hot reloading of config value changes in config sources #9

Closed
TFaga opened this issue Oct 27, 2017 · 32 comments
Closed
Milestone

Comments

@TFaga
Copy link

TFaga commented Oct 27, 2017

Often configuration values are stored and retrieved from an external remote resource, server or service in order to better facilitate a central cohesive management of your environment. The result of this is that configuration values for a particular application/service are often changed on the external service.

Right now if an application uses the Configuration API it will not know if or when a particular configuration value was changed or updated. The change would eventually get detected the next time a particular configuration value was queried through the API (if it's not cached). However for values that are typically only read at startup, they may never get updated.

On top of updating the values when queried, we should have a relatively lightweight API to enable ConfigSources to notify the application using the Configuration API of any (or selected) config value changes that occur within the ConfigSource. The notification could be a simple CDI event we define or we can enable applications to register a listener in non CDI environments.

How updates are detected would be left to the ConfigSource implementation (polling, watches on files, ...).

The same API or functionality could be used to notify the application of updates when a config file changes, as discussed in #8 .

@Emily-Jiang
Copy link
Member

Can you add your comments on #8, so that we can just discuss on one thread?

@TFaga
Copy link
Author

TFaga commented Oct 29, 2017

No problem. Added.

@atsticks
Copy link
Contributor

This is more complex as suggested. Letting a ConfigSource triggering an event directly for me makes no sense, because a ConfigSource does not know it a change of a value has any effect on the overall configuration, where a ConfigSource is included. We might think of a mechanism, where a ConfigSource can trigger a ChangeEvent for itself, so a Configuration can listen to the event and react as needed. Though using an SE approach there is nothing such as an event. So this scenario should be discussed as well.

@struberg
Copy link
Contributor

Anatole, that's almost word for word what I wrote ;)

The point is that the ConfigSource must notify the Config itself. The Config then needs to re-evaluate the whole chain and compare it with the previous value. If changed -> send event to the user code.

We might think of a mechanism, where a ConfigSource can trigger a ChangeEvent for itself,
so a Configuration can listen to the event and react as needed.
Though using an SE approach there is nothing such as an event.

Of course there are ways in SE to do it. See my code example in #8.

/**
 * This callback should get invoked if an attribute change got detected
 */
default void setOnAttributeChange(Consumer<Set<String>> reportAttributeChange) {
     // do nothing by default. Just for compat with older ConfigSources.
}

After a ConfigSource gets picked up this method gets called by the Config, providing a callback function. And whenever a ConfigSource detects an attribute change it needs to invoke this very method.

@TFaga
Copy link
Author

TFaga commented Oct 31, 2017

Yes agree. A ConfigSource itself should not be able to notify the application itself, which should be the responsibility of the main Config.

As was already mentioned and suggested in #8 such a mechanism could be made of two parts or APIs. The internal part or API, which the ConfigSources could use to notify the main Config implementation and the public user/application part or API, which the main Config could use to deliver the final notification. This way each ConfigSource is responsible for watching its config values and notifying the main Config, while the latter would be responsible for delivering the notification to the end user/application (or not) depending on whether its relevant or not. Higher order ConfigSources may override the value that was detected to change.

@struberg
Copy link
Contributor

+1 @TFaga. When re-reading @atsticks mail I think he initially didn't fully get what I tried to express. But I think we are on the right track, given that he also ends up with basically the same approach - at least if I understood his mail correctly ;)

@kazumura
Copy link

I would like to know a real use case.

There are roughly three patterns:

(1) I can use the old value, even if the config value is changed.
(2) I want to use always a new value. But I don't care when it was changed.
(3) I want to use a new value immediately after the config value is changed.
This is very emergency. I don't want to miss a chance.

I can imagine (1) and (2), but not (3).

If there is so emergency case, you can use other mechanism such as messaging.
Why does Conig API need so emergency ?

I do not insist to consider about (3), but just want to know the use case.

@hendrikebbers
Copy link
Contributor

@kazumura some use cases:

  • You have a Websocket based endpoint on the server and want to integrate a feature toggle. If the feature toggle is active or deactivated should be defined in config. When the flag changes something should be send to a connected client.

  • In a Java based client you want to change the font size. The font size is defined in the config. Once the config (like a property file) is changed the font size on the screen should change.

@atsticks
Copy link
Contributor

atsticks commented Oct 31, 2017 via email

@atsticks
Copy link
Contributor

atsticks commented Oct 31, 2017 via email

@kazumura
Copy link

@atsticks, @hendrikebbers
Thank you for use cases.
I understand these might be the cases.
But my question is using config is a best practice or not to achieve these features.
In case of the font, the usual steps are as follows:
(a) you interact with application to change the font
(b) application write a new value to the config
(c) application fire a paint event
(d) application receive the event, re-read the config, and re-paint.

If Config has the dynamic notification feature,
steps become as follows:
(a) you interact with application to change the font
(b) application write a new value to the config
(c) application are notified by Config, and application fire a paint event
(e) application receive the event, re-read the config, and re-paint.

Is the latter a better way?

@struberg
Copy link
Contributor

struberg commented Oct 31, 2017 via email

@hendrikebbers
Copy link
Contributor

hendrikebbers commented Oct 31, 2017 via email

@Emily-Jiang
Copy link
Member

Trying to catch up with the discussion. I think @kazumura listed the three situations:
(1) I can use the old value, even if the config value is changed.
(2) I want to use always a new value. But I don't care when it was changed.
(3) I want to use a new value immediately after the config value is changed.

for (3) basically not much waiting time is needed. In this case, reduce cache time might do the trick.
I can even think another scenario;
(4) I want to use the old value if the new value is not what I want. Basically you can choose what new value you want.
In this use case, maybe the configured value is not valid etc. I can continue functioning with the old value until a better value is provided.

I have been thinking about the event notification solution.

  1. We need to compute the config and then cache it.
  2. For a pre-configured time period, go back to check the config sources again
  3. calculate the value again.
  4. Compare this value with old value and then fire events.

Let's not prebake any solution as yet. We might come up with a simple solution.

@atsticks
Copy link
Contributor

atsticks commented Oct 31, 2017 via email

@lilian-benoit
Copy link

Sorry but for me toggle feature and background aren't good use case.

  • Toggle feature is more complex than active / deactivate a feature.
  • Background is more concept of preference that configuration.

I prefer Mark's use case with changing port.

Also, this use case show that event should fire when config changed.

Any more, what happens if we have two modified properties? for example hostname and port number.
We have two events ?

@Emily-Jiang
Copy link
Member

Each property will trigger a event. The event body should be the property name and value.

@TFaga
Copy link
Author

TFaga commented Oct 31, 2017

+1 for aiming towards a simple solution.

Notifications would mostly be used when the end user/application would need to explicitly react to a config value change. I've had a case where multiple microservices logger configuration was stored on an external service (consul in my case, which natively supports watches, so we didn't want to do custom polling). And we needed to trigger logger reconfiguration in the code as part of the config updating. These kind of use cases, however, are usually not so common, so if we add watches/notifications they should probably be on an opt-in basis per value, as well as try to keep them simple from the API perspective.

Other config value changes would most likely be sufficiently resolved the next time the application queries the config value as was already mentioned. A configurable cache policy would help to fine tune the rate of refreshes.

jmesnil pushed a commit to jmesnil/ConfigJSR that referenced this issue Nov 3, 2017
@jmesnil
Copy link
Contributor

jmesnil commented Nov 21, 2017

Is there an actual use case for (3)?

If I want to use the value immediately after it is changed, I'd better not cache it at all and call Config.getValue() every time I need to know its value.

@jmesnil
Copy link
Contributor

jmesnil commented Nov 21, 2017

I am not a CDI expert so sorry in advance if I'm proposing something stupid but isn't the CDI API we have enough for application to be notified of config value changes?
If I annotate a Consumer<T> function with a @ConfigProperty parameter, couldn't the implementation calls the consumer whenever the value is changed? For example:

 void myFunction(@ConfigProperty(name = "foo", defaultValue = "bar") String foo) {
  // called when the value of the "foo" property is changed
 }

If I want to get an initial value when my app is deployed and be notified of any change, I just need to annotate a property and a parameter:

// injected at deployment time, provide an initial value
@Inject
@ConfigProperty(name = "foo", defaultValue = "bar")
String foo;

// called whenver the foo property is changed
void observeFoo(@ConfigProperty(name = "foo") String foo) {
}

That's for the CDI use case.

For the plain Config API, we coud add a getValue(String, Consumer<T>) that is called whenever the config detects a change in the configuration value. It would then be used like:

config.getValue("foo", (Consumer<String>) prop -> {
   System.out.println("prop = " + prop);
 });

@struberg
Copy link
Contributor

we will need a CDI observer plus a SE way to register an Observer.

@jmesnil
Copy link
Contributor

jmesnil commented Nov 21, 2017

@struberg right. my comment was focusing on the Config API that the user is facing.

We will need to have observers for the SPI so that ConfigSource can notify the Config of some events but I don't see the need to include an Observer pattern in the user API.

@struberg
Copy link
Contributor

@jmesnil was talking about the user facing Config part as well.

struberg added a commit to struberg/ConfigJSR-1 that referenced this issue Nov 22, 2017
…anges

This is just the basic approach for now
@Emily-Jiang Emily-Jiang added this to the 1.0 milestone Dec 7, 2017
struberg added a commit to struberg/ConfigJSR-1 that referenced this issue Dec 28, 2017
…n API

work in progress, API looks solid though

cherry picked over from original commit: 4c282bd
struberg added a commit to struberg/ConfigJSR-1 that referenced this issue Dec 28, 2017
struberg added a commit to struberg/ConfigJSR-1 that referenced this issue Dec 28, 2017
@atsticks
Copy link
Contributor

@struberg Regarding useConverter, my point was adding something like the following to the Config interface:
<T> T getValue(String key, Converter<T> converter);
<T> Optional<T> getOptionalValue(String key, Converter<T> converter);

Despite adding another method to the API, why this should be an issue?

@atsticks
Copy link
Contributor

From my original point the following I think is the most important:

I think it would be more logical to define the lookup chain, when accessing the ConfigValue from the Config, instead of creating another ConfigValue with a different lookup chain from a ConfigValue.

@atsticks
Copy link
Contributor

+1 for adding a latest changed timestamp to the Config interface. It would make it easy to detect changes from (ConfigValue, Config) instance implementations, by simply polling and checking for value changes. When a change has been detected, the values can be read and listeners be informed.

struberg added a commit to struberg/ConfigJSR-1 that referenced this issue May 3, 2018
We will probably introduce another way to get notified later on.
struberg added a commit to struberg/ConfigJSR-1 that referenced this issue May 3, 2018
This code got originally developed on 2018-04-04 in Apache DeltaSpike by
Mark Struberg, Manfred Huber, Alex Falb, Gerhard Petracek and Romain Manni-Bucau
based on the problem analysis discussions between Mark Struberg and
Tomas Langer (tomas-langer) who also did reviews and gave important feedback.
struberg added a commit to struberg/ConfigJSR-1 that referenced this issue May 3, 2018
struberg added a commit to struberg/ConfigJSR-1 that referenced this issue May 3, 2018
struberg added a commit that referenced this issue May 8, 2018
struberg added a commit that referenced this issue May 8, 2018
struberg added a commit that referenced this issue May 8, 2018
struberg added a commit to struberg/ConfigJSR-1 that referenced this issue May 8, 2018
struberg added a commit to struberg/ConfigJSR-1 that referenced this issue May 11, 2018
struberg added a commit to struberg/ConfigJSR-1 that referenced this issue May 11, 2018
struberg added a commit to struberg/ConfigJSR-1 that referenced this issue May 11, 2018
struberg added a commit to struberg/ConfigJSR-1 that referenced this issue May 11, 2018
struberg added a commit to struberg/ConfigJSR-1 that referenced this issue May 17, 2018
struberg added a commit to struberg/ConfigJSR-1 that referenced this issue May 17, 2018
struberg added a commit to struberg/ConfigJSR-1 that referenced this issue May 17, 2018
as discussed in the EG meeting

Signed-off-by: Mark Struberg <[email protected]>
struberg added a commit to struberg/ConfigJSR-1 that referenced this issue May 17, 2018
struberg added a commit to struberg/ConfigJSR-1 that referenced this issue May 17, 2018
Emily and Tomas will come up with a better way to test this behaviour
struberg added a commit to struberg/ConfigJSR-1 that referenced this issue May 17, 2018
@struberg
Copy link
Contributor

I think we can consider this as closed, right?

Smallish improvements as separate tickets please!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants