Add auto-configuration support for Hazelcast client #7469

vpavic · 2016-11-23T22:24:08Z

As discussed in #4918, this is a WIP effort on auto-configuration support for Hazelcast client.
Most of the parts should be covered by this PR, however the tricky part is auto-config prioritization between server and client modes and I'm not quite sure how to approach that (HazelcastClientAutoConfigurationTests are failing).

Hazelcast itself of course prefers client if both are available (see HazelcastCachingProvider constructor as an example) - this is why CacheAutoConfigurationTests required setting up hazelcast.jcache.provider.type once hazelcast-client was available on the classpath.

/cc @snicoll @neilstevenson

snicoll · 2016-11-24T07:32:05Z

I had a very quick look and my immediate reaction is to have a HazelcastClientAutoConfiguration with @AutoconfigureAfter(HazelcastClientAutoConfiguration.class) on HazelcastAutoConfiguration

This has the advantage of leaving the current auto-configuration untouched and we can add a simple condition to make sure the server-side of things backs off if the client is configured

All things considered, I am wondering if the auto-configuration of a separate HazelcastInstance for the cache is worth the complexity it brings.

Don't worry about having to set the type now for everything. We should move some of these in separate integration tests (CacheAutoConfigurationTests is too big!) that use the new awesome classpath filter. That way we can simulate what happens when only certains jars are present.

Can you rework the first part to separate the client in its own auto-config? Does that make sense?

vpavic · 2016-11-24T07:43:42Z

Thanks for the quick feedback @snicoll.

I had a very quick look and my immediate reaction is to have a HazelcastClientAutoConfiguration with @AutoconfigureAfter(HazelcastClientAutoConfiguration.class) on HazelcastAutoConfiguration

Can you rework the first part to separate the client in its own auto-config? Does that make sense?

It makes sense, and actually it was the approach I've initially taken but the @AutoconfigureAfter hint didn't work for some reason. I've tried the @AutoconfigureBefore in the opposite direction as well. No problem, I'll give it another try and rework the PR.

All things considered, I am wondering if the auto-configuration of a separate HazelcastInstance for the cache is worth the complexity it brings.

Same thoughts here.

snicoll · 2016-11-24T07:44:40Z

It makes sense, and actually it was the approach I've initially taken but the @AutoconfigureAfter hint didn't work for some reason. I

The other thing must be an auto-configuration (with a separate entry in spring.factories). It does not work if it's specified on a "regular" configuration class.

So should we remove that separate auto-configuration in caching?

vpavic · 2016-11-24T08:00:09Z

The other thing must be an auto-configuration (with a separate entry in spring.factories). It does not work if it's specified on a "regular" configuration class.

Yep, I've paid special attention to that part as well.

So should we remove that separate auto-configuration in caching?

Are you referring to org.springframework.boot.autoconfigure.cache.HazelcastInstanceConfiguration? That one completely slipped my mind. It seems there is some overlap with Hazelcast auto-config.

snicoll · 2016-11-24T08:02:00Z

No overlap. It's a crazy idea I had and that I deeply regret now. Basically you can auto-configure Hazelcast and have a separate instance (with a different configuration) for caching purposes only.

vpavic · 2016-11-24T08:21:41Z

It sounds reasonable to remove the support for configuring a separate cache dedicated instance and guide users towards general Hazelcast instance auto-config. It just feels more natural that way, and 2.0 looks like an ideal target for a such change.

I've updated the PR.

vpavic · 2016-11-25T07:56:28Z

@snicoll Is there anything further you need me to provide, given that I've updated the PR yesterday and the label waiting-for-feedback is still on? Thanks.

snicoll · 2016-11-25T08:10:25Z

Nope, thanks.

vpavic · 2017-03-03T12:11:12Z

I've rebased this against current master and sorted out the implementation/tests.

All Hazelcast related tests pass when run individually, however HazelcastAutoConfigurationTests fail when run as a part of full project build, at least for me locally. I suspect this might be related to the fact that HazelcastClientAutoConfigurationTests starts HazelcastInstance to test client-server setup.

EDIT: Scratch that, build passed on TravisCI.

snicoll

It's a very good first step, thank you. But I believe we should simplify the current proposal. Let me know what you think.

snicoll · 2017-03-29T12:20:27Z

