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

Add public API to load configuration files #16864

Merged
merged 10 commits into from
Apr 17, 2020

Conversation

leszko
Copy link

@leszko leszko commented Apr 8, 2020

Add load() method to Config, ClientConfig, and ClientFailoverConfig. This method loads the config with the "known" locations. If not found, the default config is returned.

fix #16809

Related to spring-projects/spring-boot#19487

This is the completion of the initial work done by @jerrinot and opened as a draft PR: #16386, I addressed all the comments from there.

jerrinot and others added 7 commits January 1, 2020 13:58
It allows to load config from an external file and then customize
it programmatically. This is useful when embedding Hazelcast into
managed environments: When a runtime container wants to create an
instance as if it was created via `Hazelcast.newHazelcastInstance()`,
but it wants to inject an instance of `ManagedContext` into Hazelcast
configuration.

Without this capability all containers have to duplicate the `Config`
location strategy: "First try this, then that, etc."

Related to spring-projects/spring-boot#19487

TODO: Tests
…experiments/populate-config-from-default
@leszko leszko requested a review from a team as a code owner April 8, 2020 13:27
@mmedenjak mmedenjak added this to the 4.1 milestone Apr 9, 2020
Copy link
Contributor

@blazember blazember left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the change 👍

One improvement idea. Since this logic has become a public API now, perhaps it would make sense going forward to let the user control what happens if no config is found. Unsuccessful load attempts might indicate a deployment/environment issue that one might want to fail fast if it happens. So something like Config#load(MissingConfigFileBehavior) and

enum MissingConfigFileBehavior {
  LOAD_DEFAULT_CONFIG,
  THROW_EXCEPTION
}

with

Config#load() {
  load(LOAD_DEFAULT_CONFIG);
}

WDYT?

@leszko
Copy link
Author

leszko commented Apr 15, 2020

Thanks @blazember . I'll actually let you and the Core team decide on MissingConfigFileBehavior. You probably know better if people missed such a feature. If unsure, we can always add MissingConfigFileBehavior later.

@@ -168,6 +172,45 @@ public ClientConfig(ClientConfig config) {
metricsConfig = new ClientMetricsConfig(config.metricsConfig);
Copy link
Contributor

@pveentjer pveentjer Apr 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we log which file got loaded? So that at least from the logging it is obvious which file is being used. It can be extremely annoying if you are fighting an configuration issue and then you finally determine that your file didn't get loaded in the first place.

I had that problem a few times with simulator and every time I wanted to smack my head into my desk.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we do log it. At least I see something like this in the logs:

INFO: Loading configuration '/opt/hazelcast/hazelcast.yaml' from System property 'hazelcast.config'

@pveentjer
Copy link
Contributor

pveentjer commented Apr 16, 2020

I like the change +1

One improvement idea. Since this logic has become a public API now, perhaps it would make sense going forward to let the user control what happens if no config is found. Unsuccessful load attempts might indicate a deployment/environment issue that one might want to fail fast if it happens. So something like Config#load(MissingConfigFileBehavior) and

enum MissingConfigFileBehavior {
  LOAD_DEFAULT_CONFIG,
  THROW_EXCEPTION
}

with

Config#load() {
  load(LOAD_DEFAULT_CONFIG);
}

WDYT?

Perhaps it is simplest to return null or an exception instead of adding a strategy? KISS

null can be checked and exception can be caught; especially when properly documented.

@leszko
Copy link
Author

leszko commented Apr 17, 2020

verify

@leszko leszko merged commit 9c7c62f into master Apr 17, 2020
@leszko leszko deleted the experiments/populate-config-from-default branch April 17, 2020 09:13
leszko pushed a commit to leszko/hazelcast that referenced this pull request Apr 17, 2020
leszko pushed a commit to leszko/hazelcast that referenced this pull request Apr 17, 2020
leszko pushed a commit to leszko/hazelcast that referenced this pull request Apr 17, 2020
leszko pushed a commit to leszko/hazelcast that referenced this pull request Apr 17, 2020
leszko pushed a commit to leszko/hazelcast that referenced this pull request Apr 17, 2020
mmedenjak pushed a commit that referenced this pull request Apr 18, 2020
leszko pushed a commit to leszko/hazelcast that referenced this pull request Apr 20, 2020
@leszko leszko mentioned this pull request Apr 20, 2020
leszko pushed a commit that referenced this pull request Apr 20, 2020
@mmedenjak mmedenjak removed their request for review June 26, 2020 11:32
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.

Provide a public API to create a Config/ClientConfig from URL with default fallback
6 participants