Add support for Hazelcast YAML configuration #16632

leszko · 2019-04-24T07:51:51Z

Since Hazlecast 3.12, YAML configuration format is supported in
addition to XML. Therefore, this change makes Spring Boot automatically
discover not only hazelcast.xml (and hazelcast-client.xml), but also
hazelcast.yaml (and hazelcast-client.yaml).

fix #16074

Signed-off-by: Rafal Leszko rafal@hazelcast.com

Since Hazlecast 3.12, YAML configuration format is supported in addition to XML. Therefore, this change makes Spring Boot automatically discover not only `hazelcast.xml` (and `hazelcast-client.xml`), but also `hazelcast.yaml` (and `hazelcast-client.yaml`). Signed-off-by: Rafal Leszko <rafal@hazelcast.com>

snicoll

Thank you very much for opening your first PR. I've added some questions and suggestions. Also wondering if Hazelcast work with .yaml and .yml files. The latter is not taken into account and yet is a valid file extension.

snicoll · 2019-04-24T08:56:13Z

...main/java/org/springframework/boot/autoconfigure/hazelcast/HazelcastClientConfiguration.java

@@ -79,7 +79,8 @@ public HazelcastInstance hazelcastInstance(ClientConfig config) {

 		ConfigAvailableCondition() {
 			super(CONFIG_SYSTEM_PROPERTY, "file:./hazelcast-client.xml",
-					"classpath:/hazelcast-client.xml");
+					"classpath:/hazelcast-client.xml", "file:./hazelcast-client.yaml",


We should probably group them (file and classpath) rather than mixing them. The order in this list is the precedence order so it must match the expectation. Right now, an xml file on the classpath has precedence over a yaml file at the root. That doesn't sound right to me.

That is actually what we do in Hazelcast to be backwards-compatible. Before there was only XML, so it takes the precedence. The exact algorithm for Hazelcast is as follows:

System property: the file located in the hazelcast.config JVM argument, if present. It can be either XML or YAML.

A file named hazelcast.xml located in the application's working directory

A file named hazelcast.xml located on the classpath

A file named hazelcast.yaml located in the application's working directory

A file named hazelcast.yaml located on the classpath

The default configuration hazelcast-default.xml we distribute in our jar files

snicoll · 2019-04-25T07:32:13Z

...e/src/main/java/org/springframework/boot/autoconfigure/hazelcast/HazelcastClientFactory.java

+		if (configFileName.endsWith(".xml")) {
+			return new XmlClientConfigBuilder(configUrl).build();
+		}
+		else { // yaml config available in the classpath


What does this comment mean?

Actually it's useless, removed.

snicoll · 2019-04-25T07:32:54Z

...src/main/java/org/springframework/boot/autoconfigure/hazelcast/HazelcastInstanceFactory.java

-		Config config = new XmlConfigBuilder(configUrl).build();
+		String configFileName = configUrl.getFile();
+
+		Config config;


Rather than having a field initialization, I'd create a private method that returns the Config instance.

snicoll · 2019-04-25T07:33:23Z

...main/java/org/springframework/boot/autoconfigure/hazelcast/HazelcastServerConfiguration.java

@@ -77,7 +77,8 @@ public HazelcastInstance hazelcastInstance(Config config) {

 		ConfigAvailableCondition() {
 			super(CONFIG_SYSTEM_PROPERTY, "file:./hazelcast.xml",
-					"classpath:/hazelcast.xml");
+					"classpath:/hazelcast.xml", "file:./hazelcast.yaml",


Same ordering remark

snicoll · 2019-04-25T07:34:48Z

...t/java/org/springframework/boot/autoconfigure/hazelcast/HazelcastAutoConfigurationTests.java

@@ -39,6 +39,8 @@
 	@Test
 	public void defaultConfigFile() {
 		// no hazelcast-client.xml and hazelcast.xml is present in root classpath
+		// this also asserts that XML has priority over YAML


It seems wrong to me that XML on the classpath has priority over YAML on the file system. Is this really what Hazelcast does?

yeah, as explained in the other comment.

snicoll · 2019-04-25T07:36:03Z

spring-boot-project/spring-boot-autoconfigure/src/test/resources/hazelcast.xml

@@ -5,7 +5,6 @@
 	<map name="defaultCache" />
 	<network>
 		<join>
-			<tcp-ip enabled="false" />


This seems an unrelated change. I am happy to polish the support in a separate commit though. Can you explain why you've removed this line?

Yeah, it's just not used. Reverted. Can be sent in a separate PR.

snicoll · 2019-04-25T07:36:35Z

...e/src/test/resources/org/springframework/boot/autoconfigure/hazelcast/hazelcast-specific.xml

@@ -2,16 +2,13 @@
 		   xmlns="http://www.hazelcast.com/schema/config"
 		   xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">

-	<queue name="foobar"/>


Same remark. Is this related to the main purpose of this PR?

leszko

@snicoll Thanks for the review. I addressed your comments. PTAL.

Concerning the *.yml extension, we don't use it automatically in Hazelcast, so I guess the behaviour of Spring Boot should be the same.

leszko · 2019-04-25T08:57:41Z

...main/java/org/springframework/boot/autoconfigure/hazelcast/HazelcastClientConfiguration.java

@@ -79,7 +79,8 @@ public HazelcastInstance hazelcastInstance(ClientConfig config) {

 		ConfigAvailableCondition() {
 			super(CONFIG_SYSTEM_PROPERTY, "file:./hazelcast-client.xml",
-					"classpath:/hazelcast-client.xml");
+					"classpath:/hazelcast-client.xml", "file:./hazelcast-client.yaml",


That is actually what we do in Hazelcast to be backwards-compatible. Before there was only XML, so it takes the precedence. The exact algorithm for Hazelcast is as follows:

System property: the file located in the hazelcast.config JVM argument, if present. It can be either XML or YAML.

A file named hazelcast.xml located in the application's working directory

A file named hazelcast.xml located on the classpath

A file named hazelcast.yaml located in the application's working directory

A file named hazelcast.yaml located on the classpath

The default configuration hazelcast-default.xml we distribute in our jar files

leszko · 2019-04-25T08:58:19Z

...e/src/main/java/org/springframework/boot/autoconfigure/hazelcast/HazelcastClientFactory.java

+		if (configFileName.endsWith(".xml")) {
+			return new XmlClientConfigBuilder(configUrl).build();
+		}
+		else { // yaml config available in the classpath


Actually it's useless, removed.

leszko · 2019-04-25T09:02:43Z

...src/main/java/org/springframework/boot/autoconfigure/hazelcast/HazelcastInstanceFactory.java

-		Config config = new XmlConfigBuilder(configUrl).build();
+		String configFileName = configUrl.getFile();
+
+		Config config;


leszko · 2019-04-25T09:03:43Z

...t/java/org/springframework/boot/autoconfigure/hazelcast/HazelcastAutoConfigurationTests.java

@@ -39,6 +39,8 @@
 	@Test
 	public void defaultConfigFile() {
 		// no hazelcast-client.xml and hazelcast.xml is present in root classpath
+		// this also asserts that XML has priority over YAML


yeah, as explained in the other comment.

leszko · 2019-04-25T09:04:32Z

spring-boot-project/spring-boot-autoconfigure/src/test/resources/hazelcast.xml

@@ -5,7 +5,6 @@
 	<map name="defaultCache" />
 	<network>
 		<join>
-			<tcp-ip enabled="false" />


Yeah, it's just not used. Reverted. Can be sent in a separate PR.

leszko · 2019-04-25T09:05:49Z

...e/src/test/resources/org/springframework/boot/autoconfigure/hazelcast/hazelcast-specific.xml

@@ -2,16 +2,13 @@
 		   xmlns="http://www.hazelcast.com/schema/config"
 		   xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">

-	<queue name="foobar"/>


Signed-off-by: Rafal Leszko <rafal@hazelcast.com>

Since Hazlecast 3.12, YAML configuration format is supported in addition to XML. Therefore, this change makes Spring Boot automatically discover not only `hazelcast.xml` (and `hazelcast-client.xml`), but also `hazelcast.yaml` (and `hazelcast-client.yaml`). See gh-16632

* pr/16632: Polish "Add support for Hazelcast YAML configuration" Add support for Hazelcast YAML configuration

snicoll · 2019-04-29T13:21:47Z

@leszko thank you for making your first contribution to Spring Boot and following up quickly on feedback. This is now merged in master with a polish commit. What I've changed in particular is the check on xml vs. yaml, with an explicit check on yaml and a fallback on xml rather than the other way around.

Also, tests assertions were all identical so that didn't really test that the right file was loaded. I couldn't fix it for system properties test of the server as I believe I've found an inconsistency in Hazelcast. I've raised hazelcast/hazelcast#14960.

Thanks again.

leszko · 2019-05-20T10:51:39Z

Thanks @snicoll

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 24, 2019

snicoll reviewed Apr 25, 2019

View reviewed changes

snicoll added the status: waiting-for-feedback We need additional information before we can continue label Apr 25, 2019

leszko commented Apr 25, 2019

View reviewed changes

Updates after code review

bd58226

Signed-off-by: Rafal Leszko <rafal@hazelcast.com>

leszko force-pushed the add-hazelcast-yaml branch from 92b1ca4 to bd58226 Compare April 25, 2019 09:33

snicoll self-assigned this Apr 29, 2019

snicoll closed this in 704da17 Apr 29, 2019

snicoll added a commit that referenced this pull request Apr 29, 2019

Merge pull request #16632 from leszko

84530ce

* pr/16632: Polish "Add support for Hazelcast YAML configuration" Add support for Hazelcast YAML configuration

snicoll added type: enhancement A general enhancement and removed status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged labels Apr 29, 2019

snicoll added this to the 2.2.0.M3 milestone Apr 29, 2019

izeye mentioned this pull request May 10, 2019

Support YAML for Hazelcast configuration #16074

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Hazelcast YAML configuration #16632

Add support for Hazelcast YAML configuration #16632

leszko commented Apr 24, 2019

snicoll left a comment

snicoll Apr 24, 2019

leszko Apr 25, 2019

snicoll Apr 25, 2019

leszko Apr 25, 2019

snicoll Apr 25, 2019

leszko Apr 25, 2019

snicoll Apr 25, 2019

snicoll Apr 25, 2019

leszko Apr 25, 2019

snicoll Apr 25, 2019

leszko Apr 25, 2019

snicoll Apr 25, 2019

leszko Apr 25, 2019

leszko left a comment

leszko Apr 25, 2019

leszko Apr 25, 2019

leszko Apr 25, 2019

leszko Apr 25, 2019

leszko Apr 25, 2019

leszko Apr 25, 2019

snicoll commented Apr 29, 2019

leszko commented May 20, 2019

Add support for Hazelcast YAML configuration #16632

Add support for Hazelcast YAML configuration #16632

Conversation

leszko commented Apr 24, 2019

snicoll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leszko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snicoll commented Apr 29, 2019

leszko commented May 20, 2019