...c/main/java/org/springframework/boot/autoconfigure/hazelcast/HazelcastAutoConfiguration.java

 @ConditionalOnClass(HazelcastInstance.class)
 @ConditionalOnMissingBean(HazelcastInstance.class)
 @EnableConfigurationProperties(HazelcastProperties.class)
+@AutoConfigureAfter(HazelcastClientAutoConfiguration.class)


Rather than having two distinct auto-configuration, I'd like that we merge those in the same HazelcastAutoConfiguration with a HazelcastClientConfiguration and a HazelcastServerConfiguration.

It would be nice if we could have a spring.hazelcast.mode enum (HazelcastMode) that could either be CLIENT or SERVER. The default value would be to detect what would work in practice at runtime.

😄 If you recall, that was exactly the situation when PR was initially opened (see your first comment on this PR). I've also been undecided between the two.

I'll try to go back to single HazelcastAutoConfiguration again with the next PR update.

snicoll · 2017-03-29T12:22:52Z

.../java/org/springframework/boot/autoconfigure/hazelcast/HazelcastClientAutoConfiguration.java

+			extends HazelcastClientConfigResourceCondition {
+
+		ClientConfigAvailableCondition() {
+			super("spring.hazelcast", "client-config");


Is there a reason why you'd want to have a dedicated property for this. If you look at how Hazelcast has implemented the JSR-107 support they have a server and client flag (that leads to two CacheProvider implementations and each of them read the same properties as far as I know). I would not deviate from that so, IMO, that should read spring.hazelcast.config.

I guess there shouldn't be a reason we couldn't reuse the existing property. Initially I went with simplest approach when putting this PR together, at the expense of some code duplication. I'll address this with next update.

snicoll · 2017-03-29T12:30:54Z

...org/springframework/boot/autoconfigure/hazelcast/HazelcastClientConfigResourceCondition.java

+
+	static final String CONFIG_SYSTEM_PROPERTY = "hazelcast.client.config";
+
+	protected HazelcastClientConfigResourceCondition(String prefix, String propertyName) {


Perhaps there is a way to abstract more since all that should change is the name of the file (hazelcast.xml vs. hazelcast-client.xml).

snicoll · 2017-03-29T12:32:32Z

...in/java/org/springframework/boot/autoconfigure/hazelcast/HazelcastClientInstanceFactory.java

+ * @author Vedran Pavic
+ * @since 2.0.0
+ */
+public class HazelcastClientInstanceFactory {


speaking out loud, I wonder if two different factories are deserved for this. Perhaps we should restructure the one we have a bit so that we could create a server or a client HazelcastInstance.

After looking into this a bit more I'd say we could even remove both factories. HazelcastInstanceFactory was introduced quite a while ago in db41fb1 to (from what I call tell) avoid having HazelcastCacheConfiguration directly rely on HazelcastAutoConfiguration#createHazelcastInstance. With your changes from #8470 there are no more uses of HazelcastInstanceFactory outside of HazelcastAutoConfiguration so we could move that logic back in there. WDYT?

We could but it's public API so I'd rather break the contract than remove it completely. It doesn't hurt to offer the feature to create an HazelcastInstance based on a Resource anyway.

snicoll · 2017-03-29T12:34:04Z

.../src/test/java/org/springframework/boot/autoconfigure/cache/CacheAutoConfigurationTests.java

+	public static void close() {
+		System.clearProperty("hazelcast.jcache.provider.type");
+	}
+


I don't like that. You shouldn't mess with this in the general CacheAutoConfiguration. Is it because the client is now on the classpath and the JCache support doesn't know what to do?

If that's the case, we should fix the JCache implementation to set the right flag based on the HazelcastInstance that is available.

Yes, it's due to client being on the classpath and Hazelcast prefers it in that case. See HazelcastCachingProvider#createClientProvider.

If that's the case, that's what the test should assert then. And you should have a test that filters out the client (see that JUnit rule) to restore what happens when the client jar isn't on the classpath.

snicoll · 2017-03-29T12:34:49Z

...rg/springframework/boot/autoconfigure/hazelcast/AbstractHazelcastAutoConfigurationTests.java

+ * @author Stephane Nicoll
+ * @author Vedran Pavic
+ */
+public class AbstractHazelcastAutoConfigurationTests {


Meh. I'd really like we avoid separating things like that. In particular we could test what happens when both the server and the client is available (the client wins?). And then we could add an explicit spring.hazelcast.mode that sets the other one and we can validate the right instance has been auto-configured.

There is such test already, see HazelcastClientAutoConfigurationTests#clientConfigHasPriority. I guess when we go back to single HazelcastAutoConfiguration it should help the situation with tests too.

vpavic · 2017-04-04T06:32:48Z

@snicoll I've updated the PR to address your comments.

However I didn't add spring.hazelcast.mode as it seemed of little value to me. Since there are basically 4 ways to auto-configure Hazelcast:

config bean: Config vs ClientConfig
default config file location: hazelcast.xml vs hazelcast-client.xml at the root of classpath
system property config file location: hazelcast.config vs hazelcast.client.config
config property config file location using spring.hazelcast.config

spring.hazelcast.mode IMO only makes sense with spring.hazelcast.config, but even then the presence of Hazelcast client on classpath should indicate that the user prefers client. Going forward, mode makes even less sense since Hazelcast have added hazelcast/hazelcast#7448 to their roadmap.

snicoll · 2017-04-04T08:29:50Z

I am confused.

Say that you have the hazelcast-client.jar on your classpath for some reason and you have a my-hazelcast.xml configuration that contains a "server" config. You're using the property to refer to it. The problem I have with that approach is that you have no way to tell that the file defines a server config.

If I understood properly, this use case will always attempt to create a client and you'll have no way to force it to use another mode. The only way you'll be able to force that is by excluding the client auto-configuration part but we've merged it into one (which IMO is the right call).

Unless I am missing something, you need a way to tell us that looking at the classpath isn't enough to figure out the mode to use. The fact the issue you referenced is implemented or not makes no difference (a.k.a. you can't ensure that all apps will have a clean classpath).

Having said that, it is a corner case for the property so perhaps we should create it only if someone complains. Let me know if I've missed anything.

vpavic · 2017-04-04T08:52:37Z

Thanks for the quick feedback.

Say that you have the hazelcast-client.jar on your classpath for some reason and you have a my-hazelcast.xml configuration that contains a "server" config. You're using the property to refer to it. The problem I have with that approach is that you have no way to tell that the file defines a server config.

Correct. Currently the only way to achieve this is by removing Hazelcast client from the classpath. I agree it's not ideal but I too consider this to be a corner case.

Unless I am missing something, you need a way to tell us that looking at the classpath isn't enough to figure out the mode to use.

Yes, but the thing that bothers me with spring.hazelcast.mode is that it messed with the other 3 configuration methods which offer a clear server vs client distinction by themselves. Perhaps we should consider going back to having a dedicated config property for both server and client config location as it solves the problem from above using a single configuration property (vs two with spring.hazelcast.config plus mode property). It also provides parity with Hazelcast's system properties.

neilstevenson · 2017-04-05T20:34:12Z

@snicoll How near is this to going live ? It may need amended again for the arrival of [http://jet.hazelcast.org/](Hazelcast Jet) which embeds jars from [https://hazelcast.org/](Hazelcast IMDG)

snicoll · 2017-04-06T05:49:26Z

@neilstevenson I have not make up my mind about the two properties yet. I have no idea what "Hazelcast Jet" is nor what you mean by "amending again".

neilstevenson · 2017-04-07T13:00:03Z

@snicoll I've logged #8863 to outline Hazelcast IMDG Client & Server and Hazelcast Jet Client & Server, with a link to sample project on github - #8863

I'm happy to make a fork to fix the issue myself, but just don't want this to clash with work already underway for this issue.

Let me know what you'd prefer.

snicoll · 2017-04-09T08:28:31Z

Thanks for creating the issue; the reference to a sample is also much appreciated. Let's follow up there.

snicoll · 2017-04-09T08:34:36Z

the thing that bothers me with spring.hazelcast.mode is that it messed with the other 3 configuration methods which offer a clear server vs client distinction by themselves.

How does it mess with them exactly? IMO, you are looking to this from a Hazelcast perspective, not a Spring Boot perspective. Spring Boot has an auto-configuration model so it can be smart about the environment and should provide a way-out if the smartness doesn't do the thing that the user is expecting.

I hazelcast had such tool, I am not sure they would have come up with two distinct properties for this.

Having said that, you made your case and I think we can introduce that property later if users are complaining. After all, if you have both on the classpath and you don't want to use the client and your config file is not using the default scheme, you're already quite far in customizations. That mode would be useful only for that.

I'll resume without the mode then, thank you very much!

vpavic · 2017-04-09T09:47:12Z

How does it mess with them exactly?

What I've meant to say by that, but perhaps wasn't clear enough, is that spring.hazelcast.mode would have to be considered in all configuration scenarios and could therefore allow to form an invalid configuration if used with anything other than spring.hazelcast.config.

In contrast two properties for config location would be focused on only that configuration scenario.

But I agree your approach to start simple and introduce any additional configuration properties if the need arises.

See gh-7469

* pr/7469: Polish "Add auto-configuration support for Hazelcast client" Add auto-configuration support for Hazelcast client

snicoll · 2017-05-04T13:54:39Z

Merged, thanks @vpavic! In the end I changed my mind on some minor area (typically the factory thing).

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 23, 2016

snicoll added the status: waiting-for-feedback We need additional information before we can continue label Nov 24, 2016

vpavic force-pushed the gh-4918 branch from c6668c2 to 7c35334 Compare November 24, 2016 08:21

snicoll removed the status: waiting-for-feedback We need additional information before we can continue label Nov 25, 2016

snicoll added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Nov 25, 2016

snicoll added this to the 2.0.0.M1 milestone Nov 25, 2016

snicoll self-assigned this Nov 25, 2016

vpavic force-pushed the gh-4918 branch from 7c35334 to bfbd119 Compare March 3, 2017 11:58

snicoll requested changes Mar 29, 2017

View reviewed changes

vpavic force-pushed the gh-4918 branch from bfbd119 to a81b1a2 Compare April 3, 2017 22:24

snicoll added the for: team-attention An issue we'd like other members of the team to review label Apr 4, 2017

Add auto-configuration support for Hazelcast client

5185be1

vpavic force-pushed the gh-4918 branch from a81b1a2 to 5185be1 Compare April 5, 2017 15:37

snicoll mentioned this pull request Apr 7, 2017

Autoconfiguration support for Hazelcast clients #4918

Closed

neilstevenson mentioned this pull request Apr 7, 2017

Autoconfiguration support for Hazelcast IMDG & Jet #8863

Closed

snicoll added priority: normal and removed for: team-attention An issue we'd like other members of the team to review labels Apr 9, 2017

snicoll pushed a commit that referenced this pull request May 4, 2017

Add auto-configuration support for Hazelcast client

3fbf1a2

See gh-7469

snicoll closed this in 0aded58 May 4, 2017

snicoll added a commit that referenced this pull request May 4, 2017

Merge pull request #7469 from vpavic:gh-4918

39a1c5e

* pr/7469: Polish "Add auto-configuration support for Hazelcast client" Add auto-configuration support for Hazelcast client

vpavic deleted the gh-4918 branch May 4, 2017 15:40

snicoll mentioned this pull request May 16, 2018

@ConfigurationProperties : issue with com.hazelcast.client.config.ClientConfig.networkConfig.addressList/setAddresses() #13189

Closed


		static final String CONFIG_SYSTEM_PROPERTY = "hazelcast.client.config";

		protected HazelcastClientConfigResourceCondition(String prefix, String propertyName) {

Uh oh!

Add auto-configuration support for Hazelcast client #7469

Add auto-configuration support for Hazelcast client #7469

Uh oh!

Conversation

vpavic commented Nov 23, 2016

Uh oh!

snicoll commented Nov 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vpavic commented Nov 24, 2016

Uh oh!

snicoll commented Nov 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vpavic commented Nov 24, 2016

Uh oh!

snicoll commented Nov 24, 2016

Uh oh!

vpavic commented Nov 24, 2016

Uh oh!

vpavic commented Nov 25, 2016

Uh oh!

snicoll commented Nov 25, 2016

Uh oh!

vpavic commented Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snicoll left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vpavic Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vpavic commented Apr 4, 2017

Uh oh!

snicoll commented Apr 4, 2017

Uh oh!

vpavic commented Apr 4, 2017

Uh oh!

neilstevenson commented Apr 5, 2017

Uh oh!

snicoll commented Apr 6, 2017

Uh oh!

neilstevenson commented Apr 7, 2017

Uh oh!

snicoll commented Apr 9, 2017

Uh oh!

snicoll commented Apr 9, 2017

Uh oh!

vpavic commented Apr 9, 2017

snicoll commented Nov 24, 2016 •

edited

Loading

snicoll commented Nov 24, 2016 •

edited

Loading

vpavic commented Mar 3, 2017 •

edited

Loading

vpavic Mar 29, 2017 •

edited

Loading