IGNITE-13809 Adds thin client support to ignite-spring-cache. #46
Conversation
petrov-mg
force-pushed
the
IGNITE-13809
branch
2 times, most recently
from
a028f7a
to
a1e3d2d
Compare
petrov-mg
force-pushed
the
IGNITE-13809
branch
from
a1e3d2d
to
b218144
Compare
| * xsi:schemaLocation=" | ||
| * http://www.springframework.org/schema/beans http://www.springframework.org/schema/beans/spring-beans.xsd | ||
| * http://www.springframework.org/schema/cache http://www.springframework.org/schema/cache/spring-cache.xsd"> | ||
| * <bean id="igniteClient" class="org.apache.ignite.IgniteClientSpringBean"> |
There was a problem hiding this comment.
Let's add a note that this option available only in AI 2.11 and later
| * is not found among the configurations specified by this method, the default configuration with the requested | ||
| * name will be used. | ||
| */ | ||
| public IgniteClientSpringCacheManager setCacheConfigurations(ClientCacheConfiguration... cfgs) { |
There was a problem hiding this comment.
Why are we use different approaches for thin and thick clients? Can we use one dynamic configuration and create new caches based on this configuration changing only the cache name?
There was a problem hiding this comment.
At first I was stuck with ClientCacheConfiguration not implementing copy constructor as CacheConfiguration does.
Then I thought it would be quite useful to be able to provide a unique configuration for each cache, rather than one for all. I think it's not a problem to add dynamic configuration approach for IgniteClientSpringCacheManager but the current approach includes it. WDYT?
There was a problem hiding this comment.
Unique configuration for each cache can be defined in Ignite node configuration.
The current approach doesn't include a dynamic cache configuration approach, you should always set cache configuration explicitly for each cache.
There was a problem hiding this comment.
Done. Fresh TC build run - https://ci.ignite.apache.org/project.html?projectId=IgniteExtensions&branch_IgniteExtensions=pull%2F46%2F
There was a problem hiding this comment.
I'm sorry, I spoke too soon. It's not finished yet.
| return ccfgs == null ? Collections.emptyList() : ccfgs.values(); | ||
| /** Gets dynamic Ignite cache configuration template. */ | ||
| public ClientCacheConfiguration getDynamicCacheConfiguration() { | ||
| return dynamicCacheCfg == null ? null : dynamicCacheCfg.setName(null); // To avoid copying the dynamic cache configuration each time as we only change its name. |
There was a problem hiding this comment.
The line is too long.
| } | ||
| } | ||
| public IgniteClientSpringCacheManager setDynamicCacheConfiguration(ClientCacheConfiguration dynamicCacheCfg) { | ||
| this.dynamicCacheCfg = dynamicCacheCfg; | ||
|
|
||
| return this; | ||
| } | ||
|
|
||
| /** {@inheritDoc} */ | ||
| @Override protected SpringCache createCache(String name) { |
There was a problem hiding this comment.
I'm not sure if this method can be invoked concurrently, but if it can, it's better to make it synchronized since there can be a race between name modification and cache creation.
There was a problem hiding this comment.
Leaving it unsynchronized anyway is too dangerous. I did not think about that. Thank you. Done.
| import static org.apache.ignite.configuration.ClientConnectorConfiguration.DFLT_PORT; | ||
|
|
||
| /** Tests Spring Cache manager implementation that uses thin client to connect to the Ignite cluster. */ | ||
| public class IgniteClientSpringCacheManagerTest extends GridSpringCacheManagerAbstractTest { |
There was a problem hiding this comment.
Please add this test to IgniteSpringCacheTestSuite
No description provided.