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.

Sign up for GitHub

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

Jump to bottom

Add a way to customize Hazelcast's Client/Server configs #19669

Closed
wants to merge 1 commit into from
Closed

Add a way to customize Hazelcast's Client/Server configs #19669

wants to merge 1 commit into from

Conversation

nosan
Copy link
Contributor

@nosan nosan commented Jan 13, 2020 •
edited
Loading

This PR provides a way to customize Hazelcast's Client/Server configs before HazelcastInstance is created. At the moment there is no way to do this except creating your own config.

Here is a small example of a configuration, that shows how to set SpringManagedContext using the HazelcastConfigCustomizer

@Configuration(proxyBeanMethods = false)
static class HazelcastConfiguration {

	@Bean
	HazelcastConfigCustomizer hazelcastConfigCustomizer(ApplicationContext applicationContext) {
		return (config) -> {
			SpringManagedContext managedContext = new SpringManagedContext();
			managedContext.setApplicationContext(applicationContext);
			config.setManagedContext(managedContext);
		};
	}

}

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 13, 2020
@nosan
Copy link
Contributor Author

nosan commented Jan 13, 2020

Related issues

snicoll
snicoll requested changes Jan 13, 2020
View reviewed changes
Copy link
Member

@snicoll snicoll left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @nosan. Before we actually look at the code I would have preferred that we discuss what concrete use cases this is addressing. I know that there are some part of Config that can't be set via config (and requires a Config instance) but we have to balance the cost of adding more public APIs vs. what concrete problems adding a public API will solve.

Generally speaking, the idea of creating a customizer is helpful but I haven't heard a lot of requests in that direction. If you have some more concrete use cases that the management context, please let us know.

I'd also research how to improve what we have rather than creating yet another pair of factories for the client.

...main/java/org/springframework/boot/autoconfigure/hazelcast/HazelcastClientConfigFactory.java Outdated
* @author Dmytro Nosan
* @since 2.3.0
*/
public final class HazelcastClientConfigFactory {
Copy link
Member

Choose a reason for hiding this comment

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

This partially duplicates what HazelcastClientFactory is doing. If that behaviour can/should be improved I don't think we should do it as part of this particular issue.

...e/src/main/java/org/springframework/boot/autoconfigure/hazelcast/HazelcastConfigFactory.java Outdated
* @author Dmytro Nosan
* @since 2.3.0
*/
public final class HazelcastConfigFactory {
Copy link
Member

Choose a reason for hiding this comment

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

Ditto. I also question the creation of such high-level concept for this. IMO this feels like Hazelcast itself should provide that.

...src/main/java/org/springframework/boot/autoconfigure/hazelcast/HazelcastInstanceFactory.java Outdated
*/
@Deprecated
Copy link
Member

Choose a reason for hiding this comment

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

Isn't that making HazelcastInstanceFactory effectively deprecated? For the reason above, I don't think we should do that and rather research how to improve the existing class to account for the customizers.

spring-boot-project/spring-boot-docs/src/docs/asciidoc/spring-boot-features.adoc Outdated
@@ -5666,9 +5666,6 @@ If https://hazelcast.com/[Hazelcast] is on the classpath and a suitable configur
If you define a `com.hazelcast.config.Config` bean, Spring Boot uses that.
If your configuration defines an instance name, Spring Boot tries to locate an existing instance rather than creating a new one.

If you define a `com.hazelcast.config.Config` bean, Spring Boot uses that.
If your configuration defines an instance name, Spring Boot tries to locate an existing instance rather than creating a new one.

Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be removed. If a user specifies an existing Config we should use that rather than trying to lookup one.

Copy link
Contributor Author

@nosan nosan Jan 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

These are duplicates.

Copy link
Member

Choose a reason for hiding this comment

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

Ah sorry, I missed that. Such polish must be done in a separate commit. I've addressed it in #19678

...va/org/springframework/boot/docs/hazelcast/HazelcastClientConfigCustomizerConfiguration.java
return (clientConfig) -> {
SpringManagedContext managedContext = new SpringManagedContext();
managedContext.setApplicationContext(applicationContext);
clientConfig.setManagedContext(managedContext);
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to enable the management context in a Hazelcast client?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generally speaking, I don't know.

Copy link
Member

Choose a reason for hiding this comment

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

@jerrinot can you please help here and let us know if it does?

@nosan
Copy link
Contributor Author

nosan commented Jan 13, 2020

Thank you for your feedback @snicoll

If you have some more concrete use cases that the management context, please let us know.

Unfortunately, I don't have any use cases. I've seen some snippets on StackOverflow, e.g. how to set a ClassLoader.
I definitely can not say whether this will be useful for folks or not. I'm considering this feature as nice-to-have.

Of course, I understand that you will have to maintain this code and if you feel it is not worthy then feel free to close this PR.

PR has been updated. I have removed additional factories that were used for configs creation.

@nosan nosan force-pushed the hazelcast-config-customizers branch from 29ad12a to ef7eda4 Compare January 13, 2020 15:49
@nosan
Add a way to customize Hazelcast's Client/Server configs
dc26e23 
Prior to this commit, there was no way to do this except creating your own config.
@nosan nosan force-pushed the hazelcast-config-customizers branch from ef7eda4 to dc26e23 Compare January 13, 2020 16:05
@snicoll snicoll added the for: team-attention An issue we'd like other members of the team to review label Jan 13, 2020
@snicoll snicoll mentioned this pull request Jan 13, 2020
Closed
@nosan nosan mentioned this pull request Jan 14, 2020
Closed
@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Jan 20, 2020
@snicoll
Copy link
Member

snicoll commented Feb 7, 2020

Thanks @nosan. I feel that adding two new top-level interface is not warranted here. If it turns out the callback is required to implement the configuration of the SpringManagementContext we can reconsider.

@snicoll snicoll closed this Feb 7, 2020
@snicoll snicoll added status: declined A suggestion or change that we don't feel we should currently apply and removed for: team-attention An issue we'd like other members of the team to review status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged labels Feb 7, 2020
@nosan
Copy link
Contributor Author

nosan commented Feb 7, 2020

@snicoll np, thanks

@nosan nosan deleted the hazelcast-config-customizers branch October 5, 2024 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snicoll snicoll snicoll requested changes

Assignees
No one assigned
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nosan @snicoll @spring-projects-issues