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

Factory to populate Config object from well-known locations #16386

Closed
wants to merge 7 commits into from

Conversation

jerrinot
Copy link
Contributor

@jerrinot jerrinot commented Jan 1, 2020

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

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
@jerrinot
Copy link
Contributor Author

jerrinot commented Jan 1, 2020

I realize it's VERY late in the dev cycle, so this is more for a consideration. I wonder what does Zoltan think about this change.

*
* @return Config created from a file when exists, otherwise default.
*/
public static Config load() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not a great name

Copy link
Contributor

Choose a reason for hiding this comment

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

I have no problem with the name. It explains that the config is loaded and this implies some files being checked. Unless you can come up with a better name, I would leave it as it is.

@jerrinot
Copy link
Contributor Author

jerrinot commented Jan 1, 2020

run-lab-run

@jerrinot
Copy link
Contributor Author

jerrinot commented Jan 2, 2020

Additional reasoning behind this change (copy and pasted from a conversation with @mmedenjak)

A runtime container typically wants to either:

  1. start an instance by using programmatic configuration:
    new Config().setFoo().setBar().setManagedContext()
  2. start new instance with a specific configuration file.
  3. start a new instance as if it was started by Hazelcast.newHazelcastInstance(), but amend the Config with an instance of ManagedContext

#1 works without any problem, obviously.
#2 works OK-ish. You have to create either a YamlConfigBuilder or XmlConfigBuilder, Construct the Config object and then set the ManagedContext. Ideally I think we should hide the builder objects from users: Just passing a path to configuration file to a factory method on Config should be enough. The Config object can detect what file is that and create the Xml/Yaml Builder behind the scene. Maybe there is a reason to have the builders exposed. Perhaps constructing from InputStream with a config? I am not too sure.
#3 is currently complicated. You need to copy&paste the config file location strategy from Hazelcast to the container, create the instance of Config and then inject the ManagedContext. + if we change the strategy then the container code has to be changed as well. for a consistent experience.

@mmedenjak
Copy link
Contributor

Does this fix #16400?

@jerrinot
Copy link
Contributor Author

jerrinot commented Jan 6, 2020

@mmedenjak it does not.

@jerrinot
Copy link
Contributor Author

jerrinot commented Jan 6, 2020

run-lab-run

@blazember
Copy link
Contributor

The builders are public for the reason you mentioned: it's a flexible API letting the users to build the config from any source, mostly through the InputStream constructor.

Exposing the locator logic on a public API is a good step I believe. Since we have the convenient Config classes (FileSystemXmlConfig & co), I'd move it to something like DeclarativeConfig instead of adding it to Config directly. It could be used this way: new DeclarativeConfig().setManagedContext();.

As an additional thought, since we have convenient Config classes, it might make sense to create one for InputStreams too, so that we can make the builders private.

WDYT?

if (xmlConfigLocator.locateFromSystemProperty()) {
// 1. Try loading XML config from the configuration provided in system property
config = new XmlConfigBuilder(xmlConfigLocator).build();

Copy link
Contributor

Choose a reason for hiding this comment

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

Are these empty lines required?

* Populate Hazelcast <code>Config</code> object from an external configuration file.
* It tries to load Hazelcast configuration from a list of well-known locations.
* When no location contains Hazelcast configuration then it returns default.
*
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a note that the same mechanism is used when you call Hazelcast.newHazelcastInstance() ?

@pveentjer
Copy link
Contributor

I like this API. It is simple from a user perspective. It doesn't expose anything that shouldn't be exposed.

So I give my thumbs up.

image

I would like to see a note that calling

HazelcastInstance hz = Hazelcast.newHazelcastInstance()

Is exactly the same as:

Config config = Config.load();
HazelcastInstance hz = Hazelcast.newHazelcastInstance(config);

@devOpsHazelcast
Copy link
Collaborator

devOpsHazelcast commented Apr 5, 2020

CLA assistant check
All committers have signed the CLA.

@hazelcast hazelcast deleted a comment from Holmistr Apr 6, 2020
@leszko leszko self-assigned this Apr 7, 2020
@leszko
Copy link

leszko commented Apr 8, 2020

Closing this one. I'll open a new PR with this in a moment.

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.

None yet

6 participants