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.

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

Use URI as config location or instance name in CachingProvider.getCacheManager #9973

Merged

Conversation

@vbekiaris
Copy link
Contributor

vbekiaris commented Feb 23, 2017

When failing to locate or create a HazelcastInstance via Properties, use URI as config location or instance name in CachingProvider.getCacheManager as described in reference manual

Fixes #9958

@vbekiaris vbekiaris added this to the 3.9 milestone Feb 23, 2017
@vbekiaris vbekiaris self-assigned this Feb 23, 2017
@devOpsHazelcast
Copy link
Contributor

devOpsHazelcast commented Feb 23, 2017

Test PASSed.

@vbekiaris vbekiaris force-pushed the vbekiaris:fixes/3.9/cachemanager-hz-instance branch from d222440 to f4800d1 Feb 27, 2017
@devOpsHazelcast
Copy link
Contributor

devOpsHazelcast commented Feb 27, 2017

Test PASSed.

@vbekiaris vbekiaris changed the title [WIP] Use URI as config location or instance name in CachingProvider.getCacheManager Use URI as config location or instance name in CachingProvider.getCacheManager Feb 28, 2017
@vbekiaris vbekiaris requested a review from ahmetmircik Feb 28, 2017
@snicoll
Copy link
Contributor

snicoll commented Mar 2, 2017

Just to make sure I get this right, here is a scenario in Spring Boot:

  1. Hazelcast is started via the general auto-configuration. An HazelcastInstance is created using the default configuration file (hazelcast.xml) at the root of the classpath
  2. A javax.cache.CacheManager is requested after, using Hazelcast as the provider. With this fix, Hazelcast will realize that no URI = default and will check for the presence of an HazelcastInstancethat match those criteria, and therefore will not start another instance

Am I getting this properly?

If I am, the scenario above can be tuned with an URI to a configuration file (The hazelcast instance is created with foo-bar-hazelcast.xml and if we call CacheProvider.getCacheManager("foo-bar-hazelcast.xml", ...) it should also realize such a URI has been processed and reused the existing HazelcastInstance.

If I got this wrong, please let me know as Spring Boot exposes those limitations at the moment.

For futher reference, here is a test that showcases what I mean (in CacheAutoConfigurationTests):

@Test
public void hazelcastAsJCacheWithExistingHazelcastInstance() throws IOException {
	String cachingProviderFqn = HazelcastCachingProvider.class.getName();
	String configLocation = "org/springframework/boot/autoconfigure/hazelcast/hazelcast-specific.xml";
	load(new Class[]{DefaultCacheConfiguration.class,
			HazelcastAndCacheConfiguration.class},
			"spring.hazelcast.config=" + configLocation,
			"spring.cache.type=jcache",
			"spring.cache.jcache.provider=" + cachingProviderFqn,
			"spring.cache.jcache.config=" + configLocation);
	JCacheCacheManager cacheManager = validateCacheManager(
			JCacheCacheManager.class);
	javax.cache.CacheManager jCacheCacheManager = cacheManager.getCacheManager();
	assertThat(jCacheCacheManager).isInstanceOf(
			com.hazelcast.cache.HazelcastCacheManager.class);
	assertThat(((com.hazelcast.cache.HazelcastCacheManager) jCacheCacheManager)
			.getHazelcastInstance()).isSameAs(this.context.getBean(
					HazelcastInstance.class));
}

gives

java.lang.AssertionError: 
Expecting:
 <HazelcastInstance{name='_hzInstance_1_dev', node=[192.168.1.50]:5702}>
and actual:
 <HazelcastInstance{name='file:/Users/snicoll/workspace/pivotal/spring-boot/spring-boot-autoconfigure/target/test-classes/org/springframework/boot/autoconfigure/hazelcast/hazelcast-specific.xml', node=[192.168.1.50]:5701}>
to refer to the same object

However that test pass as soon as I provide an instance name in the configuration file

<hazelcast>
    <instance-name>foo-bar</instance-name>
</hazelcast>
@vbekiaris vbekiaris force-pushed the vbekiaris:fixes/3.9/cachemanager-hz-instance branch 2 times, most recently from f22f3fb to 0002168 Mar 2, 2017
@devOpsHazelcast
Copy link
Contributor

devOpsHazelcast commented Mar 2, 2017

Test PASSed.

1 similar comment
@devOpsHazelcast
Copy link
Contributor

devOpsHazelcast commented Mar 2, 2017

Test PASSed.

@vbekiaris
Copy link
Contributor Author

vbekiaris commented Mar 2, 2017

Hi @snicoll , this PR does not work around the fact that instance-name must be set in the configuration in order to match an existing HazelcastInstance. This limitation stems from the fact that it is possible to create as many HazelcastInstances as you want from a given configuration file that does not specify an instance name -> how should we pick one HazelcastInstance of many?

@snicoll
Copy link
Contributor

snicoll commented Mar 2, 2017

I think that makes sense and there is this getOrCreate that does exactly what I want. But it does not for the automatic detection of the configuration file. I've created #10007 to track that. I think this would fix most of the issues we have in Boot.

@vbekiaris vbekiaris force-pushed the vbekiaris:fixes/3.9/cachemanager-hz-instance branch from 0002168 to 029fa38 Mar 14, 2017
@devOpsHazelcast
Copy link
Contributor

devOpsHazelcast commented Mar 14, 2017

Test PASSed.

Copy link
Contributor

jerrinot left a comment

tiny comment regarding logging + suggestion for another PR. good work!

throw ExceptionUtil.rethrow(e);
}
Config config = getConfigFromLocation(location, classLoader, instanceName);
return HazelcastInstanceFactory.getOrCreateHazelcastInstance(config);
}

