Fixes #20856.
Added a validation for Hazelcast client configuration on HazelcastClientConfiguration.ConfigAvailableCondition when spring.hazelcast.config property is available. If the configured config is a valid Hazelcast client configuration, then the client instance bean is initialized.
The validation is done by building the ClientConfig object from the configuration file for now, since there are no public API for client file validation in Hazelcast. When it is exposed, it can be updated to use this validation API.
Comment From: pivotal-issuemaster
@alparslanavci Please sign the Contributor License Agreement!
Click here to manually synchronize the status of this Pull Request.
See the FAQ for frequently asked questions.
Comment From: pivotal-issuemaster
@alparslanavci Thank you for signing the Contributor License Agreement!
Comment From: snicoll
@alparslanavci quite a coincidence there, we just had a team meeting when you submitted that PR and our conclusion was to try something like this.
there are no public API for client file validation in Hazelcast.
Is there an issue to have such API? Thanks!
Comment From: alparslanavci
@snicoll glad to hear that!
Is there an issue to have such API? Thanks!
AFAIK, no for now, but I will discuss with our team members and create it. I will share it here when it is ready.
Comment From: alparslanavci
Please see the created issue here: https://github.com/hazelcast/hazelcast/issues/16866
Comment From: alparslanavci
@snicoll I run some more test other than the ones in the issue, and found that there are still problems with .yaml configuration files. We have to await https://github.com/hazelcast/hazelcast/issues/16866 to be fixed to send a proper solution to this issue.
I am closing this PR now, we will send a new one when the fix is ready.
Comment From: snicoll
Thanks for the feedback. For the record we will not upgrade to Hazelcast v4 and drop compatiblity for Hazelcast v3 in this release so using that fix if that's a 4.1 change only is only an option via reflection. Can you share the tests you've made? It would be nice to have them automated or documented somewhere.
Comment From: alparslanavci
The current tests in HazelcastAutoConfigurationServerTests and HazelcastAutoConfigurationClientTests will be sufficient when you upgrade to Hazelcast 4.0.
We will update you when the fix is ready.
Comment From: snicoll
I run some more test other than the ones in the issue, and found that there are still problems with .yaml configuration files.
I was referring to that. I don't think we will go with something that is 4.1 only as I've indicated above.
Comment From: alparslanavci
@snicoll The work on https://github.com/hazelcast/hazelcast/issues/16866 is close to completion. Thus, I updated this PR using this new API. I accessed it using reflection to keep support for Hazelcast 3. Please have a look, and add your comments to see if we are on the same track. If so, I can finalize this PR when the issue is fixed and officially released with Hazelcast.
Comment From: snicoll
@alparslanavci thanks for the update and sorry I couldn't provide a review in time. My initial reaction is that it's a lot of reflection code. I had hoped that the change would have been a bit less invasive. I understand that the API is now published, do you have an update for us?
Comment From: alparslanavci
@snicoll thanks, the API is published with Hazelcast 4.0.2 and I am now working on this to finalize.
My initial reaction is that it's a lot of reflection code. I had hoped that the change would have been a bit less invasive.
Yes, unfortunately we had to check for 3 classes within the new API to make it work together with Hazelcast 3.x and 4.x. The good news is I used MethodHandles, which won't add any performance drawbacks.
As I mentioned, I am working to finalize and I will let you know about my updates.
Comment From: alparslanavci
@snicoll this PR is ready for review & merge, please have a look when you have time. I didn't add any other changes, it will support both Hazelcast 3.x and 4.x. I am planning to send another PR for Hazelcast 4.0.2 upgrade.
Comment From: alparslanavci
@snicoll I think if we don't use reflection, then Hazelcast 4.x will be needed for the configuration beans' instantiation. I checked it without using reflection and getting the following exception if I try to run with Hazelcast 3:
Caused by: org.springframework.beans.BeanInstantiationException: Failed to instantiate [org.springframework.boot.autoconfigure.hazelcast.HazelcastClientConfiguration$ConfigAvailableCondition]: Unresolvable class definition; nested exception is java.lang.NoClassDefFoundError: com/hazelcast/config/ConfigRecognizer
at org.springframework.beans.BeanUtils.instantiateClass(BeanUtils.java:149) ~[spring-beans-5.2.5.RELEASE.jar:5.2.5.RELEASE]
at org.springframework.context.annotation.ConditionEvaluator.getCondition(ConditionEvaluator.java:125) ~[spring-context-5.2.5.RELEASE.jar:5.2.5.RELEASE]
at org.springframework.context.annotation.ConditionEvaluator.shouldSkip(ConditionEvaluator.java:96) ~[spring-context-5.2.5.RELEASE.jar:5.2.5.RELEASE]
at org.springframework.context.annotation.ConfigurationClassParser.processConfigurationClass(ConfigurationClassParser.java:225) ~[spring-context-5.2.5.RELEASE.jar:5.2.5.RELEASE]
at org.springframework.context.annotation.ConfigurationClassParser.processMemberClasses(ConfigurationClassParser.java:371) ~[spring-context-5.2.5.RELEASE.jar:5.2.5.RELEASE]
at org.springframework.context.annotation.ConfigurationClassParser.doProcessConfigurationClass(ConfigurationClassParser.java:271) ~[spring-context-5.2.5.RELEASE.jar:5.2.5.RELEASE]
at org.springframework.context.annotation.ConfigurationClassParser.processConfigurationClass(ConfigurationClassParser.java:249) ~[spring-context-5.2.5.RELEASE.jar:5.2.5.RELEASE]
at org.springframework.context.annotation.ConfigurationClassParser.processImports(ConfigurationClassParser.java:599) ~[spring-context-5.2.5.RELEASE.jar:5.2.5.RELEASE]
... 22 common frames omitted
Caused by: java.lang.NoClassDefFoundError: com/hazelcast/config/ConfigRecognizer
at java.lang.Class.getDeclaredConstructors0(Native Method) ~[na:1.8.0_232]
at java.lang.Class.privateGetDeclaredConstructors(Class.java:2671) ~[na:1.8.0_232]
at java.lang.Class.getConstructor0(Class.java:3075) ~[na:1.8.0_232]
at java.lang.Class.getDeclaredConstructor(Class.java:2178) ~[na:1.8.0_232]
at org.springframework.beans.BeanUtils.instantiateClass(BeanUtils.java:139) ~[spring-beans-5.2.5.RELEASE.jar:5.2.5.RELEASE]
... 29 common frames omitted
Caused by: java.lang.ClassNotFoundException: com.hazelcast.config.ConfigRecognizer
at java.net.URLClassLoader.findClass(URLClassLoader.java:382) ~[na:1.8.0_232]
at java.lang.ClassLoader.loadClass(ClassLoader.java:418) ~[na:1.8.0_232]
at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:349) ~[na:1.8.0_232]
at java.lang.ClassLoader.loadClass(ClassLoader.java:351) ~[na:1.8.0_232]
... 34 common frames omitted
Anything I am missing?
Comment From: snicoll
I think if we don't use reflection, then Hazelcast 4.x will be needed for the configuration beans' instantiation
Not if you catch the exception as a fallback case that assumes that Hazelcast 4 is not around. I haven't looked at this in detail but it feels to me that it should be doable to only call this method as a fallback when hazelcast-client is not around. If the default behaviour is to go with the non client mode that should be just enough.
Comment From: alparslanavci
Is there any proper way to understand if hazelcast-client is not around using Conditions? I don't think it is a good idea to check for the dependency jar names.
Comment From: snicoll
@alparslanavci thanks for the PR but we can't really proceed with this one without a proper test coverage. I've started to look at it and there are other implications that need to be taken into account as part of supporting Hazelcast 4 in Spring Boot, see WIP in https://github.com/spring-projects/spring-boot/issues/20856#issuecomment-689653073