Added auto-configuration for Spring Boot #103

alparslanavci · 2020-12-15T10:25:14Z

Fixes #73.

enozcan

Regardless of the PR, does it make sense to use instance name from configuration to seek for existing instances? If I'm not wrong, this will throw NPE when an ordinary hazelcast.xml with instance-name field is used whereas it's supposed to be a valid configuration to start a standalone instance:

hazelcast-tomcat-sessionmanager/tomcat-core/src/main/java/com/hazelcast/session/HazelcastInstanceFactory.java

Lines 57 to 58 in 3e86d43

    
           } else if (instanceName != null) { 
        
               instance = Hazelcast.getHazelcastInstanceByName(instanceName);

and then

hazelcast-tomcat-sessionmanager/tomcat9/src/main/java/com/hazelcast/session/HazelcastSessionManager.java

Line 92 in 3e86d43

sessionMap = instance.getMap(mapName);

...at9/src/main/java/com/hazelcast/session/springboot/HazelcastSessionManagerConfiguration.java

alparslanavci · 2020-12-16T14:25:06Z

@enozcan yes, you are correct. This is a known issue, and actually, it requires much larger refactoring. Maybe we can create a GH issue for tracking, but I am pretty sure that it needs a larger effort to fix it in general.

leszko

Added my comments.

Also, I tried this autoconfiguration with the latest Spring Boot Web and got the following exception. Could you check?

15:38:06.275 [main] ERROR o.s.boot.SpringApplication - Application run failed
java.lang.IllegalStateException: Error processing condition on org.springframework.boot.autoconfigure.cache.CacheAutoConfiguration
	at org.springframework.boot.autoconfigure.condition.SpringBootCondition.matches(SpringBootCondition.java:60)
	at org.springframework.context.annotation.ConditionEvaluator.shouldSkip(ConditionEvaluator.java:108)
	at org.springframework.context.annotation.ConfigurationClassBeanDefinitionReader$TrackedConditionEvaluator.shouldSkip(ConfigurationClassBeanDefinitionReader.java:489)
	at org.springframework.context.annotation.ConfigurationClassBeanDefinitionReader$TrackedConditionEvaluator.shouldSkip(ConfigurationClassBeanDefinitionReader.java:478)
	at org.springframework.context.annotation.ConfigurationClassBeanDefinitionReader.loadBeanDefinitionsForConfigurationClass(ConfigurationClassBeanDefinitionReader.java:140)
	at org.springframework.context.annotation.ConfigurationClassBeanDefinitionReader.loadBeanDefinitions(ConfigurationClassBeanDefinitionReader.java:129)
	at org.springframework.context.annotation.ConfigurationClassPostProcessor.processConfigBeanDefinitions(ConfigurationClassPostProcessor.java:348)
	at org.springframework.context.annotation.ConfigurationClassPostProcessor.postProcessBeanDefinitionRegistry(ConfigurationClassPostProcessor.java:252)
	at org.springframework.context.support.PostProcessorRegistrationDelegate.invokeBeanDefinitionRegistryPostProcessors(PostProcessorRegistrationDelegate.java:285)
	at org.springframework.context.support.PostProcessorRegistrationDelegate.invokeBeanFactoryPostProcessors(PostProcessorRegistrationDelegate.java:99)
	at org.springframework.context.support.AbstractApplicationContext.invokeBeanFactoryPostProcessors(AbstractApplicationContext.java:751)
	at org.springframework.context.support.AbstractApplicationContext.refresh(AbstractApplicationContext.java:569)
	at org.springframework.boot.web.servlet.context.ServletWebServerApplicationContext.refresh(ServletWebServerApplicationContext.java:144)
	at org.springframework.boot.SpringApplication.refresh(SpringApplication.java:767)
	at org.springframework.boot.SpringApplication.refresh(SpringApplication.java:759)
	at org.springframework.boot.SpringApplication.refreshContext(SpringApplication.java:426)
	at org.springframework.boot.SpringApplication.run(SpringApplication.java:326)
	at org.springframework.boot.SpringApplication.run(SpringApplication.java:1309)
	at org.springframework.boot.SpringApplication.run(SpringApplication.java:1298)
	at com.example.demo.DemoApplication.main(DemoApplication.java:10)
Caused by: java.lang.IllegalStateException: Failed to introspect Class [com.hazelcast.session.springboot.HazelcastSessionManagerConfiguration] from ClassLoader [jdk.internal.loader.ClassLoaders$AppClassLoader@3764951d]
	at org.springframework.util.ReflectionUtils.getDeclaredMethods(ReflectionUtils.java:481)
	at org.springframework.util.ReflectionUtils.doWithMethods(ReflectionUtils.java:358)
	at org.springframework.util.ReflectionUtils.getUniqueDeclaredMethods(ReflectionUtils.java:414)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.lambda$getTypeForFactoryMethod$2(AbstractAutowireCapableBeanFactory.java:754)
	at java.base/java.util.concurrent.ConcurrentHashMap.computeIfAbsent(ConcurrentHashMap.java:1708)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.getTypeForFactoryMethod(AbstractAutowireCapableBeanFactory.java:753)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.determineTargetType(AbstractAutowireCapableBeanFactory.java:692)
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.predictBeanType(AbstractAutowireCapableBeanFactory.java:663)
	at org.springframework.beans.factory.support.AbstractBeanFactory.isFactoryBean(AbstractBeanFactory.java:1665)
	at org.springframework.beans.factory.support.DefaultListableBeanFactory.doGetBeanNamesForType(DefaultListableBeanFactory.java:570)
	at org.springframework.beans.factory.support.DefaultListableBeanFactory.getBeanNamesForType(DefaultListableBeanFactory.java:542)
	at org.springframework.boot.autoconfigure.condition.OnBeanCondition.collectBeanNamesForType(OnBeanCondition.java:238)
	at org.springframework.boot.autoconfigure.condition.OnBeanCondition.getBeanNamesForType(OnBeanCondition.java:231)
	at org.springframework.boot.autoconfigure.condition.OnBeanCondition.getBeanNamesForType(OnBeanCondition.java:221)
	at org.springframework.boot.autoconfigure.condition.OnBeanCondition.getMatchingBeans(OnBeanCondition.java:169)
	at org.springframework.boot.autoconfigure.condition.OnBeanCondition.getMatchOutcome(OnBeanCondition.java:119)
	at org.springframework.boot.autoconfigure.condition.SpringBootCondition.matches(SpringBootCondition.java:47)
	... 19 common frames omitted
Caused by: java.lang.NoClassDefFoundError: com/hazelcast/config/Config
	at java.base/java.lang.Class.getDeclaredMethods0(Native Method)
	at java.base/java.lang.Class.privateGetDeclaredMethods(Class.java:3325)
	at java.base/java.lang.Class.getDeclaredMethods(Class.java:2466)
	at org.springframework.util.ReflectionUtils.getDeclaredMethods(ReflectionUtils.java:463)
	... 35 common frames omitted
Caused by: java.lang.ClassNotFoundException: com.hazelcast.config.Config
	at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:606)
	at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:168)
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522)
	... 39 common frames omitted