// If instance name is specified via properties, get instance through it.
if (instanceName != null) {
return Hazelcast.getHazelcastInstanceByName(instanceName);

This comment has been minimized.

Copy link
@jerrinot

jerrinot Mar 16, 2017

Contributor

out of the scope for this PR, but I believe this should use getOrCreate()

right now there is an inconsistency: when you explicitly pass a configuration then it will always use it and it will use getOrCreate()

I believe when you pass just the instanceName, but no explicit configuration then it should use default configuration.

Config config = getConfigFromLocation(uri, classLoader, null);
return HazelcastInstanceFactory.getOrCreateHazelcastInstance(config);
} catch (Exception e) {
ignore(e);

This comment has been minimized.

Copy link
@jerrinot

jerrinot Mar 16, 2017

Contributor

this makes troubleshooting hard. LOGGER.finest() is a better option.

@devOpsHazelcast
Copy link
Contributor

devOpsHazelcast commented Mar 16, 2017

Test PASSed.

…tCacheManager

Additional tests for CacheManager creation
@vbekiaris vbekiaris force-pushed the vbekiaris:fixes/3.9/cachemanager-hz-instance branch from e26b983 to dd2ac9b Mar 16, 2017
@vbekiaris
Copy link
Contributor Author

vbekiaris commented Mar 16, 2017

Test failure com.hazelcast.client.map.querycache.ClientQueryCacheBasicTest.entryRemoved_whenValueMatchesPredicate[includeValues: false, useQueryCacheFilteringStrategy: false, nearCache: false]

@vbekiaris
Copy link
Contributor Author

vbekiaris commented Mar 16, 2017

run-lab-run

@devOpsHazelcast
Copy link
Contributor

devOpsHazelcast commented Mar 16, 2017

Test PASSed.

@vbekiaris vbekiaris merged commit afa75d6 into hazelcast:master Mar 16, 2017
1 check passed
1 check passed
default Build finished.
Details
@vbekiaris vbekiaris deleted the vbekiaris:fixes/3.9/cachemanager-hz-instance branch Mar 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.