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

Simplify ConfigurationSource (remove reload() method) and add caching layer #148

Merged
merged 44 commits into from
Jun 3, 2016

Conversation

norbertpotocki
Copy link
Member

@norbertpotocki norbertpotocki commented Apr 8, 2016

TL;DR:

Summary:
I'm making a backward-incomatible (hence version 5.0.0) change altering the the ConfigurationSource contract. Here's the idea and the reasoning behind it.

So far each configuration source had to guarantee that between two consecutive calls to the reload() method the result of getConfiguration(Environment) call will not change. This assumption had few side effects:

  • it was hard to fetch only a subset of configuration that was represented by the environment passed to the getConfiguration(Environment) since reload() method was not aware of it. For scenarios when the whole configuration set is large and the subset represented by environment was small there was significant and unnecessary overhead during reload(). See isse ConsulConfigurationSource is reading all possible values in Consul #144.
  • each configuration source had to deal with caching configuration between calls to the reload() method
  • it made implementing FallbackConfigurationSource properly tricky

Also it feels like configuration sources shouldn't deal with reloading since they sole responsibility was to to hide the storage-layer details behind a uniform interface.

So, now the change :) The ConfigurationSource interface does not extend the Reloadable interface anymore. Instead I introduced a CachedConfigurationSource that is responsible for caching the results from any other configuration source. It's now being used by default by the ConfigurationProviderBuilder to wrap the source that was passed by a user.

@norbertpotocki norbertpotocki changed the title Make ConfigurationSource.getProperties() invariable between two consecutive calls to reload() Simplify ConfigurationSource (remove reload() method) and add caching layer Jun 1, 2016
@norbertpotocki
Copy link
Member Author

@fromanator, @majkiw - what are your thoughts on this?

@majkiw
Copy link
Contributor

majkiw commented Jun 2, 2016

I definitely like the idea.
I was confused before, not knowing what to expect when calling getConfiguration again.

Are we losing anything important with this change?
For example I can see we're no longer measuring reload times with MeteredConfigurationSource.
Wouldn't it still be useful?
(Nitpicking: you're still initializing unused timer)

Also, now that caching is separated from ConfigurationSource would it be useful to make caching optional by adding a flag to ConfigurationProviderBuilder to skip creating CachedConfigurationSource? (Sorry if it's already possible and I've missed it)

@majkiw
Copy link
Contributor

majkiw commented Jun 2, 2016

Oh, right, that's the ImmediateReloadStrategy - nevermind then :)

@norbertpotocki
Copy link
Member Author

@majkiw You're right, I'm going to add a MeteredReloadable that will compensate for this change.

Making caching optional sounds good since it's hard to model the "always latest" configuration strategy with ReloadStrategies now. I'm going to deal with this (and provide more explanation to other readers) in a separate PR.

@norbertpotocki norbertpotocki merged commit 512ad5c into master Jun 3, 2016
@norbertpotocki norbertpotocki deleted the getConfiguration-contract branch June 3, 2016 17:08
@norbertpotocki norbertpotocki added this to the v.5.0.0 milestone Jun 3, 2016
@norbertpotocki norbertpotocki self-assigned this Jun 3, 2016
@norbertpotocki norbertpotocki mentioned this pull request Jun 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ConfigurationSource.getProperties() should be invariable between two consecutive calls to reload()
3 participants