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

Provide a mechanism to detect ConfigValue changes #63

Open
struberg opened this issue May 8, 2018 · 8 comments
Open

Provide a mechanism to detect ConfigValue changes #63

struberg opened this issue May 8, 2018 · 8 comments
Assignees

Comments

@struberg
Copy link
Contributor

struberg commented May 8, 2018

Splitting this out from #9 into a separate ticket as discussed in our last EG meeting.

When people make use of ConfigValue they often want to trigger some action when a new value get's picked up.
Remember that ConfigValue is only needed for configured values which might change during runtime.

Often a user wants to be aware when the underlying value got changed. Either to log it out, or to perform some other code.

This is essentially done by registering a lambda as callback which gets called whenver the underlying config gets accessed an a change gets detected.
https://github.com/struberg/ConfigJSR-1/blob/i9_configValue/api/src/main/java/javax/config/ConfigValue.java#L162

Here is a code examples to highlight why this is an important use case:

@ApplicationScoped
public class FtpService {
  private ConfigValue<String> urlCfg;

  @Inject 
  void setConfig(Config cfg) {
    cfg.access("myapp.ftp.url")
      .cacheFor(1, TimeUnits.DAY)
      .evaluateVariables(true)
      .withLookupChain("${myapp.tenant}")
      .onChange((propertyName, oldV, newV) 
          -> log.info("Switchint to a new FTP URL from {} to {}", oldV, newV));
  }

  public void store(String name, byte[] content) {
    FtpConnection = access(urlCfg.getValue());
  }
}

This is really important for our Ops team. Otherwise they don't really have a clue when the system eventually picked up new values!

The onChange callback might also be used to e.g. clean cached values, etc. It's really very versatile and an important use case!

struberg added a commit that referenced this issue May 8, 2018
…sion

As agreed in the last EG meeting I gonna split out this feature into a separate
ticket (#63).
@rgielen
Copy link

rgielen commented May 9, 2018

This is a useful addition in general. Concise API, easy to implement.

However, the downside as I understand it is that it will only catch changes made from the Config API, but not external changes to ConfigSources, say etcd or a database. So it must be clearly to users that external changes might not be catched.

While not 100% removing the problem, a companying addition might be a mean for a ConfigSource to emit change notifications when the underlying store supports such detection/notification. The problem there is how to propagate such notification, since it might not be part of the effective configuration if a source with higher precedence masks the property - in this case the it would be counter intuitive for the application to receive a change notification, since the effective application hasn't changed.

@keilw
Copy link
Contributor

keilw commented May 10, 2018

If this is a throwaway example don't worry, but otherwise "Switchint..." should be "Switching...".

@struberg
Copy link
Contributor Author

Hi @rgielen! Thanks for your feedback.

Yes, it's not intended to get actively notified asynchronously if anything deep inside the Config changes.
It's really just a callback for the case when you call getValue and some change got detected.
So now you can actively do something, like verifying the scenario, logging out the changes, etc

@OndroMih
Copy link
Contributor

So do I understand correctly that to trigger an event when a value is changed (or soon after), it's necessary to poll config for that value? Something like this:

@Schedule(hour="*", minute="*", second="*") // every second
public void pollMyConfig() {
    ConfigProvider.getConfig().getOptionalValue("my.config", String.class);
}

If yes, then it's cool because it's clear that the listeners are executed in the current thread and no other thread needs to be created.

However, I would consider adding special methods for this to make it more obvious. For example: config.propertyExists(String) (and Javadoc explaining that this also detects changes and triggers onChange event) and also config.propertyExistsAsync(String, Executor) to allow triggering onChange events in a separate executor.

@struberg
Copy link
Contributor Author

struberg commented May 25, 2018

Yes, it's fully synchronous on the same thread. That has benefits and of course the downside that you really only get the callback if the getValue() or getOptionalValue() get invoked.

It's basically internally the following code

T getValue() {
  if (onChange != null && !saveEquals(oldValue, newValue) {
    onChange(key, oldValue, newValue);
  }
  ..
  return newValue;
}

So it's not a full replacement for the Config#registerOnConfigChange we had in earlier versions. This would have been fully 'pro-active', but also would have added quite a few new problems, like getting notified asynchronously on a new thread without any EE set up, etc...

Oh and thanks for the feedback!

@struberg
Copy link
Contributor Author

Oh and I got asked for other use cases. The things I've seen is

  • logging a config value change
  • creating a new Client after the config changed
  • cleaning caches on a config change
  • cleaning statistics on a config change
  • perform an eager sanity check (e.g. if endpoint exists) + fallback logic on a config change.

@struberg
Copy link
Contributor Author

will work on it due to popular demand.

@struberg struberg self-assigned this Jun 28, 2018
@keilw
Copy link
Contributor

keilw commented Jun 28, 2018

Great, any news on the EDR?

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

4 participants