tomcat-core/pom.xml

leszko · 2020-12-16T14:41:54Z

tomcat-core/src/main/java/com/hazelcast/session/ClientServerConfigLoader.java

@@ -28,16 +28,16 @@

 import static com.hazelcast.internal.config.ConfigLoader.locateConfig;

-class ClientServerConfigLoader {
+public class ClientServerConfigLoader {


An idea: maybe we could get rid of the com.hazelcast.session.springboot package and keep these package-private?

I thought about it, but I think it will be better to keep springboot package since there are also test classes under the same package. Otherwise, all classes will be in the same parent package. WDYT?

Isn't it possible to put tests in a separate package, but keep the code together? Like we already have separate packages for different tests: nonsticky, sticky.

yes, surely we can do it. I just thought it is tidier :) I removed the springboot package now 👍

leszko · 2020-12-16T14:43:39Z

README.md

+
+Starting with v2.2, Hazelcast Tomcat Session Manager supports auto-configuration when used with Spring Boot. The only thing you need to do is to add Hazelcast Tomcat Session Manager (for Tomcat 9) library to the classpath. This will set Hazelcast Session Manager as the session manager of the Tomcat. If you would like to configure the session manager properties, you can setup the following properties in your `application.properties` file:
+
+- `tsm.hazelcast.config.location`: Allows to provide Hazelcast member configuration. If not provided, `hazelcast.xml` in the classpath is used by default. Only works if `tsm.client.only` is **not** to set true. 


Is it common to use such abbreviations as prefix? Like tsm? Did you see other projects using it?

Actually, I haven't checked but I used it to separate from other properties with the same name for other modules. Do you have any other suggestion?

I see. Yeah, makes sense, otherwise, they'd collide with other properties. But would it maybe make sense to stick to some convention from Spring, like here: https://docs.spring.io/spring-boot/docs/2.1.18.RELEASE/reference/html/boot-features-hazelcast.html

For example, in Spring you define the Hazelcast configuration with spring.hazelcast.config and in your configuration it's tsm.hazelcast.config.location. So from what I see, Spring just added a prefix spring. to all the Hazelcast properties. So, maybe you should call yours like tsm.hazelcast.config?

I preferred to use the same config property names as we use in Tomcat Session Manger itself, just adding tsm. in front of them (See: https://github.com/hazelcast/hazelcast-tomcat-sessionmanager#configuring-manager-element-for-tomcat). I think this makes more sense from the user's point of view. I changed the tsm.hazelcast.config.location to tsm.config.location accordingly.

leszko · 2020-12-16T14:47:10Z

...at9/src/main/java/com/hazelcast/session/springboot/HazelcastSessionManagerConfiguration.java

+
+    @Bean
+    @ConditionalOnMissingBean(type = "com.hazelcast.config.Config")
+    @ConditionalOnProperty(name = "tsm.client.only", havingValue = "false", matchIfMissing = true)


I wonder, how Spring Boot now detects if P2P or Client-Server is used? Maybe we can piggy-back on this and not introduce our own parameter tsm.client.only?

I will check this 👍

leszko · 2020-12-16T14:49:02Z

tomcat9/src/main/java/com/hazelcast/session/springboot/package-info.java

+/**
+ * This package provides Spring Boot auto-configuration classes
+ */
+package com.hazelcast.session.springboot;


As I already mentioned somewhere, consider removing this package.

Replied under the other comment.

leszko · 2020-12-16T14:52:23Z

tomcat9/src/test/java/com/hazelcast/session/springboot/AbstractSpringBootConfigurationTest.java

+            throws Exception {
+        HazelcastInstance hazelcastInstance = (HazelcastInstance) applicationContext.getBean("hazelcastInstance");
+        LinkedList<DistributedObject> distributedObjects = (LinkedList<DistributedObject>) hazelcastInstance.getDistributedObjects();
+        assertEquals("Session map should be created.", 1, distributedObjects.size());


It's not a good practice to mix assertions with the test preparation. The test should have a clear structure given when then. Could you either split into multiple tests or remove this assertion?

I changed them, but I am not sure if we should follow given when then structure here. These are more likely integration tests in general.

leszko · 2020-12-16T14:54:50Z

tomcat9/src/test/java/com/hazelcast/session/springboot/ClientServerApplication.java

+
+@SpringBootApplication
+@PropertySource("classpath:clientServer.properties")
+public class ClientServerApplication {


How about embedding this class into ClientServerSpringBootConfigurationTest?

leszko · 2020-12-16T14:56:09Z

tomcat9/src/test/java/com/hazelcast/session/springboot/P2PApplication.java

+
+@SpringBootApplication
+@ComponentScan(excludeFilters = @ComponentScan.Filter(type = FilterType.ASSIGNABLE_TYPE,
+        classes = ClientServerApplication.class))


why do you need classes = ClientServerApplication.class? Isn't this class independent from ClientServerApplication?

ClientServerApplication adds clientServer.properties as property source, and it is a component since they are all in the same package. I excluded it in the P2PApplication to prevent tsm.client.only=true take effect.

alparslanavci

@leszko, you got the exception because most likely you haven't added Hazelcast to the classpath. It should be also provided, I'll update the readme accordingly.

alparslanavci · 2020-12-16T17:15:12Z

README.md

+
+Starting with v2.2, Hazelcast Tomcat Session Manager supports auto-configuration when used with Spring Boot. The only thing you need to do is to add Hazelcast Tomcat Session Manager (for Tomcat 9) library to the classpath. This will set Hazelcast Session Manager as the session manager of the Tomcat. If you would like to configure the session manager properties, you can setup the following properties in your `application.properties` file:
+
+- `tsm.hazelcast.config.location`: Allows to provide Hazelcast member configuration. If not provided, `hazelcast.xml` in the classpath is used by default. Only works if `tsm.client.only` is **not** to set true. 


Actually, I haven't checked but I used it to separate from other properties with the same name for other modules. Do you have any other suggestion?

tomcat-core/pom.xml

alparslanavci · 2020-12-16T17:24:19Z

tomcat-core/src/main/java/com/hazelcast/session/ClientServerConfigLoader.java

@@ -28,16 +28,16 @@

 import static com.hazelcast.internal.config.ConfigLoader.locateConfig;

-class ClientServerConfigLoader {
+public class ClientServerConfigLoader {


I thought about it, but I think it will be better to keep springboot package since there are also test classes under the same package. Otherwise, all classes will be in the same parent package. WDYT?

alparslanavci · 2020-12-16T17:24:58Z

tomcat9/src/main/java/com/hazelcast/session/springboot/package-info.java

+/**
+ * This package provides Spring Boot auto-configuration classes
+ */
+package com.hazelcast.session.springboot;


Replied under the other comment.

alparslanavci · 2020-12-16T17:48:10Z

tomcat9/src/test/java/com/hazelcast/session/springboot/AbstractSpringBootConfigurationTest.java

+            throws Exception {
+        HazelcastInstance hazelcastInstance = (HazelcastInstance) applicationContext.getBean("hazelcastInstance");
+        LinkedList<DistributedObject> distributedObjects = (LinkedList<DistributedObject>) hazelcastInstance.getDistributedObjects();
+        assertEquals("Session map should be created.", 1, distributedObjects.size());


I changed them, but I am not sure if we should follow given when then structure here. These are more likely integration tests in general.

alparslanavci · 2020-12-16T17:52:17Z

tomcat9/src/test/java/com/hazelcast/session/springboot/ClientServerApplication.java

+
+@SpringBootApplication
+@PropertySource("classpath:clientServer.properties")
+public class ClientServerApplication {


alparslanavci · 2020-12-16T17:53:58Z

tomcat9/src/test/java/com/hazelcast/session/springboot/P2PApplication.java

+
+@SpringBootApplication
+@ComponentScan(excludeFilters = @ComponentScan.Filter(type = FilterType.ASSIGNABLE_TYPE,
+        classes = ClientServerApplication.class))


ClientServerApplication adds clientServer.properties as property source, and it is a component since they are all in the same package. I excluded it in the P2PApplication to prevent tsm.client.only=true take effect.

alparslanavci · 2020-12-16T17:54:25Z

...at9/src/main/java/com/hazelcast/session/springboot/HazelcastSessionManagerConfiguration.java

+
+    @Bean
+    @ConditionalOnMissingBean(type = "com.hazelcast.config.Config")
+    @ConditionalOnProperty(name = "tsm.client.only", havingValue = "false", matchIfMissing = true)


I will check this 👍

leszko · 2020-12-17T08:49:16Z

@leszko, you got the exception because most likely you haven't added Hazelcast to the classpath. It should be also provided, I'll update the readme accordingly.

Wouldn't it be possible to detect it at runtime and give a proper warning/error message?

enozcan · 2020-12-17T09:19:04Z

...at9/src/main/java/com/hazelcast/session/springboot/HazelcastSessionManagerConfiguration.java

+
+    @Bean(name = "hazelcastTomcatSessionManagerCustomizer")
+    public WebServerFactoryCustomizer<TomcatServletWebServerFactory> customizeTomcat(HazelcastInstance hazelcastInstance) {
+        return new WebServerFactoryCustomizer<TomcatServletWebServerFactory>() {


I afraid in client.only mode, a client will be created here via HazelcastInstance bean with the client config above. However, another client will also be created at HazelcastSessionManager#startInternal for the manager. Is that the intent? For instance, this one fails in ClientServerSpringBootConfigurationTest:

@Test public void createOnlyOneClient() { Assert.assertEquals(1, HazelcastClient.getAllHazelcastClients().size()); // java.lang.AssertionError: // Expected :1 // Actual :2 }

@enozcan yes, you were correct. I fixed it by using the Hazelcast client instance initiated by Spring Boot.

alparslanavci

@leszko regarding the error you got, we cannot give a proper error message for that since the exception is thrown from Spring Boot itself.

However, I removed the tsm.client.only parameter as you suggested. Now, it is auto-configured accordingly with the provided Hazelcast configuration file.

alparslanavci · 2021-01-01T13:16:01Z

...at9/src/main/java/com/hazelcast/session/springboot/HazelcastSessionManagerConfiguration.java

+
+    @Bean(name = "hazelcastTomcatSessionManagerCustomizer")
+    public WebServerFactoryCustomizer<TomcatServletWebServerFactory> customizeTomcat(HazelcastInstance hazelcastInstance) {
+        return new WebServerFactoryCustomizer<TomcatServletWebServerFactory>() {


@enozcan yes, you were correct. I fixed it by using the Hazelcast client instance initiated by Spring Boot.

...at9/src/main/java/com/hazelcast/session/springboot/HazelcastSessionManagerConfiguration.java

tomcat9/src/main/java/com/hazelcast/session/HazelcastSessionManager.java

leszko · 2021-01-04T10:38:23Z

@leszko regarding the error you got, we cannot give a proper error message for that since the exception is thrown from Spring Boot itself.

Wouldn't it work if you check it somewhere here? I mean to check if you have Hazelcast on the classpath?

The other option is to change the Hazelcast dependency scope to not be provided. What do you think?

leszko

Thanks for the changes Alparslan. Added some replies to your comments.

leszko

Please make the classes package-private. The rest looks good 👍

Added auto-configuration for Spring Boot

c1c6524

alparslanavci added this to the 2.2 milestone Dec 15, 2020

alparslanavci added 3 commits December 15, 2020 14:32

Updated Spring Boot version

78a150d

Updated README.md for Spring Boot auto-configuration

5d3d400

Updated README.md contents

1c284f0

alparslanavci requested review from leszko and enozcan December 15, 2020 17:12

enozcan reviewed Dec 16, 2020

View reviewed changes

...at9/src/main/java/com/hazelcast/session/springboot/HazelcastSessionManagerConfiguration.java Outdated Show resolved Hide resolved

alparslanavci requested a review from enozcan December 16, 2020 14:36

leszko reviewed Dec 16, 2020

View reviewed changes

alparslanavci commented Dec 16, 2020

View reviewed changes

Review updates

1e187fe

enozcan reviewed Dec 17, 2020

View reviewed changes

alparslanavci added 4 commits December 30, 2020 17:17

Fixed multiple clients issue with Spring Boot

5ed2d6e

Small refactoring

6a7d496

Removed tsm.client.only property

445ed8d

Added RuntimeException in case of non-initiated Hazelcast instance

7dce3bc

alparslanavci commented Jan 1, 2021

View reviewed changes

alparslanavci requested review from leszko and enozcan January 1, 2021 13:21

enozcan reviewed Jan 4, 2021

View reviewed changes

leszko reviewed Jan 4, 2021

View reviewed changes

Review updates

4fb15dc

alparslanavci requested review from leszko and enozcan January 4, 2021 12:49

enozcan approved these changes Jan 4, 2021

View reviewed changes

leszko approved these changes Jan 4, 2021

View reviewed changes

Removed public access

1665ee6

alparslanavci merged commit 0de5d69 into hazelcast:master Jan 4, 2021

	} else if (instanceName != null) {
	instance = Hazelcast.getHazelcastInstanceByName(instanceName);


		Starting with v2.2, Hazelcast Tomcat Session Manager supports auto-configuration when used with Spring Boot. The only thing you need to do is to add Hazelcast Tomcat Session Manager (for Tomcat 9) library to the classpath. This will set Hazelcast Session Manager as the session manager of the Tomcat. If you would like to configure the session manager properties, you can setup the following properties in your `application.properties` file:

		- `tsm.hazelcast.config.location`: Allows to provide Hazelcast member configuration. If not provided, `hazelcast.xml` in the classpath is used by default. Only works if `tsm.client.only` is not to set true.

Added auto-configuration for Spring Boot #103

Added auto-configuration for Spring Boot #103

Conversation

alparslanavci commented Dec 15, 2020

enozcan left a comment • edited Loading

Choose a reason for hiding this comment

alparslanavci commented Dec 16, 2020

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

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

alparslanavci 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

leszko commented Dec 17, 2020

enozcan Dec 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alparslanavci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leszko commented Jan 4, 2021

leszko left a comment

Choose a reason for hiding this comment

leszko left a comment

Choose a reason for hiding this comment

enozcan left a comment •

edited

Loading

enozcan Dec 17, 2020 •

edited

Loading