You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2020/11/23 13:33:17 UTC
[GitHub] [ignite-extensions] ololo3000 opened a new pull request #31: IGNITE-13722 Adds correct closing of Ignite resources.
ololo3000 opened a new pull request #31:
URL: https://github.com/apache/ignite-extensions/pull/31
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-extensions] ololo3000 commented on a change in pull request #31: IGNITE-13722 Adds correct closing of Spring Data Ignite resources.
Posted by GitBox <gi...@apache.org>.
ololo3000 commented on a change in pull request #31:
URL: https://github.com/apache/ignite-extensions/pull/31#discussion_r540323783
##########
File path: modules/spring-data-commons/src/main/java/org/apache/ignite/springdata/proxy/IgniteProxyImpl.java
##########
@@ -36,11 +38,34 @@ public IgniteProxyImpl(Ignite ignite) {
/** {@inheritDoc} */
@Override public <K, V> IgniteCacheProxy<K, V> cache(String name) {
- return new IgniteCacheProxyImpl<>(ignite.cache(name));
+ IgniteCache<K, V> cache = ignite.cache(name);
+
+ return cache == null ? null : new IgniteCacheProxyImpl<>(cache);
}
/** @return {@link Ignite} instance to which operations are delegated. */
public Ignite delegate() {
return ignite;
}
+
+ /** {@inheritDoc} */
+ @Override public boolean equals(Object other) {
+ if (this == other)
+ return true;
+
+ if (other == null || getClass() != other.getClass())
+ return false;
+
+ return Objects.equals(ignite, ((IgniteProxyImpl)other).ignite);
+ }
+
+ /** {@inheritDoc} */
+ @Override public int hashCode() {
+ return ignite.hashCode();
+ }
+
+ /** {@inheritDoc} */
+ @Override public void close() {
+ ignite.close();
Review comment:
Done.
##########
File path: modules/spring-data-2.0-ext/src/main/java/org/apache/ignite/springdata20/repository/support/IgniteRepositoryFactoryBean.java
##########
@@ -62,6 +62,6 @@ protected IgniteRepositoryFactoryBean(Class<? extends T> repoInterface) {
/** {@inheritDoc} */
@Override protected RepositoryFactorySupport createRepositoryFactory() {
- return new IgniteRepositoryFactory(ctx);
+ return new IgniteRepositoryFactory(ctx, getObjectType());
Review comment:
I see the following problem: IgniteRepositoryFactory is created during IgniteRepositoryFactoryBean bean initialization. Therefore, if the initialization of the IgniteRepositoryFactoryBean fails with an error (for example, in case any query method has invalid configuration or some Spring bean internal dependencies issues), Spring will not call the "destroy" method, and since the Ignite instance was started earlier, it remains unclosed.
##########
File path: modules/spring-data-2.0-ext/src/main/java/org/apache/ignite/springdata20/repository/support/IgniteProxyFactory.java
##########
@@ -0,0 +1,172 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.springdata20.repository.support;
+
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import org.apache.ignite.Ignite;
+import org.apache.ignite.Ignition;
+import org.apache.ignite.client.IgniteClient;
+import org.apache.ignite.configuration.ClientConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.springdata.proxy.IgniteClientProxy;
+import org.apache.ignite.springdata.proxy.IgniteProxy;
+import org.apache.ignite.springdata.proxy.IgniteProxyImpl;
+import org.apache.ignite.springdata20.repository.config.RepositoryConfig;
+import org.springframework.beans.BeansException;
+import org.springframework.beans.factory.DisposableBean;
+import org.springframework.beans.factory.config.BeanExpressionContext;
+import org.springframework.beans.factory.config.BeanExpressionResolver;
+import org.springframework.beans.factory.support.DefaultListableBeanFactory;
+import org.springframework.context.ApplicationContext;
+import org.springframework.context.ApplicationContextAware;
+import org.springframework.context.expression.StandardBeanExpressionResolver;
+
+import static org.apache.ignite.springdata20.repository.support.IgniteRepositoryFactory.getRepositoryConfiguration;
+
+/**
+ * Represents factory for obtaining instances of {@link IgniteProxy} that provide client-independent connection to the
+ * Ignite cluster.
+ */
+public class IgniteProxyFactory implements ApplicationContextAware, DisposableBean {
+ /** Spring application expression resolver. */
+ private final BeanExpressionResolver expressionResolver = new StandardBeanExpressionResolver();
+
+ /** Repositories associated with Ignite proxy. */
+ private final Map<Class<?>, IgniteProxy> igniteProxies = new ConcurrentHashMap<>();
+
+ /** Spring application context. */
+ private ApplicationContext ctx;
+
+ /** Spring application bean expression context. */
+ private BeanExpressionContext beanExpressionCtx;
+
+ /**
+ * @param repoInterface The repository interface class for which {@link IgniteProxy} will be created.
+ * @return {@link IgniteProxy} instance.
+ */
+ public IgniteProxy igniteProxy(Class<?> repoInterface) {
+ return igniteProxies.computeIfAbsent(repoInterface, k -> createIgniteProxy(repoInterface));
+ }
+
+ /** {@inheritDoc} */
+ @Override public void setApplicationContext(ApplicationContext ctx) throws BeansException {
+ this.ctx = ctx;
+
+ beanExpressionCtx = new BeanExpressionContext(
+ new DefaultListableBeanFactory(ctx.getAutowireCapableBeanFactory()),
+ null);
+ }
+
+ /** {@inheritDoc} */
+ @Override public void destroy() throws Exception {
+ Set<IgniteProxy> proxies = new HashSet<>(igniteProxies.values());
+
+ Exception destroyE = null;
+
+ for (IgniteProxy proxy : proxies) {
+ try {
+ proxy.close();
+ }
+ catch (Exception e) {
+ if (destroyE == null)
+ destroyE = e;
+ else
+ destroyE.addSuppressed(e);
+ }
+ }
+
+ if (destroyE != null)
+ throw destroyE;
+ }
+
+ /**
+ * Creates {@link IgniteProxy} to be used for providing access to the Ignite cluster for specified repository.
+ *
+ * @param repoInterface {@link Class} instance of the repository interface.
+ * @return Instance of {@link IgniteProxy} associated with specified repository.
+ *
+ * @see RepositoryConfig
+ */
+ private IgniteProxy createIgniteProxy(Class<?> repoInterface) {
+ RepositoryConfig repoCfg = getRepositoryConfiguration(repoInterface);
+
+ try {
+ Object igniteInstanceBean = ctx.getBean(evaluateExpression(repoCfg.igniteInstance()));
+
+ if (igniteInstanceBean instanceof Ignite)
+ return new IgniteProxyImpl((Ignite)igniteInstanceBean);
+ else if (igniteInstanceBean instanceof IgniteClient)
+ return new IgniteClientProxy((IgniteClient)igniteInstanceBean);
+
+ throw new IllegalArgumentException("Invalid configuration for repository " + repoInterface.getName() +
+ ". The Spring Bean corresponding to the \"igniteInstance\" property of repository configuration must" +
+ " be one of the following types: [" + Ignite.class.getName() + ", " + IgniteClient.class.getName() +
+ ']');
+ }
+ catch (BeansException ex) {
+ try {
+ Object igniteCfgBean = ctx.getBean(evaluateExpression(repoCfg.igniteCfg()));
+
+ if (igniteCfgBean instanceof IgniteConfiguration) {
+ try {
+ return new IgniteProxyImpl(Ignition.ignite(
+ ((IgniteConfiguration)igniteCfgBean).getIgniteInstanceName()));
+ }
+ catch (Exception ignored) {
+ // No-op.
+ }
+
+ return new IgniteProxyImpl(Ignition.start((IgniteConfiguration)igniteCfgBean));
+ }
+ else if (igniteCfgBean instanceof ClientConfiguration)
+ return new IgniteClientProxy(Ignition.startClient((ClientConfiguration)igniteCfgBean));
+
+ throw new IllegalArgumentException("Invalid configuration for repository " + repoInterface.getName() +
+ ". The Spring Bean corresponding to the \"igniteCfg\" property of repository configuration must" +
+ " be one of the following types: [" + IgniteConfiguration.class.getName() + ", " +
+ ClientConfiguration.class.getName() + ']');
+ }
+ catch (BeansException ex2) {
+ try {
+ String igniteCfgPath = ctx.getBean(evaluateExpression(repoCfg.igniteSpringCfgPath()), String.class);
+
+ return new IgniteProxyImpl(Ignition.start(igniteCfgPath));
+ }
+ catch (BeansException ex3) {
+ throw new IllegalArgumentException("Invalid configuration for repository " +
+ repoInterface.getName() + ". No beans were found that provide connection configuration to the" +
+ " Ignite cluster. Check \"igniteInstance\", \"igniteCfg\", \"igniteSpringCfgPath\" parameters" +
+ " of " + RepositoryConfig.class.getName() + " repository annotation.");
+ }
Review comment:
Done.
##########
File path: modules/spring-data-2.0-ext/src/main/java/org/apache/ignite/springdata20/repository/config/EnableIgniteRepositories.java
##########
@@ -116,4 +118,10 @@
* repositories infrastructure.
*/
boolean considerNestedRepositories() default false;
+
+ /**
+ * @return Implementation of the Ignite proxy factory to be used to create {@link IgniteProxy} instances
+ * that provide access to the Ignite cluster for each Spring Data repository.
+ */
+ Class<?> igniteProxyFactoryClass() default IgniteProxyFactory.class;
Review comment:
Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-extensions] alex-plekhanov commented on a change in pull request #31: IGNITE-13722 Adds correct closing of Spring Data Ignite resources.
Posted by GitBox <gi...@apache.org>.
alex-plekhanov commented on a change in pull request #31:
URL: https://github.com/apache/ignite-extensions/pull/31#discussion_r542137250
##########
File path: modules/spring-data-commons/src/main/java/org/apache/ignite/springdata/proxy/IgniteClientProxy.java
##########
@@ -38,4 +39,20 @@ public IgniteClientProxy(IgniteClient cli) {
@Override public <K, V> IgniteCacheProxy<K, V> cache(String name) {
return new IgniteCacheClientProxy<>(cli.cache(name));
Review comment:
We should return `null` in case of cache is `null` (the same way as for `Ignite IgniteProxyImpl`)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-extensions] timoninmaxim commented on a change in pull request #31: IGNITE-13722 Adds correct closing of Spring Data Ignite resources.
Posted by GitBox <gi...@apache.org>.
timoninmaxim commented on a change in pull request #31:
URL: https://github.com/apache/ignite-extensions/pull/31#discussion_r532592714
##########
File path: modules/spring-data-commons/src/main/java/org/apache/ignite/springdata/proxy/ClosableIgniteClientProxy.java
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.springdata.proxy;
+
+import org.apache.ignite.client.IgniteClient;
+
+/** Extends {@link IgniteClientProxy} with the ability to close underlying thin client instance. */
+public class ClosableIgniteClientProxy extends IgniteClientProxy implements AutoCloseable {
Review comment:
ClientProxy is Closable while Auto is Clos**e**able. Let's name it the same way.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-extensions] ololo3000 commented on a change in pull request #31: IGNITE-13722 Adds correct closing of Spring Data Ignite resources.
Posted by GitBox <gi...@apache.org>.
ololo3000 commented on a change in pull request #31:
URL: https://github.com/apache/ignite-extensions/pull/31#discussion_r531237989
##########
File path: modules/spring-data-2.0-ext/src/test/java/org/apache/ignite/springdata/IgniteSpringDataConfigurationTest.java
##########
@@ -0,0 +1,221 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.springdata;
+
+import java.io.Serializable;
+import org.apache.ignite.Ignite;
+import org.apache.ignite.Ignition;
+import org.apache.ignite.configuration.CacheConfiguration;
+import org.apache.ignite.configuration.ClientConfiguration;
+import org.apache.ignite.configuration.ClientConnectorConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.spi.discovery.tcp.TcpDiscoverySpi;
+import org.apache.ignite.spi.discovery.tcp.ipfinder.TcpDiscoveryIpFinder;
+import org.apache.ignite.spi.discovery.tcp.ipfinder.vm.TcpDiscoveryVmIpFinder;
+import org.apache.ignite.springdata.misc.IgniteClientConfigRepository;
+import org.apache.ignite.springdata.misc.IgniteConfigRepository;
+import org.apache.ignite.springdata.misc.IgniteSpringConfigRepository;
+import org.apache.ignite.springdata20.repository.IgniteRepository;
+import org.apache.ignite.springdata20.repository.config.EnableIgniteRepositories;
+import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest;
+import org.junit.Test;
+import org.springframework.context.annotation.AnnotationConfigApplicationContext;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.ComponentScan.Filter;
+import org.springframework.context.annotation.Configuration;
+
+import static org.apache.ignite.testframework.GridTestUtils.assertThrowsAnyCause;
+import static org.springframework.context.annotation.FilterType.ASSIGNABLE_TYPE;
+
+/** Tests Sprign Data cluster access configurations. */
+public class IgniteSpringDataConfigurationTest extends GridCommonAbstractTest {
+ /** */
+ private static final TcpDiscoveryIpFinder IP_FINDER = new TcpDiscoveryVmIpFinder(true);
+
+ /** Tests repository configuration in case {@link IgniteConfiguration} is used to access the Ignite cluster. */
+ @Test
+ public void testRepositoryWithIgniteConfiguration() {
+ checkRepositoryConfiguration(IgniteConfigurationApplication.class, IgniteConfigRepository.class);
+ }
+
+ /** Tests repository configuration in case Spring configuration path is used to access the Ignite cluster. */
+ @Test
+ public void testRepositoryWithIgniteSpringConfiguration() {
+ checkRepositoryConfiguration(SpringConfigurationApplication.class, IgniteSpringConfigRepository.class);
+ }
+
+ /** Tests repository configuration in case {@link ClientConfiguration} is used to access the Ignite cluster.*/
+ @Test
+ public void testRepositoryWithClientConfiguration() {
+ checkRepositoryConfiguration(ClientConfigurationApplication.class, IgniteClientConfigRepository.class);
+ }
+
+ /** Tests repository configuration in case specified cache name is invalid. */
+ @Test
+ public void testRepositoryWithInvalidCacheNameConfiguration() {
+ try (AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext()) {
+ ctx.register(InvalidCacheNameApplication.class);
+
+ assertThrowsAnyCause(log,
+ () -> {
+ ctx.refresh();
+
+ return null;
+ },
+ IllegalArgumentException.class,
+ "Cache 'PersonCache' not found for repository interface" +
+ " org.apache.ignite.springdata.misc.IgniteConfigRepository. Please, add a cache configuration to" +
+ " ignite configuration or pass autoCreateCache=true to" +
+ " org.apache.ignite.springdata20.repository.config.RepositoryConfig annotation.");
+ }
+
+ assertTrue(Ignition.allGrids().isEmpty());
+ }
+
+ /** */
+ private void checkRepositoryConfiguration(
+ Class<?> cfgCls,
+ Class<? extends IgniteRepository<Object, Serializable>> repoCls
+ ) {
+ try (AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext()) {
+ ctx.register(cfgCls);
+ ctx.refresh();
+
+ IgniteRepository<Object, Serializable> repo = ctx.getBean(repoCls);
+
+ repo.save(1, "1");
+
+ assertTrue(repo.count() > 0);
+ }
+
+ assertTrue(Ignition.allGrids().isEmpty());
+ }
+
+ /**
+ * Spring Application configuration for repository testing in case {@link IgniteConfiguration} is used
+ * for accessing the cluster.
+ */
+ @Configuration
+ @EnableIgniteRepositories(
+ value = "org.apache.ignite.springdata.misc",
+ includeFilters = @Filter(type = ASSIGNABLE_TYPE, classes = IgniteConfigRepository.class))
+ public static class InvalidCacheNameApplication {
+ /** */
+ @Bean
+ public Ignite igniteServerNode() {
+ return Ignition.start(getIgniteConfiguration("srv-node", false));
+ }
+
+ /** Ignite configuration bean. */
+ @Bean
+ public IgniteConfiguration igniteConfiguration() {
+ return getIgniteConfiguration("cli-node", true);
+ }
+
+ /** */
+ private IgniteConfiguration getIgniteConfiguration(String name, boolean clientMode) {
+ return new IgniteConfiguration()
+ .setIgniteInstanceName(name)
+ .setClientMode(clientMode)
+ .setDiscoverySpi(new TcpDiscoverySpi().setIpFinder(IP_FINDER));
+ }
+ }
+
+ /**
+ * Spring Application configuration for repository testing in case Spring configuration is used
+ * for accessing the cluster.
+ */
+ @Configuration
+ @EnableIgniteRepositories(
+ value = "org.apache.ignite.springdata.misc",
+ includeFilters = @Filter(type = ASSIGNABLE_TYPE, classes = IgniteSpringConfigRepository.class))
+ public static class SpringConfigurationApplication {
+ /** */
+ @Bean
+ public Ignite igniteServerNode() {
+ return Ignition.start(new IgniteConfiguration()
+ .setIgniteInstanceName("srv-node")
+ .setDiscoverySpi(new TcpDiscoverySpi().setIpFinder(new TcpDiscoveryVmIpFinder(true))));
+ }
+
+ /** Ignite Spring configuration path bean. */
+ @Bean
+ public String igniteConfiguration() {
+ return "repository-ignite-config.xml";
+ }
+ }
+
+ /**
+ * Spring Application configuration for repository testing in case {@link IgniteConfiguration} is used
+ * for accessing the cluster.
+ */
+ @Configuration
+ @EnableIgniteRepositories(
+ value = "org.apache.ignite.springdata.misc",
+ includeFilters = @Filter(type = ASSIGNABLE_TYPE, classes = IgniteConfigRepository.class))
+ public static class IgniteConfigurationApplication {
+ /** */
+ @Bean
+ public Ignite igniteServerNode() {
+ return Ignition.start(getIgniteConfiguration("srv-node", false));
+ }
+
+ /** Ignite configuration bean. */
+ @Bean
+ public IgniteConfiguration igniteConfiguration() {
+ return getIgniteConfiguration("cli-node", true);
+ }
+
+ /** */
+ private IgniteConfiguration getIgniteConfiguration(String name, boolean clientMode) {
+ return new IgniteConfiguration()
+ .setIgniteInstanceName(name)
+ .setClientMode(clientMode)
+ .setDiscoverySpi(new TcpDiscoverySpi().setIpFinder(IP_FINDER))
+ .setCacheConfiguration(new CacheConfiguration<>("PersonCache"));
+ }
+ }
+
+ /**
+ * Spring Application configuration for repository testing in case {@link ClientConfiguration} is used
+ * for accessing the cluster.
+ */
+ @Configuration
+ @EnableIgniteRepositories(
+ value = "org.apache.ignite.springdata.misc",
+ includeFilters = @Filter(type = ASSIGNABLE_TYPE, classes = IgniteClientConfigRepository.class))
+ public static class ClientConfigurationApplication {
+ /** */
+ private static final int CLI_CONN_PORT = 10810;
+
+ /** */
+ @Bean
+ public Ignite igniteServerNode() {
+ return Ignition.start(new IgniteConfiguration()
+ .setDiscoverySpi(new TcpDiscoverySpi().setIpFinder(new TcpDiscoveryVmIpFinder(true)))
+ .setClientConnectorConfiguration(new ClientConnectorConfiguration().setPort(CLI_CONN_PORT))
+ .setCacheConfiguration(new CacheConfiguration<>("PersonCache")));
+ }
+
+ /** Ignite client configuraition bean. */
Review comment:
Done.
##########
File path: modules/spring-data-ext/src/test/java/org/apache/ignite/springdata/IgniteSpringDataConfigurationTest.java
##########
@@ -0,0 +1,215 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.springdata;
+
+import org.apache.ignite.Ignite;
+import org.apache.ignite.Ignition;
+import org.apache.ignite.configuration.CacheConfiguration;
+import org.apache.ignite.configuration.ClientConfiguration;
+import org.apache.ignite.configuration.ClientConnectorConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.spi.discovery.tcp.TcpDiscoverySpi;
+import org.apache.ignite.spi.discovery.tcp.ipfinder.TcpDiscoveryIpFinder;
+import org.apache.ignite.spi.discovery.tcp.ipfinder.vm.TcpDiscoveryVmIpFinder;
+import org.apache.ignite.springdata.misc.InvalidCacheRepository;
+import org.apache.ignite.springdata.misc.Person;
+import org.apache.ignite.springdata.misc.PersonRepository;
+import org.apache.ignite.springdata.repository.config.EnableIgniteRepositories;
+import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest;
+import org.junit.Test;
+import org.springframework.context.annotation.AnnotationConfigApplicationContext;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.ComponentScan.Filter;
+import org.springframework.context.annotation.Configuration;
+
+import static org.apache.ignite.testframework.GridTestUtils.assertThrowsAnyCause;
+import static org.springframework.context.annotation.FilterType.ASSIGNABLE_TYPE;
+
+/** Tests Sprign Data cluster access configurations. */
+public class IgniteSpringDataConfigurationTest extends GridCommonAbstractTest {
+ /** */
+ private static final TcpDiscoveryIpFinder IP_FINDER = new TcpDiscoveryVmIpFinder(true);
+
+ /** Tests repository configuration in case {@link IgniteConfiguration} is used to access the Ignite cluster. */
+ @Test
+ public void testRepositoryWithIgniteConfiguration() {
+ checkRepositoryConfiguration(IgniteConfigurationApplication.class);
+ }
+
+ /** Tests repository configuration in case Spring configuration path is used to access the Ignite cluster. */
+ @Test
+ public void testRepositoryWithIgniteSpringConfiguration() {
+ checkRepositoryConfiguration(SpringConfigurationApplication.class);
+ }
+
+ /** Tests repository configuration in case {@link ClientConfiguration} is used to access the Ignite cluster.*/
+ @Test
+ public void testRepositoryWithClientConfiguration() {
+ checkRepositoryConfiguration(ClientConfigurationApplication.class);
+ }
+
+ /** Tests repository configuration in case specified cache name is invalid. */
+ @Test
+ public void testRepositoryWithInvalidCacheNameConfiguration() {
+ try (AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext()) {
+ ctx.register(InvalidCacheNameApplication.class);
+
+ assertThrowsAnyCause(log,
+ () -> {
+ ctx.refresh();
+
+ return null;
+ },
+ IllegalArgumentException.class,
+ "Set a name of an Apache Ignite cache using" +
+ " org.apache.ignite.springdata.repository.config.RepositoryConfig annotation to map this" +
+ " repository to the underlying cache.");
+ }
+
+ assertTrue(Ignition.allGrids().isEmpty());
+ }
+
+ /** */
+ private void checkRepositoryConfiguration(Class<?> cfgCls) {
+ try (AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext()) {
+ ctx.register(cfgCls);
+ ctx.refresh();
+
+ PersonRepository repo = ctx.getBean(PersonRepository.class);
+
+ repo.save(1, new Person("Domenico", "Scarlatti"));
+
+ assertTrue(repo.count() > 0);
+ }
+
+ assertTrue(Ignition.allGrids().isEmpty());
+ }
+
+ /**
+ * Spring Application configuration for repository testing in case {@link IgniteConfiguration} is used
+ * for accessing the cluster.
+ */
+ @Configuration
+ @EnableIgniteRepositories(
+ value = "org.apache.ignite.springdata.misc",
+ includeFilters = @Filter(type = ASSIGNABLE_TYPE, classes = PersonRepository.class))
+ public static class IgniteConfigurationApplication {
+ /** */
+ @Bean
+ public Ignite igniteServerNode() {
+ return Ignition.start(getIgniteConfiguration("srv-node", false));
+ }
+
+ /** Ignite configuration bean. */
+ @Bean
+ public IgniteConfiguration igniteCfg() {
+ return getIgniteConfiguration("cli-node", true);
+ }
+
+ /** */
+ private IgniteConfiguration getIgniteConfiguration(String name, boolean clientMode) {
+ return new IgniteConfiguration()
+ .setIgniteInstanceName(name)
+ .setClientMode(clientMode)
+ .setDiscoverySpi(new TcpDiscoverySpi().setIpFinder(IP_FINDER))
+ .setCacheConfiguration(new CacheConfiguration<>("PersonCache"));
+ }
+ }
+
+ /**
+ * Spring Application configuration for repository testing in case {@link IgniteConfiguration} is used
+ * for accessing the cluster.
+ */
+ @Configuration
+ @EnableIgniteRepositories(
+ value = "org.apache.ignite.springdata.misc",
+ includeFilters = @Filter(type = ASSIGNABLE_TYPE, classes = InvalidCacheRepository.class))
+ public static class InvalidCacheNameApplication {
+ /** */
+ @Bean
+ public Ignite igniteServerNode() {
+ return Ignition.start(getIgniteConfiguration("srv-node", false));
+ }
+
+ /** Ignite configuration bean. */
+ @Bean
+ public IgniteConfiguration igniteCfg() {
+ return getIgniteConfiguration("cli-node", true);
+ }
+
+ /** */
+ private IgniteConfiguration getIgniteConfiguration(String name, boolean clientMode) {
+ return new IgniteConfiguration()
+ .setIgniteInstanceName(name)
+ .setClientMode(clientMode)
+ .setDiscoverySpi(new TcpDiscoverySpi().setIpFinder(IP_FINDER));
+ }
+ }
+
+ /**
+ * Spring Application configuration for repository testing in case Spring configuration is used
+ * for accessing the cluster.
+ */
+ @Configuration
+ @EnableIgniteRepositories(
+ value = "org.apache.ignite.springdata.misc",
+ includeFilters = @Filter(type = ASSIGNABLE_TYPE, classes = PersonRepository.class))
+ public static class SpringConfigurationApplication {
+ /** */
+ @Bean
+ public Ignite igniteServerNode() {
+ return Ignition.start(new IgniteConfiguration()
+ .setIgniteInstanceName("srv-node")
+ .setDiscoverySpi(new TcpDiscoverySpi().setIpFinder(new TcpDiscoveryVmIpFinder(true))));
Review comment:
Done.
##########
File path: modules/spring-data-2.0-ext/src/main/java/org/apache/ignite/springdata20/repository/support/IgniteResourceProvider.java
##########
@@ -0,0 +1,31 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.springdata20.repository.support;
+
+import org.apache.ignite.springdata.proxy.IgniteProxy;
+import org.apache.ignite.springdata20.repository.config.RepositoryConfig;
+
+/** Represents interdace for providing the resources required to access the Ignite cluster for each repository. */
Review comment:
Done.
##########
File path: modules/spring-data-2.0-ext/src/main/java/org/apache/ignite/springdata20/repository/support/IgniteRepositoryFactory.java
##########
@@ -62,85 +52,45 @@
* @author Manuel Núñez (manuel.nunez@hawkore.com)
*/
public class IgniteRepositoryFactory extends RepositoryFactorySupport {
- /** Spring application context */
- private final ApplicationContext ctx;
-
- /** Spring application bean factory */
- private final DefaultListableBeanFactory beanFactory;
-
/** Spring application expression resolver */
private final StandardBeanExpressionResolver resolver = new StandardBeanExpressionResolver();
/** Spring application bean expression context */
private final BeanExpressionContext beanExpressionContext;
- /** Mapping of a repository to a cache. */
- private final Map<Class<?>, String> repoToCache = new HashMap<>();
+ /** Ignite cache proxy instance associated with the current repository. */
+ private final IgniteCacheProxy<?, ?> cache;
- /** Mapping of a repository to a ignite instance. */
- private final Map<Class<?>, IgniteProxy> repoToIgnite = new HashMap<>();
+ /** Ignite proxy instance associated with the current repository. */
+ private final IgniteProxy ignite;
Review comment:
Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-extensions] ololo3000 commented on a change in pull request #31: IGNITE-13722 Adds correct closing of Spring Data Ignite resources.
Posted by GitBox <gi...@apache.org>.
ololo3000 commented on a change in pull request #31:
URL: https://github.com/apache/ignite-extensions/pull/31#discussion_r532668373
##########
File path: modules/spring-data-2.0-ext/src/main/java/org/apache/ignite/springdata20/repository/support/IgniteRepositoryFactoryBean.java
##########
@@ -22,6 +22,7 @@
import org.apache.ignite.configuration.IgniteConfiguration;
import org.apache.ignite.springdata20.repository.IgniteRepository;
import org.springframework.beans.BeansException;
+import org.springframework.beans.factory.annotation.Autowired;
Review comment:
Done.
##########
File path: modules/spring-data-2.0-ext/src/main/java/org/apache/ignite/springdata20/repository/config/IgniteRepositoryConfigurationExtension.java
##########
@@ -18,15 +18,28 @@
import java.util.Collection;
import java.util.Collections;
+import org.apache.ignite.springdata.proxy.IgniteProxy;
import org.apache.ignite.springdata20.repository.IgniteRepository;
import org.apache.ignite.springdata20.repository.support.IgniteRepositoryFactoryBean;
+import org.springframework.beans.factory.support.BeanDefinitionBuilder;
+import org.springframework.beans.factory.support.BeanDefinitionRegistry;
+import org.springframework.data.repository.config.AnnotationRepositoryConfigurationSource;
import org.springframework.data.repository.config.RepositoryConfigurationExtension;
import org.springframework.data.repository.config.RepositoryConfigurationExtensionSupport;
+import org.springframework.data.repository.config.RepositoryConfigurationSource;
+
+import static org.springframework.beans.factory.config.ConfigurableBeanFactory.SCOPE_PROTOTYPE;
/**
* Apache Ignite specific implementation of {@link RepositoryConfigurationExtension}.
*/
public class IgniteRepositoryConfigurationExtension extends RepositoryConfigurationExtensionSupport {
+ /** Name of the auto-registered Ignite proxy factory bean. */
+ private static final String IGNITE_PROXY_FACTORY_BEAN_NAME = "igniteResourceProvider";
Review comment:
Done.
##########
File path: modules/spring-data-commons/src/main/java/org/apache/ignite/springdata/proxy/ClosableIgniteClientProxy.java
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.springdata.proxy;
+
+import org.apache.ignite.client.IgniteClient;
+
+/** Extends {@link IgniteClientProxy} with the ability to close underlying thin client instance. */
+public class ClosableIgniteClientProxy extends IgniteClientProxy implements AutoCloseable {
Review comment:
Classes were removed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-extensions] ololo3000 commented on a change in pull request #31: IGNITE-13722 Adds correct closing of Spring Data Ignite resources.
Posted by GitBox <gi...@apache.org>.
ololo3000 commented on a change in pull request #31:
URL: https://github.com/apache/ignite-extensions/pull/31#discussion_r542336852
##########
File path: modules/spring-data-commons/src/main/java/org/apache/ignite/springdata/proxy/IgniteClientProxy.java
##########
@@ -38,4 +39,20 @@ public IgniteClientProxy(IgniteClient cli) {
@Override public <K, V> IgniteCacheProxy<K, V> cache(String name) {
return new IgniteCacheClientProxy<>(cli.cache(name));
Review comment:
It seems that the ClientCache instance obtained through the client API cannot be null in the current implementation.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-extensions] timoninmaxim commented on a change in pull request #31: IGNITE-13722 Adds correct closing of Spring Data Ignite resources.
Posted by GitBox <gi...@apache.org>.
timoninmaxim commented on a change in pull request #31:
URL: https://github.com/apache/ignite-extensions/pull/31#discussion_r531093713
##########
File path: modules/spring-data-ext/src/test/java/org/apache/ignite/springdata/misc/InvalidCacheRepository.java
##########
@@ -0,0 +1,27 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.springdata.misc;
+
+import org.apache.ignite.springdata.repository.IgniteRepository;
+import org.apache.ignite.springdata.repository.config.RepositoryConfig;
+
+/** Repository for testing application behavior in case the cache name is not specified in the repository configuration. */
Review comment:
There and above, please use Ignite code guidelines for javadocs, one-liner javadocs are used only for fields, check the [guide](https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines#CodingGuidelines-Shorterone-lineJavadoc)
##########
File path: modules/spring-data-ext/src/test/java/org/apache/ignite/springdata/IgniteSpringDataConfigurationTest.java
##########
@@ -0,0 +1,215 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.springdata;
+
+import org.apache.ignite.Ignite;
+import org.apache.ignite.Ignition;
+import org.apache.ignite.configuration.CacheConfiguration;
+import org.apache.ignite.configuration.ClientConfiguration;
+import org.apache.ignite.configuration.ClientConnectorConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.spi.discovery.tcp.TcpDiscoverySpi;
+import org.apache.ignite.spi.discovery.tcp.ipfinder.TcpDiscoveryIpFinder;
+import org.apache.ignite.spi.discovery.tcp.ipfinder.vm.TcpDiscoveryVmIpFinder;
+import org.apache.ignite.springdata.misc.InvalidCacheRepository;
+import org.apache.ignite.springdata.misc.Person;
+import org.apache.ignite.springdata.misc.PersonRepository;
+import org.apache.ignite.springdata.repository.config.EnableIgniteRepositories;
+import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest;
+import org.junit.Test;
+import org.springframework.context.annotation.AnnotationConfigApplicationContext;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.ComponentScan.Filter;
+import org.springframework.context.annotation.Configuration;
+
+import static org.apache.ignite.testframework.GridTestUtils.assertThrowsAnyCause;
+import static org.springframework.context.annotation.FilterType.ASSIGNABLE_TYPE;
+
+/** Tests Sprign Data cluster access configurations. */
Review comment:
s/Sprign/Spring/
##########
File path: modules/spring-data-2.0-ext/src/test/java/org/apache/ignite/springdata/IgniteSpringDataConfigurationTest.java
##########
@@ -0,0 +1,221 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.springdata;
+
+import java.io.Serializable;
+import org.apache.ignite.Ignite;
+import org.apache.ignite.Ignition;
+import org.apache.ignite.configuration.CacheConfiguration;
+import org.apache.ignite.configuration.ClientConfiguration;
+import org.apache.ignite.configuration.ClientConnectorConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.spi.discovery.tcp.TcpDiscoverySpi;
+import org.apache.ignite.spi.discovery.tcp.ipfinder.TcpDiscoveryIpFinder;
+import org.apache.ignite.spi.discovery.tcp.ipfinder.vm.TcpDiscoveryVmIpFinder;
+import org.apache.ignite.springdata.misc.IgniteClientConfigRepository;
+import org.apache.ignite.springdata.misc.IgniteConfigRepository;
+import org.apache.ignite.springdata.misc.IgniteSpringConfigRepository;
+import org.apache.ignite.springdata20.repository.IgniteRepository;
+import org.apache.ignite.springdata20.repository.config.EnableIgniteRepositories;
+import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest;
+import org.junit.Test;
+import org.springframework.context.annotation.AnnotationConfigApplicationContext;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.ComponentScan.Filter;
+import org.springframework.context.annotation.Configuration;
+
+import static org.apache.ignite.testframework.GridTestUtils.assertThrowsAnyCause;
+import static org.springframework.context.annotation.FilterType.ASSIGNABLE_TYPE;
+
+/** Tests Sprign Data cluster access configurations. */
+public class IgniteSpringDataConfigurationTest extends GridCommonAbstractTest {
+ /** */
+ private static final TcpDiscoveryIpFinder IP_FINDER = new TcpDiscoveryVmIpFinder(true);
+
+ /** Tests repository configuration in case {@link IgniteConfiguration} is used to access the Ignite cluster. */
+ @Test
+ public void testRepositoryWithIgniteConfiguration() {
+ checkRepositoryConfiguration(IgniteConfigurationApplication.class, IgniteConfigRepository.class);
+ }
+
+ /** Tests repository configuration in case Spring configuration path is used to access the Ignite cluster. */
+ @Test
+ public void testRepositoryWithIgniteSpringConfiguration() {
+ checkRepositoryConfiguration(SpringConfigurationApplication.class, IgniteSpringConfigRepository.class);
+ }
+
+ /** Tests repository configuration in case {@link ClientConfiguration} is used to access the Ignite cluster.*/
+ @Test
+ public void testRepositoryWithClientConfiguration() {
+ checkRepositoryConfiguration(ClientConfigurationApplication.class, IgniteClientConfigRepository.class);
+ }
+
+ /** Tests repository configuration in case specified cache name is invalid. */
+ @Test
+ public void testRepositoryWithInvalidCacheNameConfiguration() {
+ try (AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext()) {
+ ctx.register(InvalidCacheNameApplication.class);
+
+ assertThrowsAnyCause(log,
+ () -> {
+ ctx.refresh();
+
+ return null;
+ },
+ IllegalArgumentException.class,
+ "Cache 'PersonCache' not found for repository interface" +
+ " org.apache.ignite.springdata.misc.IgniteConfigRepository. Please, add a cache configuration to" +
+ " ignite configuration or pass autoCreateCache=true to" +
+ " org.apache.ignite.springdata20.repository.config.RepositoryConfig annotation.");
+ }
+
+ assertTrue(Ignition.allGrids().isEmpty());
+ }
+
+ /** */
+ private void checkRepositoryConfiguration(
+ Class<?> cfgCls,
+ Class<? extends IgniteRepository<Object, Serializable>> repoCls
+ ) {
+ try (AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext()) {
+ ctx.register(cfgCls);
+ ctx.refresh();
+
+ IgniteRepository<Object, Serializable> repo = ctx.getBean(repoCls);
+
+ repo.save(1, "1");
+
+ assertTrue(repo.count() > 0);
+ }
+
+ assertTrue(Ignition.allGrids().isEmpty());
+ }
+
+ /**
+ * Spring Application configuration for repository testing in case {@link IgniteConfiguration} is used
+ * for accessing the cluster.
+ */
+ @Configuration
+ @EnableIgniteRepositories(
+ value = "org.apache.ignite.springdata.misc",
+ includeFilters = @Filter(type = ASSIGNABLE_TYPE, classes = IgniteConfigRepository.class))
+ public static class InvalidCacheNameApplication {
+ /** */
+ @Bean
+ public Ignite igniteServerNode() {
+ return Ignition.start(getIgniteConfiguration("srv-node", false));
+ }
+
+ /** Ignite configuration bean. */
+ @Bean
+ public IgniteConfiguration igniteConfiguration() {
+ return getIgniteConfiguration("cli-node", true);
+ }
+
+ /** */
+ private IgniteConfiguration getIgniteConfiguration(String name, boolean clientMode) {
+ return new IgniteConfiguration()
+ .setIgniteInstanceName(name)
+ .setClientMode(clientMode)
+ .setDiscoverySpi(new TcpDiscoverySpi().setIpFinder(IP_FINDER));
+ }
+ }
+
+ /**
+ * Spring Application configuration for repository testing in case Spring configuration is used
+ * for accessing the cluster.
+ */
+ @Configuration
+ @EnableIgniteRepositories(
+ value = "org.apache.ignite.springdata.misc",
+ includeFilters = @Filter(type = ASSIGNABLE_TYPE, classes = IgniteSpringConfigRepository.class))
+ public static class SpringConfigurationApplication {
+ /** */
+ @Bean
+ public Ignite igniteServerNode() {
+ return Ignition.start(new IgniteConfiguration()
+ .setIgniteInstanceName("srv-node")
+ .setDiscoverySpi(new TcpDiscoverySpi().setIpFinder(new TcpDiscoveryVmIpFinder(true))));
+ }
+
+ /** Ignite Spring configuration path bean. */
+ @Bean
+ public String igniteConfiguration() {
+ return "repository-ignite-config.xml";
+ }
+ }
+
+ /**
+ * Spring Application configuration for repository testing in case {@link IgniteConfiguration} is used
+ * for accessing the cluster.
+ */
+ @Configuration
+ @EnableIgniteRepositories(
+ value = "org.apache.ignite.springdata.misc",
+ includeFilters = @Filter(type = ASSIGNABLE_TYPE, classes = IgniteConfigRepository.class))
+ public static class IgniteConfigurationApplication {
+ /** */
+ @Bean
+ public Ignite igniteServerNode() {
+ return Ignition.start(getIgniteConfiguration("srv-node", false));
+ }
+
+ /** Ignite configuration bean. */
+ @Bean
+ public IgniteConfiguration igniteConfiguration() {
+ return getIgniteConfiguration("cli-node", true);
+ }
+
+ /** */
+ private IgniteConfiguration getIgniteConfiguration(String name, boolean clientMode) {
+ return new IgniteConfiguration()
+ .setIgniteInstanceName(name)
+ .setClientMode(clientMode)
+ .setDiscoverySpi(new TcpDiscoverySpi().setIpFinder(IP_FINDER))
+ .setCacheConfiguration(new CacheConfiguration<>("PersonCache"));
+ }
+ }
+
+ /**
+ * Spring Application configuration for repository testing in case {@link ClientConfiguration} is used
+ * for accessing the cluster.
+ */
+ @Configuration
+ @EnableIgniteRepositories(
+ value = "org.apache.ignite.springdata.misc",
+ includeFilters = @Filter(type = ASSIGNABLE_TYPE, classes = IgniteClientConfigRepository.class))
+ public static class ClientConfigurationApplication {
+ /** */
+ private static final int CLI_CONN_PORT = 10810;
+
+ /** */
+ @Bean
+ public Ignite igniteServerNode() {
+ return Ignition.start(new IgniteConfiguration()
+ .setDiscoverySpi(new TcpDiscoverySpi().setIpFinder(new TcpDiscoveryVmIpFinder(true)))
+ .setClientConnectorConfiguration(new ClientConnectorConfiguration().setPort(CLI_CONN_PORT))
+ .setCacheConfiguration(new CacheConfiguration<>("PersonCache")));
+ }
+
+ /** Ignite client configuraition bean. */
Review comment:
s/configuraition/configuraiton/
##########
File path: modules/spring-data-2.0-ext/src/main/java/org/apache/ignite/springdata20/repository/support/IgniteResourceProvider.java
##########
@@ -0,0 +1,31 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.springdata20.repository.support;
+
+import org.apache.ignite.springdata.proxy.IgniteProxy;
+import org.apache.ignite.springdata20.repository.config.RepositoryConfig;
+
+/** Represents interdace for providing the resources required to access the Ignite cluster for each repository. */
Review comment:
Also, there are a lot of misprints that out of scope of the PR. Intellij IDEA supports spell checking, could you please fix all of them?
##########
File path: modules/spring-data-2.0-ext/src/main/java/org/apache/ignite/springdata20/repository/support/IgniteResourceProviderImpl.java
##########
@@ -0,0 +1,171 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.springdata20.repository.support;
+
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import org.apache.ignite.Ignite;
+import org.apache.ignite.Ignition;
+import org.apache.ignite.client.IgniteClient;
+import org.apache.ignite.configuration.ClientConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.springdata.proxy.ClosableIgniteClientProxy;
+import org.apache.ignite.springdata.proxy.ClosableIgniteProxy;
+import org.apache.ignite.springdata.proxy.IgniteClientProxy;
+import org.apache.ignite.springdata.proxy.IgniteProxy;
+import org.apache.ignite.springdata.proxy.IgniteProxyImpl;
+import org.apache.ignite.springdata20.repository.config.RepositoryConfig;
+import org.springframework.beans.BeansException;
+import org.springframework.beans.factory.DisposableBean;
+import org.springframework.beans.factory.config.BeanExpressionContext;
+import org.springframework.beans.factory.config.BeanExpressionResolver;
+import org.springframework.beans.factory.support.DefaultListableBeanFactory;
+import org.springframework.context.ApplicationContext;
+import org.springframework.context.ApplicationContextAware;
+import org.springframework.context.expression.StandardBeanExpressionResolver;
+import org.springframework.util.Assert;
+
+/** Represents default implementation of {@link IgniteResourceProvider} */
+public class IgniteResourceProviderImpl implements IgniteResourceProvider, ApplicationContextAware, DisposableBean {
+ /** Spring application expression resolver. */
+ private final BeanExpressionResolver expressionResolver = new StandardBeanExpressionResolver();
+
+ /** Ignite proxies associated with repositories. */
+ private final Map<Class<?>, IgniteProxy> igniteProxies = new ConcurrentHashMap<>();
+
+ /** Spring application context. */
+ private ApplicationContext ctx;
+
+ /** Spring application bean expression context. */
+ private BeanExpressionContext beanExpressionCtx;
+
+ /** {@inheritDoc} */
+ @Override public IgniteProxy igniteProxy(Class<?> repoInterface) {
+ return igniteProxies.computeIfAbsent(repoInterface, k -> createIgniteProxy(repoInterface));
+ }
+
+ /** {@inheritDoc} */
+ @Override public void setApplicationContext(ApplicationContext ctx) throws BeansException {
+ this.ctx = ctx;
+
+ beanExpressionCtx = new BeanExpressionContext(
+ new DefaultListableBeanFactory(ctx.getAutowireCapableBeanFactory()),
+ null);
+ }
+
+ /** {@inheritDoc} */
+ @Override public void destroy() throws Exception {
+ Set<IgniteProxy> proxies = new HashSet<>(igniteProxies.values());
+
+ for (IgniteProxy proxy : proxies) {
+ if (proxy instanceof AutoCloseable)
+ ((AutoCloseable)proxy).close();
+ }
+ }
+
+ /**
+ * Creates {@link IgniteProxy} to be used for providing access to the Ignite cluster for specified repository.
+ *
+ * @param repoInterface {@link Class} instance of the repository interface.
+ * @return Instance of {@link IgniteProxy} associated with specified repository.
+ *
+ * @see RepositoryConfig
+ */
+ private IgniteProxy createIgniteProxy(Class<?> repoInterface) {
+ RepositoryConfig repoCfg = getRepositoryConfiguration(repoInterface);
+
+ try {
+ Object igniteInstanceBean = ctx.getBean(evaluateExpression(repoCfg.igniteInstance()));
+
+ if (igniteInstanceBean instanceof Ignite)
+ return new IgniteProxyImpl((Ignite)igniteInstanceBean);
+ else if (igniteInstanceBean instanceof IgniteClient)
+ return new IgniteClientProxy((IgniteClient)igniteInstanceBean);
+
+ throw new IllegalArgumentException("Invalid repository configuration [name=" + repoInterface.getName() +
+ "]. The Spring Bean corresponding to the \"igniteInstance\" property of repository configuration must" +
+ " be one of the following types: [" + Ignite.class.getName() + ", " + IgniteClient.class.getName() +
+ ']');
+ }
+ catch (BeansException ex) {
Review comment:
I'm not sure about spring best practices, but usage of ctx.containsBean() looks better than build logic on exception handling. WDYT?
##########
File path: modules/spring-data-ext/src/test/resources/repository-ignite-config.xml
##########
@@ -0,0 +1,51 @@
+<?xml version="1.0" encoding="UTF-8"?>
+
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+
+<beans xmlns="http://www.springframework.org/schema/beans"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://www.springframework.org/schema/beans
+ http://www.springframework.org/schema/beans/spring-beans.xsd">
+ <bean id="ignite.cfg" class="org.apache.ignite.configuration.IgniteConfiguration">
+ <property name="igniteInstanceName" value="cli-node"/>
+
+ <property name="clientMode" value="true"/>
+
+ <property name="cacheConfiguration">
+ <list>
+ <bean class="org.apache.ignite.configuration.CacheConfiguration">
+ <property name="name" value="PersonCache"/>
+ </bean>
+ </list>
+ </property>
+
+ <property name="discoverySpi">
+ <bean class="org.apache.ignite.spi.discovery.tcp.TcpDiscoverySpi">
+ <property name="ipFinder">
+ <bean class="org.apache.ignite.spi.discovery.tcp.ipfinder.vm.TcpDiscoveryVmIpFinder">
+ <property name="addresses">
+ <list>
+ <value>127.0.0.1:47500..47509</value>
+ </list>
+ </property>
+ </bean>
+ </property>
+ </bean>
+ </property>
+ </bean>
+</beans>
Review comment:
No empty line in the end of file.
##########
File path: modules/spring-data-ext/src/test/java/org/apache/ignite/springdata/IgniteSpringDataCrudSelfTest.java
##########
@@ -93,6 +94,8 @@
/** {@inheritDoc} */
@Override protected void afterTestsStopped() {
ctx.destroy();
+
+ assertTrue(Ignition.allGrids().isEmpty());
Review comment:
There and below. Why do you place assertion there? Could you just provide a test case that covers this assert? Assertions in wrapper methods (before / after) are not part of tests and then may lead to unreadable junit report.
##########
File path: modules/spring-data-commons/src/main/java/org/apache/ignite/springdata/proxy/ClosableIgniteProxy.java
##########
@@ -0,0 +1,35 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.springdata.proxy;
+
+import org.apache.ignite.Ignite;
+
+/** Extends {@link IgniteProxyImpl} with the ability to close underlying Ignite instance. */
+public class ClosableIgniteProxy extends IgniteProxyImpl implements AutoCloseable {
Review comment:
Is there a case that requires non-closable IgniteProxy (also as underlying Ignite is auto closeable)?
##########
File path: modules/spring-data-ext/src/test/java/org/apache/ignite/springdata/IgniteSpringDataConfigurationTest.java
##########
@@ -0,0 +1,215 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.springdata;
+
+import org.apache.ignite.Ignite;
+import org.apache.ignite.Ignition;
+import org.apache.ignite.configuration.CacheConfiguration;
+import org.apache.ignite.configuration.ClientConfiguration;
+import org.apache.ignite.configuration.ClientConnectorConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.spi.discovery.tcp.TcpDiscoverySpi;
+import org.apache.ignite.spi.discovery.tcp.ipfinder.TcpDiscoveryIpFinder;
+import org.apache.ignite.spi.discovery.tcp.ipfinder.vm.TcpDiscoveryVmIpFinder;
+import org.apache.ignite.springdata.misc.InvalidCacheRepository;
+import org.apache.ignite.springdata.misc.Person;
+import org.apache.ignite.springdata.misc.PersonRepository;
+import org.apache.ignite.springdata.repository.config.EnableIgniteRepositories;
+import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest;
+import org.junit.Test;
+import org.springframework.context.annotation.AnnotationConfigApplicationContext;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.ComponentScan.Filter;
+import org.springframework.context.annotation.Configuration;
+
+import static org.apache.ignite.testframework.GridTestUtils.assertThrowsAnyCause;
+import static org.springframework.context.annotation.FilterType.ASSIGNABLE_TYPE;
+
+/** Tests Sprign Data cluster access configurations. */
+public class IgniteSpringDataConfigurationTest extends GridCommonAbstractTest {
+ /** */
+ private static final TcpDiscoveryIpFinder IP_FINDER = new TcpDiscoveryVmIpFinder(true);
+
+ /** Tests repository configuration in case {@link IgniteConfiguration} is used to access the Ignite cluster. */
+ @Test
+ public void testRepositoryWithIgniteConfiguration() {
+ checkRepositoryConfiguration(IgniteConfigurationApplication.class);
+ }
+
+ /** Tests repository configuration in case Spring configuration path is used to access the Ignite cluster. */
+ @Test
+ public void testRepositoryWithIgniteSpringConfiguration() {
+ checkRepositoryConfiguration(SpringConfigurationApplication.class);
+ }
+
+ /** Tests repository configuration in case {@link ClientConfiguration} is used to access the Ignite cluster.*/
+ @Test
+ public void testRepositoryWithClientConfiguration() {
+ checkRepositoryConfiguration(ClientConfigurationApplication.class);
+ }
+
+ /** Tests repository configuration in case specified cache name is invalid. */
+ @Test
+ public void testRepositoryWithInvalidCacheNameConfiguration() {
+ try (AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext()) {
+ ctx.register(InvalidCacheNameApplication.class);
+
+ assertThrowsAnyCause(log,
+ () -> {
+ ctx.refresh();
+
+ return null;
+ },
+ IllegalArgumentException.class,
+ "Set a name of an Apache Ignite cache using" +
+ " org.apache.ignite.springdata.repository.config.RepositoryConfig annotation to map this" +
+ " repository to the underlying cache.");
+ }
+
+ assertTrue(Ignition.allGrids().isEmpty());
+ }
+
+ /** */
+ private void checkRepositoryConfiguration(Class<?> cfgCls) {
+ try (AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext()) {
+ ctx.register(cfgCls);
+ ctx.refresh();
+
+ PersonRepository repo = ctx.getBean(PersonRepository.class);
+
+ repo.save(1, new Person("Domenico", "Scarlatti"));
+
+ assertTrue(repo.count() > 0);
+ }
+
+ assertTrue(Ignition.allGrids().isEmpty());
+ }
+
+ /**
+ * Spring Application configuration for repository testing in case {@link IgniteConfiguration} is used
+ * for accessing the cluster.
+ */
+ @Configuration
+ @EnableIgniteRepositories(
+ value = "org.apache.ignite.springdata.misc",
+ includeFilters = @Filter(type = ASSIGNABLE_TYPE, classes = PersonRepository.class))
+ public static class IgniteConfigurationApplication {
+ /** */
+ @Bean
+ public Ignite igniteServerNode() {
+ return Ignition.start(getIgniteConfiguration("srv-node", false));
+ }
+
+ /** Ignite configuration bean. */
+ @Bean
+ public IgniteConfiguration igniteCfg() {
+ return getIgniteConfiguration("cli-node", true);
+ }
+
+ /** */
+ private IgniteConfiguration getIgniteConfiguration(String name, boolean clientMode) {
+ return new IgniteConfiguration()
+ .setIgniteInstanceName(name)
+ .setClientMode(clientMode)
+ .setDiscoverySpi(new TcpDiscoverySpi().setIpFinder(IP_FINDER))
+ .setCacheConfiguration(new CacheConfiguration<>("PersonCache"));
+ }
+ }
+
+ /**
+ * Spring Application configuration for repository testing in case {@link IgniteConfiguration} is used
+ * for accessing the cluster.
+ */
+ @Configuration
+ @EnableIgniteRepositories(
+ value = "org.apache.ignite.springdata.misc",
+ includeFilters = @Filter(type = ASSIGNABLE_TYPE, classes = InvalidCacheRepository.class))
+ public static class InvalidCacheNameApplication {
+ /** */
+ @Bean
+ public Ignite igniteServerNode() {
+ return Ignition.start(getIgniteConfiguration("srv-node", false));
+ }
+
+ /** Ignite configuration bean. */
+ @Bean
+ public IgniteConfiguration igniteCfg() {
+ return getIgniteConfiguration("cli-node", true);
+ }
+
+ /** */
+ private IgniteConfiguration getIgniteConfiguration(String name, boolean clientMode) {
+ return new IgniteConfiguration()
+ .setIgniteInstanceName(name)
+ .setClientMode(clientMode)
+ .setDiscoverySpi(new TcpDiscoverySpi().setIpFinder(IP_FINDER));
+ }
+ }
+
+ /**
+ * Spring Application configuration for repository testing in case Spring configuration is used
+ * for accessing the cluster.
+ */
+ @Configuration
+ @EnableIgniteRepositories(
+ value = "org.apache.ignite.springdata.misc",
+ includeFilters = @Filter(type = ASSIGNABLE_TYPE, classes = PersonRepository.class))
+ public static class SpringConfigurationApplication {
+ /** */
+ @Bean
+ public Ignite igniteServerNode() {
+ return Ignition.start(new IgniteConfiguration()
+ .setIgniteInstanceName("srv-node")
+ .setDiscoverySpi(new TcpDiscoverySpi().setIpFinder(new TcpDiscoveryVmIpFinder(true))));
Review comment:
Could you use IP_FINDER variable there as in different tests?
##########
File path: modules/spring-data-2.0-ext/src/main/java/org/apache/ignite/springdata20/repository/support/IgniteRepositoryFactory.java
##########
@@ -62,85 +52,45 @@
* @author Manuel Núñez (manuel.nunez@hawkore.com)
*/
public class IgniteRepositoryFactory extends RepositoryFactorySupport {
- /** Spring application context */
- private final ApplicationContext ctx;
-
- /** Spring application bean factory */
- private final DefaultListableBeanFactory beanFactory;
-
/** Spring application expression resolver */
private final StandardBeanExpressionResolver resolver = new StandardBeanExpressionResolver();
/** Spring application bean expression context */
private final BeanExpressionContext beanExpressionContext;
- /** Mapping of a repository to a cache. */
- private final Map<Class<?>, String> repoToCache = new HashMap<>();
+ /** Ignite cache proxy instance associated with the current repository. */
+ private final IgniteCacheProxy<?, ?> cache;
- /** Mapping of a repository to a ignite instance. */
- private final Map<Class<?>, IgniteProxy> repoToIgnite = new HashMap<>();
+ /** Ignite proxy instance associated with the current repository. */
+ private final IgniteProxy ignite;
Review comment:
unused variable
##########
File path: modules/spring-data-ext/src/test/java/org/apache/ignite/springdata/misc/IgniteClientApplicationConfiguration.java
##########
@@ -27,13 +27,17 @@
import org.apache.ignite.internal.IgniteEx;
import org.apache.ignite.springdata.repository.config.EnableIgniteRepositories;
import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.ComponentScan;
import org.springframework.context.annotation.Configuration;
import static org.apache.ignite.springdata.compoundkey.CompoundKeyApplicationConfiguration.CLI_CONN_PORT;
+import static org.springframework.context.annotation.FilterType.ASSIGNABLE_TYPE;
/** Spring Application configuration for repository testing in case thin client is used for accessing the cluster. */
@Configuration
-@EnableIgniteRepositories({"org.apache.ignite.springdata.compoundkey", "org.apache.ignite.springdata.misc"})
+@EnableIgniteRepositories(
+ value = {"org.apache.ignite.springdata.compoundkey", "org.apache.ignite.springdata.misc"},
+ excludeFilters = @ComponentScan.Filter(type = ASSIGNABLE_TYPE, classes = InvalidCacheRepository.class))
Review comment:
Is it possible to simplify this with moving InvalidCacheRepository to different package?
##########
File path: modules/spring-data-2.0-ext/src/main/java/org/apache/ignite/springdata20/repository/support/IgniteResourceProvider.java
##########
@@ -0,0 +1,31 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.springdata20.repository.support;
+
+import org.apache.ignite.springdata.proxy.IgniteProxy;
+import org.apache.ignite.springdata20.repository.config.RepositoryConfig;
+
+/** Represents interdace for providing the resources required to access the Ignite cluster for each repository. */
Review comment:
s/interdace/interface/
##########
File path: modules/spring-data-ext/src/main/java/org/apache/ignite/springdata/repository/support/IgniteResourceProviderImpl.java
##########
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.springdata.repository.support;
+
+import org.apache.ignite.Ignite;
+import org.apache.ignite.Ignition;
+import org.apache.ignite.client.IgniteClient;
+import org.apache.ignite.configuration.ClientConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.springdata.proxy.ClosableIgniteClientProxy;
+import org.apache.ignite.springdata.proxy.ClosableIgniteProxy;
+import org.apache.ignite.springdata.proxy.IgniteClientProxy;
+import org.apache.ignite.springdata.proxy.IgniteProxy;
+import org.apache.ignite.springdata.proxy.IgniteProxyImpl;
+import org.apache.ignite.springdata.repository.config.RepositoryConfig;
+import org.springframework.beans.BeansException;
+import org.springframework.beans.factory.DisposableBean;
+import org.springframework.context.ApplicationContext;
+import org.springframework.context.ApplicationContextAware;
+
+/** Represents default implementation of {@link IgniteResourceProvider} */
+public class IgniteResourceProviderImpl implements IgniteResourceProvider, ApplicationContextAware, DisposableBean {
+ /** Spring application context. */
+ @SuppressWarnings("FieldAccessedSynchronizedAndUnsynchronized")
+ private ApplicationContext ctx;
+
+ /** Ignite proxy. */
+ private volatile IgniteProxy igniteProxy;
+
+ /** {@inheritDoc} */
+ @Override public IgniteProxy igniteProxy() {
+ IgniteProxy res = igniteProxy;
+
+ if (res == null) {
+ synchronized (this) {
+ res = igniteProxy;
+
+ if (res == null) {
+ res = createIgniteProxy();
+
+ igniteProxy = res;
+ }
+ }
+ }
+
+ return igniteProxy;
+ }
+
+ /** {@inheritDoc} */
+ @Override public void setApplicationContext(ApplicationContext ctx) throws BeansException {
+ this.ctx = ctx;
+ }
+
+ /** {@inheritDoc} */
+ @Override public void destroy() throws Exception {
+ IgniteProxy proxy = igniteProxy;
+
+ if (proxy instanceof AutoCloseable)
+ ((AutoCloseable)proxy).close();
+ }
+
+ /**
+ * Creates {@link IgniteProxy} to be used for providing access to the Ignite cluster for all repositories.
+ *
+ * @return Instance of {@link IgniteProxy}.
+ *
+ * @see RepositoryConfig
+ */
+ private IgniteProxy createIgniteProxy() {
+ try {
+ Object igniteInstanceBean = ctx.getBean("igniteInstance");
Review comment:
User may wants to run multiple instances of a client / server or both of them. Looks like it is a valid case, but ctx.getBean does not handle it, isn't it?
##########
File path: modules/spring-data-ext/src/main/java/org/apache/ignite/springdata/repository/support/IgniteResourceProviderImpl.java
##########
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.springdata.repository.support;
+
+import org.apache.ignite.Ignite;
+import org.apache.ignite.Ignition;
+import org.apache.ignite.client.IgniteClient;
+import org.apache.ignite.configuration.ClientConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.springdata.proxy.ClosableIgniteClientProxy;
+import org.apache.ignite.springdata.proxy.ClosableIgniteProxy;
+import org.apache.ignite.springdata.proxy.IgniteClientProxy;
+import org.apache.ignite.springdata.proxy.IgniteProxy;
+import org.apache.ignite.springdata.proxy.IgniteProxyImpl;
+import org.apache.ignite.springdata.repository.config.RepositoryConfig;
+import org.springframework.beans.BeansException;
+import org.springframework.beans.factory.DisposableBean;
+import org.springframework.context.ApplicationContext;
+import org.springframework.context.ApplicationContextAware;
+
+/** Represents default implementation of {@link IgniteResourceProvider} */
+public class IgniteResourceProviderImpl implements IgniteResourceProvider, ApplicationContextAware, DisposableBean {
Review comment:
Actually, what is a purpose of this class? Spring closes AutoCloseable beans by default, provides Lazy annotation for lazy initialization, routing by bean name looks arguable (and nevertheless earlier it was implemented different way).
##########
File path: modules/spring-data-commons/src/main/java/org/apache/ignite/springdata/proxy/ClosableIgniteProxy.java
##########
@@ -0,0 +1,35 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.springdata.proxy;
+
+import org.apache.ignite.Ignite;
+
+/** Extends {@link IgniteProxyImpl} with the ability to close underlying Ignite instance. */
+public class ClosableIgniteProxy extends IgniteProxyImpl implements AutoCloseable {
Review comment:
The same for ClientProxy that just holder of AutoCloseable IgniteClient
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-extensions] ololo3000 commented on a change in pull request #31: IGNITE-13722 Adds correct closing of Spring Data Ignite resources.
Posted by GitBox <gi...@apache.org>.
ololo3000 commented on a change in pull request #31:
URL: https://github.com/apache/ignite-extensions/pull/31#discussion_r531505827
##########
File path: modules/spring-data-ext/src/main/java/org/apache/ignite/springdata/repository/support/IgniteResourceProviderImpl.java
##########
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.springdata.repository.support;
+
+import org.apache.ignite.Ignite;
+import org.apache.ignite.Ignition;
+import org.apache.ignite.client.IgniteClient;
+import org.apache.ignite.configuration.ClientConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.springdata.proxy.ClosableIgniteClientProxy;
+import org.apache.ignite.springdata.proxy.ClosableIgniteProxy;
+import org.apache.ignite.springdata.proxy.IgniteClientProxy;
+import org.apache.ignite.springdata.proxy.IgniteProxy;
+import org.apache.ignite.springdata.proxy.IgniteProxyImpl;
+import org.apache.ignite.springdata.repository.config.RepositoryConfig;
+import org.springframework.beans.BeansException;
+import org.springframework.beans.factory.DisposableBean;
+import org.springframework.context.ApplicationContext;
+import org.springframework.context.ApplicationContextAware;
+
+/** Represents default implementation of {@link IgniteResourceProvider} */
+public class IgniteResourceProviderImpl implements IgniteResourceProvider, ApplicationContextAware, DisposableBean {
Review comment:
I think it's backward compatibility. Now we already have the functionality to configure a Spring Data repository just providing Ignite/ClientConfiguration bean name for each repository. Please, see RepositoryConfing#igniteCfg or igniteSpringCfgPath.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-extensions] ololo3000 commented on pull request #31: IGNITE-13722 Adds correct closing of Spring Data Ignite resources.
Posted by GitBox <gi...@apache.org>.
ololo3000 commented on pull request #31:
URL: https://github.com/apache/ignite-extensions/pull/31#issuecomment-744064222
@alex-plekhanov Could you, please, take a look?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-extensions] timoninmaxim commented on a change in pull request #31: IGNITE-13722 Adds correct closing of Spring Data Ignite resources.
Posted by GitBox <gi...@apache.org>.
timoninmaxim commented on a change in pull request #31:
URL: https://github.com/apache/ignite-extensions/pull/31#discussion_r531495407
##########
File path: modules/spring-data-ext/src/main/java/org/apache/ignite/springdata/repository/support/IgniteResourceProviderImpl.java
##########
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.springdata.repository.support;
+
+import org.apache.ignite.Ignite;
+import org.apache.ignite.Ignition;
+import org.apache.ignite.client.IgniteClient;
+import org.apache.ignite.configuration.ClientConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.springdata.proxy.ClosableIgniteClientProxy;
+import org.apache.ignite.springdata.proxy.ClosableIgniteProxy;
+import org.apache.ignite.springdata.proxy.IgniteClientProxy;
+import org.apache.ignite.springdata.proxy.IgniteProxy;
+import org.apache.ignite.springdata.proxy.IgniteProxyImpl;
+import org.apache.ignite.springdata.repository.config.RepositoryConfig;
+import org.springframework.beans.BeansException;
+import org.springframework.beans.factory.DisposableBean;
+import org.springframework.context.ApplicationContext;
+import org.springframework.context.ApplicationContextAware;
+
+/** Represents default implementation of {@link IgniteResourceProvider} */
+public class IgniteResourceProviderImpl implements IgniteResourceProvider, ApplicationContextAware, DisposableBean {
Review comment:
Is there any issues with managing Ignite as a bean in every case? In case with igniteCfg it is possible to create Ignite bean with method, isn't it?
I mean smth like that:
`public Ignite igniteInstance(@Autowired IgniteConfiguration cfg)`
`public IgniteProxy igniteProxy(@Autowired Ignite ignite)`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-extensions] timoninmaxim commented on a change in pull request #31: IGNITE-13722 Adds correct closing of Spring Data Ignite resources.
Posted by GitBox <gi...@apache.org>.
timoninmaxim commented on a change in pull request #31:
URL: https://github.com/apache/ignite-extensions/pull/31#discussion_r532588701
##########
File path: modules/spring-data-2.0-ext/src/main/java/org/apache/ignite/springdata20/repository/config/IgniteRepositoryConfigurationExtension.java
##########
@@ -18,15 +18,28 @@
import java.util.Collection;
import java.util.Collections;
+import org.apache.ignite.springdata.proxy.IgniteProxy;
import org.apache.ignite.springdata20.repository.IgniteRepository;
import org.apache.ignite.springdata20.repository.support.IgniteRepositoryFactoryBean;
+import org.springframework.beans.factory.support.BeanDefinitionBuilder;
+import org.springframework.beans.factory.support.BeanDefinitionRegistry;
+import org.springframework.data.repository.config.AnnotationRepositoryConfigurationSource;
import org.springframework.data.repository.config.RepositoryConfigurationExtension;
import org.springframework.data.repository.config.RepositoryConfigurationExtensionSupport;
+import org.springframework.data.repository.config.RepositoryConfigurationSource;
+
+import static org.springframework.beans.factory.config.ConfigurableBeanFactory.SCOPE_PROTOTYPE;
/**
* Apache Ignite specific implementation of {@link RepositoryConfigurationExtension}.
*/
public class IgniteRepositoryConfigurationExtension extends RepositoryConfigurationExtensionSupport {
+ /** Name of the auto-registered Ignite proxy factory bean. */
+ private static final String IGNITE_PROXY_FACTORY_BEAN_NAME = "igniteResourceProvider";
Review comment:
Is "igniteResourceProvider" is a valid name after refactor it to IgniteProxyFactory?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-extensions] alex-plekhanov commented on a change in pull request #31: IGNITE-13722 Adds correct closing of Spring Data Ignite resources.
Posted by GitBox <gi...@apache.org>.
alex-plekhanov commented on a change in pull request #31:
URL: https://github.com/apache/ignite-extensions/pull/31#discussion_r539397577
##########
File path: modules/spring-data-2.0-ext/src/main/java/org/apache/ignite/springdata20/repository/config/EnableIgniteRepositories.java
##########
@@ -116,4 +118,10 @@
* repositories infrastructure.
*/
boolean considerNestedRepositories() default false;
+
+ /**
+ * @return Implementation of the Ignite proxy factory to be used to create {@link IgniteProxy} instances
+ * that provide access to the Ignite cluster for each Spring Data repository.
+ */
+ Class<?> igniteProxyFactoryClass() default IgniteProxyFactory.class;
Review comment:
Do we really need to expose IgniteProxyFactory class to public API? Other classes (repositoryBaseClass, repositoryFactoryBeanClass) are exposed because it's needed to configure sping data, but igniteProxyFactoryClass required only for our own internal usage.
##########
File path: modules/spring-data-2.0-ext/src/main/java/org/apache/ignite/springdata20/repository/support/IgniteProxyFactory.java
##########
@@ -0,0 +1,172 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.springdata20.repository.support;
+
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import org.apache.ignite.Ignite;
+import org.apache.ignite.Ignition;
+import org.apache.ignite.client.IgniteClient;
+import org.apache.ignite.configuration.ClientConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.springdata.proxy.IgniteClientProxy;
+import org.apache.ignite.springdata.proxy.IgniteProxy;
+import org.apache.ignite.springdata.proxy.IgniteProxyImpl;
+import org.apache.ignite.springdata20.repository.config.RepositoryConfig;
+import org.springframework.beans.BeansException;
+import org.springframework.beans.factory.DisposableBean;
+import org.springframework.beans.factory.config.BeanExpressionContext;
+import org.springframework.beans.factory.config.BeanExpressionResolver;
+import org.springframework.beans.factory.support.DefaultListableBeanFactory;
+import org.springframework.context.ApplicationContext;
+import org.springframework.context.ApplicationContextAware;
+import org.springframework.context.expression.StandardBeanExpressionResolver;
+
+import static org.apache.ignite.springdata20.repository.support.IgniteRepositoryFactory.getRepositoryConfiguration;
+
+/**
+ * Represents factory for obtaining instances of {@link IgniteProxy} that provide client-independent connection to the
+ * Ignite cluster.
+ */
+public class IgniteProxyFactory implements ApplicationContextAware, DisposableBean {
+ /** Spring application expression resolver. */
+ private final BeanExpressionResolver expressionResolver = new StandardBeanExpressionResolver();
+
+ /** Repositories associated with Ignite proxy. */
+ private final Map<Class<?>, IgniteProxy> igniteProxies = new ConcurrentHashMap<>();
+
+ /** Spring application context. */
+ private ApplicationContext ctx;
+
+ /** Spring application bean expression context. */
+ private BeanExpressionContext beanExpressionCtx;
+
+ /**
+ * @param repoInterface The repository interface class for which {@link IgniteProxy} will be created.
+ * @return {@link IgniteProxy} instance.
+ */
+ public IgniteProxy igniteProxy(Class<?> repoInterface) {
+ return igniteProxies.computeIfAbsent(repoInterface, k -> createIgniteProxy(repoInterface));
+ }
+
+ /** {@inheritDoc} */
+ @Override public void setApplicationContext(ApplicationContext ctx) throws BeansException {
+ this.ctx = ctx;
+
+ beanExpressionCtx = new BeanExpressionContext(
+ new DefaultListableBeanFactory(ctx.getAutowireCapableBeanFactory()),
+ null);
+ }
+
+ /** {@inheritDoc} */
+ @Override public void destroy() throws Exception {
+ Set<IgniteProxy> proxies = new HashSet<>(igniteProxies.values());
+
+ Exception destroyE = null;
+
+ for (IgniteProxy proxy : proxies) {
+ try {
+ proxy.close();
+ }
+ catch (Exception e) {
+ if (destroyE == null)
+ destroyE = e;
+ else
+ destroyE.addSuppressed(e);
+ }
+ }
+
+ if (destroyE != null)
+ throw destroyE;
+ }
+
+ /**
+ * Creates {@link IgniteProxy} to be used for providing access to the Ignite cluster for specified repository.
+ *
+ * @param repoInterface {@link Class} instance of the repository interface.
+ * @return Instance of {@link IgniteProxy} associated with specified repository.
+ *
+ * @see RepositoryConfig
+ */
+ private IgniteProxy createIgniteProxy(Class<?> repoInterface) {
+ RepositoryConfig repoCfg = getRepositoryConfiguration(repoInterface);
+
+ try {
+ Object igniteInstanceBean = ctx.getBean(evaluateExpression(repoCfg.igniteInstance()));
+
+ if (igniteInstanceBean instanceof Ignite)
+ return new IgniteProxyImpl((Ignite)igniteInstanceBean);
+ else if (igniteInstanceBean instanceof IgniteClient)
+ return new IgniteClientProxy((IgniteClient)igniteInstanceBean);
+
+ throw new IllegalArgumentException("Invalid configuration for repository " + repoInterface.getName() +
+ ". The Spring Bean corresponding to the \"igniteInstance\" property of repository configuration must" +
+ " be one of the following types: [" + Ignite.class.getName() + ", " + IgniteClient.class.getName() +
+ ']');
+ }
+ catch (BeansException ex) {
+ try {
+ Object igniteCfgBean = ctx.getBean(evaluateExpression(repoCfg.igniteCfg()));
+
+ if (igniteCfgBean instanceof IgniteConfiguration) {
+ try {
+ return new IgniteProxyImpl(Ignition.ignite(
+ ((IgniteConfiguration)igniteCfgBean).getIgniteInstanceName()));
+ }
+ catch (Exception ignored) {
+ // No-op.
+ }
+
+ return new IgniteProxyImpl(Ignition.start((IgniteConfiguration)igniteCfgBean));
+ }
+ else if (igniteCfgBean instanceof ClientConfiguration)
+ return new IgniteClientProxy(Ignition.startClient((ClientConfiguration)igniteCfgBean));
+
+ throw new IllegalArgumentException("Invalid configuration for repository " + repoInterface.getName() +
+ ". The Spring Bean corresponding to the \"igniteCfg\" property of repository configuration must" +
+ " be one of the following types: [" + IgniteConfiguration.class.getName() + ", " +
+ ClientConfiguration.class.getName() + ']');
+ }
+ catch (BeansException ex2) {
+ try {
+ String igniteCfgPath = ctx.getBean(evaluateExpression(repoCfg.igniteSpringCfgPath()), String.class);
+
+ return new IgniteProxyImpl(Ignition.start(igniteCfgPath));
+ }
+ catch (BeansException ex3) {
+ throw new IllegalArgumentException("Invalid configuration for repository " +
+ repoInterface.getName() + ". No beans were found that provide connection configuration to the" +
+ " Ignite cluster. Check \"igniteInstance\", \"igniteCfg\", \"igniteSpringCfgPath\" parameters" +
+ " of " + RepositoryConfig.class.getName() + " repository annotation.");
+ }
Review comment:
Can we also move this boilerplate code to "commons" module? We can create some static method in IgniteProxy class and provide igniteInstanceSupplier, igniteConfigurationSupplier and igniteConfigurationPathSupplier as a parameters.
##########
File path: modules/spring-data-2.0-ext/src/main/java/org/apache/ignite/springdata20/repository/support/IgniteRepositoryFactoryBean.java
##########
@@ -62,6 +62,6 @@ protected IgniteRepositoryFactoryBean(Class<? extends T> repoInterface) {
/** {@inheritDoc} */
@Override protected RepositoryFactorySupport createRepositoryFactory() {
- return new IgniteRepositoryFactory(ctx);
+ return new IgniteRepositoryFactory(ctx, getObjectType());
Review comment:
There is a one-to-one relation between `IgniteRepositoryFactoryBean` and `IgniteRepositoryFactory`, also there is a one-to-one relation between `IgniteRepositoryFactory` and `IgniteProxy`. Perhaps we can simplify a little (but I'm not sure) if we mark `IgniteRepositoryFactoryBean` as disposable, and close factory and Ignite proxy on bean destroy. In this case, we don't need `IgniteProxyFactory` class, `igniteProxyFactory` and `igniteProxy` beans.
WDYT?
##########
File path: modules/spring-data-commons/src/main/java/org/apache/ignite/springdata/proxy/IgniteProxyImpl.java
##########
@@ -36,11 +38,34 @@ public IgniteProxyImpl(Ignite ignite) {
/** {@inheritDoc} */
@Override public <K, V> IgniteCacheProxy<K, V> cache(String name) {
- return new IgniteCacheProxyImpl<>(ignite.cache(name));
+ IgniteCache<K, V> cache = ignite.cache(name);
+
+ return cache == null ? null : new IgniteCacheProxyImpl<>(cache);
}
/** @return {@link Ignite} instance to which operations are delegated. */
public Ignite delegate() {
return ignite;
}
+
+ /** {@inheritDoc} */
+ @Override public boolean equals(Object other) {
+ if (this == other)
+ return true;
+
+ if (other == null || getClass() != other.getClass())
+ return false;
+
+ return Objects.equals(ignite, ((IgniteProxyImpl)other).ignite);
+ }
+
+ /** {@inheritDoc} */
+ @Override public int hashCode() {
+ return ignite.hashCode();
+ }
+
+ /** {@inheritDoc} */
+ @Override public void close() {
+ ignite.close();
Review comment:
I'm not sure we should close Ignite in all cases. For example, this looks strange if Ignite was started before Spring application context initialized and we close it after Spring application exits:
``` @Test
public void testIgniteInstanceOpenedBeforeRepository() throws Exception {
Ignite ignite = startGrid("ignite");
try (AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext()) {
ctx.register(ApplicationConfiguration.class);
ctx.refresh();
PersonRepository repo = ctx.getBean(PersonRepository.class);
repo.save(0, new Person("test", "test"));
assertEquals(new Person("test", "test"), repo.findOne(0));
}
assertTrue(ignite.cluster().state().active());
}
@Configuration
@EnableIgniteRepositories(includeFilters = @Filter(type = ASSIGNABLE_TYPE, classes = PersonRepository.class))
public static class ApplicationConfiguration {
@Bean
public IgniteConfiguration igniteCfg() {
return new IgniteConfiguration().setIgniteInstanceName("ignite");
}
}
```
Perhaps, we should close Ignite when we start it explicitly. If we get already initiated instance as a bean, this instance will be automatically closed by the Spring, on context destroy since Ignite is AutoClosable.
WDYT?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-extensions] ololo3000 commented on a change in pull request #31: IGNITE-13722 Adds correct closing of Spring Data Ignite resources.
Posted by GitBox <gi...@apache.org>.
ololo3000 commented on a change in pull request #31:
URL: https://github.com/apache/ignite-extensions/pull/31#discussion_r531222877
##########
File path: modules/spring-data-2.0-ext/src/main/java/org/apache/ignite/springdata20/repository/support/IgniteResourceProviderImpl.java
##########
@@ -0,0 +1,171 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.springdata20.repository.support;
+
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import org.apache.ignite.Ignite;
+import org.apache.ignite.Ignition;
+import org.apache.ignite.client.IgniteClient;
+import org.apache.ignite.configuration.ClientConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.springdata.proxy.ClosableIgniteClientProxy;
+import org.apache.ignite.springdata.proxy.ClosableIgniteProxy;
+import org.apache.ignite.springdata.proxy.IgniteClientProxy;
+import org.apache.ignite.springdata.proxy.IgniteProxy;
+import org.apache.ignite.springdata.proxy.IgniteProxyImpl;
+import org.apache.ignite.springdata20.repository.config.RepositoryConfig;
+import org.springframework.beans.BeansException;
+import org.springframework.beans.factory.DisposableBean;
+import org.springframework.beans.factory.config.BeanExpressionContext;
+import org.springframework.beans.factory.config.BeanExpressionResolver;
+import org.springframework.beans.factory.support.DefaultListableBeanFactory;
+import org.springframework.context.ApplicationContext;
+import org.springframework.context.ApplicationContextAware;
+import org.springframework.context.expression.StandardBeanExpressionResolver;
+import org.springframework.util.Assert;
+
+/** Represents default implementation of {@link IgniteResourceProvider} */
+public class IgniteResourceProviderImpl implements IgniteResourceProvider, ApplicationContextAware, DisposableBean {
+ /** Spring application expression resolver. */
+ private final BeanExpressionResolver expressionResolver = new StandardBeanExpressionResolver();
+
+ /** Ignite proxies associated with repositories. */
+ private final Map<Class<?>, IgniteProxy> igniteProxies = new ConcurrentHashMap<>();
+
+ /** Spring application context. */
+ private ApplicationContext ctx;
+
+ /** Spring application bean expression context. */
+ private BeanExpressionContext beanExpressionCtx;
+
+ /** {@inheritDoc} */
+ @Override public IgniteProxy igniteProxy(Class<?> repoInterface) {
+ return igniteProxies.computeIfAbsent(repoInterface, k -> createIgniteProxy(repoInterface));
+ }
+
+ /** {@inheritDoc} */
+ @Override public void setApplicationContext(ApplicationContext ctx) throws BeansException {
+ this.ctx = ctx;
+
+ beanExpressionCtx = new BeanExpressionContext(
+ new DefaultListableBeanFactory(ctx.getAutowireCapableBeanFactory()),
+ null);
+ }
+
+ /** {@inheritDoc} */
+ @Override public void destroy() throws Exception {
+ Set<IgniteProxy> proxies = new HashSet<>(igniteProxies.values());
+
+ for (IgniteProxy proxy : proxies) {
+ if (proxy instanceof AutoCloseable)
+ ((AutoCloseable)proxy).close();
+ }
+ }
+
+ /**
+ * Creates {@link IgniteProxy} to be used for providing access to the Ignite cluster for specified repository.
+ *
+ * @param repoInterface {@link Class} instance of the repository interface.
+ * @return Instance of {@link IgniteProxy} associated with specified repository.
+ *
+ * @see RepositoryConfig
+ */
+ private IgniteProxy createIgniteProxy(Class<?> repoInterface) {
+ RepositoryConfig repoCfg = getRepositoryConfiguration(repoInterface);
+
+ try {
+ Object igniteInstanceBean = ctx.getBean(evaluateExpression(repoCfg.igniteInstance()));
+
+ if (igniteInstanceBean instanceof Ignite)
+ return new IgniteProxyImpl((Ignite)igniteInstanceBean);
+ else if (igniteInstanceBean instanceof IgniteClient)
+ return new IgniteClientProxy((IgniteClient)igniteInstanceBean);
+
+ throw new IllegalArgumentException("Invalid repository configuration [name=" + repoInterface.getName() +
+ "]. The Spring Bean corresponding to the \"igniteInstance\" property of repository configuration must" +
+ " be one of the following types: [" + Ignite.class.getName() + ", " + IgniteClient.class.getName() +
+ ']');
+ }
+ catch (BeansException ex) {
Review comment:
In this case, I keep the logic as it is. Just moved it from IgniteRepository Factory to avoid unrelated changes. I think It can be done in separate ticket.
##########
File path: modules/spring-data-2.0-ext/src/main/java/org/apache/ignite/springdata20/repository/support/IgniteRepositoryFactory.java
##########
@@ -62,85 +52,45 @@
* @author Manuel Núñez (manuel.nunez@hawkore.com)
*/
public class IgniteRepositoryFactory extends RepositoryFactorySupport {
- /** Spring application context */
- private final ApplicationContext ctx;
-
- /** Spring application bean factory */
- private final DefaultListableBeanFactory beanFactory;
-
/** Spring application expression resolver */
private final StandardBeanExpressionResolver resolver = new StandardBeanExpressionResolver();
/** Spring application bean expression context */
private final BeanExpressionContext beanExpressionContext;
- /** Mapping of a repository to a cache. */
- private final Map<Class<?>, String> repoToCache = new HashMap<>();
+ /** Ignite cache proxy instance associated with the current repository. */
+ private final IgniteCacheProxy<?, ?> cache;
- /** Mapping of a repository to a ignite instance. */
- private final Map<Class<?>, IgniteProxy> repoToIgnite = new HashMap<>();
+ /** Ignite proxy instance associated with the current repository. */
+ private final IgniteProxy ignite;
Review comment:
Please double-check. Used to create cache and repository.
##########
File path: modules/spring-data-commons/src/main/java/org/apache/ignite/springdata/proxy/ClosableIgniteProxy.java
##########
@@ -0,0 +1,35 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.springdata.proxy;
+
+import org.apache.ignite.Ignite;
+
+/** Extends {@link IgniteProxyImpl} with the ability to close underlying Ignite instance. */
+public class ClosableIgniteProxy extends IgniteProxyImpl implements AutoCloseable {
Review comment:
Yes, there is. Spring Data repository can be configured in two ways - by providing name of the existing bean that contains Ignite instance explicitly or by specifying a configuration, so Spring Data can obtain Ignite instance itself. In first case, Ignite will be closed automatically by Spring. In the second, Ignite resource must be closed externally.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-extensions] timoninmaxim commented on a change in pull request #31: IGNITE-13722 Adds correct closing of Spring Data Ignite resources.
Posted by GitBox <gi...@apache.org>.
timoninmaxim commented on a change in pull request #31:
URL: https://github.com/apache/ignite-extensions/pull/31#discussion_r532588701
##########
File path: modules/spring-data-2.0-ext/src/main/java/org/apache/ignite/springdata20/repository/config/IgniteRepositoryConfigurationExtension.java
##########
@@ -18,15 +18,28 @@
import java.util.Collection;
import java.util.Collections;
+import org.apache.ignite.springdata.proxy.IgniteProxy;
import org.apache.ignite.springdata20.repository.IgniteRepository;
import org.apache.ignite.springdata20.repository.support.IgniteRepositoryFactoryBean;
+import org.springframework.beans.factory.support.BeanDefinitionBuilder;
+import org.springframework.beans.factory.support.BeanDefinitionRegistry;
+import org.springframework.data.repository.config.AnnotationRepositoryConfigurationSource;
import org.springframework.data.repository.config.RepositoryConfigurationExtension;
import org.springframework.data.repository.config.RepositoryConfigurationExtensionSupport;
+import org.springframework.data.repository.config.RepositoryConfigurationSource;
+
+import static org.springframework.beans.factory.config.ConfigurableBeanFactory.SCOPE_PROTOTYPE;
/**
* Apache Ignite specific implementation of {@link RepositoryConfigurationExtension}.
*/
public class IgniteRepositoryConfigurationExtension extends RepositoryConfigurationExtensionSupport {
+ /** Name of the auto-registered Ignite proxy factory bean. */
+ private static final String IGNITE_PROXY_FACTORY_BEAN_NAME = "igniteResourceProvider";
Review comment:
Is "igniteResourceProvider" i a valid name after refactor it to IgniteProxyFactory?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-extensions] ololo3000 commented on a change in pull request #31: IGNITE-13722 Adds correct closing of Spring Data Ignite resources.
Posted by GitBox <gi...@apache.org>.
ololo3000 commented on a change in pull request #31:
URL: https://github.com/apache/ignite-extensions/pull/31#discussion_r531222877
##########
File path: modules/spring-data-2.0-ext/src/main/java/org/apache/ignite/springdata20/repository/support/IgniteResourceProviderImpl.java
##########
@@ -0,0 +1,171 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.springdata20.repository.support;
+
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import org.apache.ignite.Ignite;
+import org.apache.ignite.Ignition;
+import org.apache.ignite.client.IgniteClient;
+import org.apache.ignite.configuration.ClientConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.springdata.proxy.ClosableIgniteClientProxy;
+import org.apache.ignite.springdata.proxy.ClosableIgniteProxy;
+import org.apache.ignite.springdata.proxy.IgniteClientProxy;
+import org.apache.ignite.springdata.proxy.IgniteProxy;
+import org.apache.ignite.springdata.proxy.IgniteProxyImpl;
+import org.apache.ignite.springdata20.repository.config.RepositoryConfig;
+import org.springframework.beans.BeansException;
+import org.springframework.beans.factory.DisposableBean;
+import org.springframework.beans.factory.config.BeanExpressionContext;
+import org.springframework.beans.factory.config.BeanExpressionResolver;
+import org.springframework.beans.factory.support.DefaultListableBeanFactory;
+import org.springframework.context.ApplicationContext;
+import org.springframework.context.ApplicationContextAware;
+import org.springframework.context.expression.StandardBeanExpressionResolver;
+import org.springframework.util.Assert;
+
+/** Represents default implementation of {@link IgniteResourceProvider} */
+public class IgniteResourceProviderImpl implements IgniteResourceProvider, ApplicationContextAware, DisposableBean {
+ /** Spring application expression resolver. */
+ private final BeanExpressionResolver expressionResolver = new StandardBeanExpressionResolver();
+
+ /** Ignite proxies associated with repositories. */
+ private final Map<Class<?>, IgniteProxy> igniteProxies = new ConcurrentHashMap<>();
+
+ /** Spring application context. */
+ private ApplicationContext ctx;
+
+ /** Spring application bean expression context. */
+ private BeanExpressionContext beanExpressionCtx;
+
+ /** {@inheritDoc} */
+ @Override public IgniteProxy igniteProxy(Class<?> repoInterface) {
+ return igniteProxies.computeIfAbsent(repoInterface, k -> createIgniteProxy(repoInterface));
+ }
+
+ /** {@inheritDoc} */
+ @Override public void setApplicationContext(ApplicationContext ctx) throws BeansException {
+ this.ctx = ctx;
+
+ beanExpressionCtx = new BeanExpressionContext(
+ new DefaultListableBeanFactory(ctx.getAutowireCapableBeanFactory()),
+ null);
+ }
+
+ /** {@inheritDoc} */
+ @Override public void destroy() throws Exception {
+ Set<IgniteProxy> proxies = new HashSet<>(igniteProxies.values());
+
+ for (IgniteProxy proxy : proxies) {
+ if (proxy instanceof AutoCloseable)
+ ((AutoCloseable)proxy).close();
+ }
+ }
+
+ /**
+ * Creates {@link IgniteProxy} to be used for providing access to the Ignite cluster for specified repository.
+ *
+ * @param repoInterface {@link Class} instance of the repository interface.
+ * @return Instance of {@link IgniteProxy} associated with specified repository.
+ *
+ * @see RepositoryConfig
+ */
+ private IgniteProxy createIgniteProxy(Class<?> repoInterface) {
+ RepositoryConfig repoCfg = getRepositoryConfiguration(repoInterface);
+
+ try {
+ Object igniteInstanceBean = ctx.getBean(evaluateExpression(repoCfg.igniteInstance()));
+
+ if (igniteInstanceBean instanceof Ignite)
+ return new IgniteProxyImpl((Ignite)igniteInstanceBean);
+ else if (igniteInstanceBean instanceof IgniteClient)
+ return new IgniteClientProxy((IgniteClient)igniteInstanceBean);
+
+ throw new IllegalArgumentException("Invalid repository configuration [name=" + repoInterface.getName() +
+ "]. The Spring Bean corresponding to the \"igniteInstance\" property of repository configuration must" +
+ " be one of the following types: [" + Ignite.class.getName() + ", " + IgniteClient.class.getName() +
+ ']');
+ }
+ catch (BeansException ex) {
Review comment:
In this case, I keep the logic as it is. Just moved it from IgniteRepositoryFactory to avoid unrelated changes. I think It can be done in separate ticket.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-extensions] ololo3000 commented on a change in pull request #31: IGNITE-13722 Adds correct closing of Spring Data Ignite resources.
Posted by GitBox <gi...@apache.org>.
ololo3000 commented on a change in pull request #31:
URL: https://github.com/apache/ignite-extensions/pull/31#discussion_r531222773
##########
File path: modules/spring-data-ext/src/main/java/org/apache/ignite/springdata/repository/support/IgniteResourceProviderImpl.java
##########
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.springdata.repository.support;
+
+import org.apache.ignite.Ignite;
+import org.apache.ignite.Ignition;
+import org.apache.ignite.client.IgniteClient;
+import org.apache.ignite.configuration.ClientConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.springdata.proxy.ClosableIgniteClientProxy;
+import org.apache.ignite.springdata.proxy.ClosableIgniteProxy;
+import org.apache.ignite.springdata.proxy.IgniteClientProxy;
+import org.apache.ignite.springdata.proxy.IgniteProxy;
+import org.apache.ignite.springdata.proxy.IgniteProxyImpl;
+import org.apache.ignite.springdata.repository.config.RepositoryConfig;
+import org.springframework.beans.BeansException;
+import org.springframework.beans.factory.DisposableBean;
+import org.springframework.context.ApplicationContext;
+import org.springframework.context.ApplicationContextAware;
+
+/** Represents default implementation of {@link IgniteResourceProvider} */
+public class IgniteResourceProviderImpl implements IgniteResourceProvider, ApplicationContextAware, DisposableBean {
Review comment:
The main problem with the previous implementation: Ignite / IgniteClient instances started by Spring Data(in case Ignite / IgniteClient configuration is used to configure the repository. See RepositoryConfing#igniteCfg or igniteSpringCfgPath) are not closed at all. Also Ignite instances are started during bean initialization. Therefore, in some cases, bean initialization can fail with error but started Ignite instances will continue to work. By moving Ignite management logic to the separate bean - IgniteResourceProvider - we can control Ignite instances regardless of the repository lifecycle and close it properly when Spring context is closed. It also allows users to use custom logic for obtaining Ignite Instance associated with a repository (see EnableIgniteRepositories#igniteResourceProvider).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-extensions] timoninmaxim commented on a change in pull request #31: IGNITE-13722 Adds correct closing of Spring Data Ignite resources.
Posted by GitBox <gi...@apache.org>.
timoninmaxim commented on a change in pull request #31:
URL: https://github.com/apache/ignite-extensions/pull/31#discussion_r531497344
##########
File path: modules/spring-data-ext/src/main/java/org/apache/ignite/springdata/repository/support/IgniteResourceProviderImpl.java
##########
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.springdata.repository.support;
+
+import org.apache.ignite.Ignite;
+import org.apache.ignite.Ignition;
+import org.apache.ignite.client.IgniteClient;
+import org.apache.ignite.configuration.ClientConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.springdata.proxy.ClosableIgniteClientProxy;
+import org.apache.ignite.springdata.proxy.ClosableIgniteProxy;
+import org.apache.ignite.springdata.proxy.IgniteClientProxy;
+import org.apache.ignite.springdata.proxy.IgniteProxy;
+import org.apache.ignite.springdata.proxy.IgniteProxyImpl;
+import org.apache.ignite.springdata.repository.config.RepositoryConfig;
+import org.springframework.beans.BeansException;
+import org.springframework.beans.factory.DisposableBean;
+import org.springframework.context.ApplicationContext;
+import org.springframework.context.ApplicationContextAware;
+
+/** Represents default implementation of {@link IgniteResourceProvider} */
+public class IgniteResourceProviderImpl implements IgniteResourceProvider, ApplicationContextAware, DisposableBean {
Review comment:
Starting Ignite with `new` inside Repository looks bad. It is not a Spring way
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-extensions] timoninmaxim commented on a change in pull request #31: IGNITE-13722 Adds correct closing of Spring Data Ignite resources.
Posted by GitBox <gi...@apache.org>.
timoninmaxim commented on a change in pull request #31:
URL: https://github.com/apache/ignite-extensions/pull/31#discussion_r531495407
##########
File path: modules/spring-data-ext/src/main/java/org/apache/ignite/springdata/repository/support/IgniteResourceProviderImpl.java
##########
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.springdata.repository.support;
+
+import org.apache.ignite.Ignite;
+import org.apache.ignite.Ignition;
+import org.apache.ignite.client.IgniteClient;
+import org.apache.ignite.configuration.ClientConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.springdata.proxy.ClosableIgniteClientProxy;
+import org.apache.ignite.springdata.proxy.ClosableIgniteProxy;
+import org.apache.ignite.springdata.proxy.IgniteClientProxy;
+import org.apache.ignite.springdata.proxy.IgniteProxy;
+import org.apache.ignite.springdata.proxy.IgniteProxyImpl;
+import org.apache.ignite.springdata.repository.config.RepositoryConfig;
+import org.springframework.beans.BeansException;
+import org.springframework.beans.factory.DisposableBean;
+import org.springframework.context.ApplicationContext;
+import org.springframework.context.ApplicationContextAware;
+
+/** Represents default implementation of {@link IgniteResourceProvider} */
+public class IgniteResourceProviderImpl implements IgniteResourceProvider, ApplicationContextAware, DisposableBean {
Review comment:
Is there any issues with managing Ignite as a bean in every case? In case with igniteCfg it is possible to create Ignite bean with method, isn't it?
I mean smth like that:
`public Ignite ignite(@Autowired IgniteConfiguration cfg)`
`public IgniteProxy igniteProxy(@Autowired Ignite ignite)`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-extensions] timoninmaxim commented on a change in pull request #31: IGNITE-13722 Adds correct closing of Spring Data Ignite resources.
Posted by GitBox <gi...@apache.org>.
timoninmaxim commented on a change in pull request #31:
URL: https://github.com/apache/ignite-extensions/pull/31#discussion_r532585807
##########
File path: modules/spring-data-2.0-ext/src/main/java/org/apache/ignite/springdata20/repository/support/IgniteRepositoryFactoryBean.java
##########
@@ -22,6 +22,7 @@
import org.apache.ignite.configuration.IgniteConfiguration;
import org.apache.ignite.springdata20.repository.IgniteRepository;
import org.springframework.beans.BeansException;
+import org.springframework.beans.factory.annotation.Autowired;
Review comment:
Unused import. Intellij IDEA supports feature to find unused imports, please check it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-extensions] ololo3000 commented on a change in pull request #31: IGNITE-13722 Adds correct closing of Spring Data Ignite resources.
Posted by GitBox <gi...@apache.org>.
ololo3000 commented on a change in pull request #31:
URL: https://github.com/apache/ignite-extensions/pull/31#discussion_r531222773
##########
File path: modules/spring-data-ext/src/main/java/org/apache/ignite/springdata/repository/support/IgniteResourceProviderImpl.java
##########
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.springdata.repository.support;
+
+import org.apache.ignite.Ignite;
+import org.apache.ignite.Ignition;
+import org.apache.ignite.client.IgniteClient;
+import org.apache.ignite.configuration.ClientConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.springdata.proxy.ClosableIgniteClientProxy;
+import org.apache.ignite.springdata.proxy.ClosableIgniteProxy;
+import org.apache.ignite.springdata.proxy.IgniteClientProxy;
+import org.apache.ignite.springdata.proxy.IgniteProxy;
+import org.apache.ignite.springdata.proxy.IgniteProxyImpl;
+import org.apache.ignite.springdata.repository.config.RepositoryConfig;
+import org.springframework.beans.BeansException;
+import org.springframework.beans.factory.DisposableBean;
+import org.springframework.context.ApplicationContext;
+import org.springframework.context.ApplicationContextAware;
+
+/** Represents default implementation of {@link IgniteResourceProvider} */
+public class IgniteResourceProviderImpl implements IgniteResourceProvider, ApplicationContextAware, DisposableBean {
Review comment:
The main problem with the previous implementation: Ignite / IgniteClient instances started by Spring Data(in case Ignite / IgniteClient configuration is used to configure the repository. See RepositoryConfing#igniteCfg or igniteSpringCfgPath) are not closed at all. Also Ignite instances are started during bean initialization. Therefore, in some cases, bean initialization can fail with error but started Ignite instances will continue to work. By moving Ignite management logic to the separate bean - IgniteResourceProvider - we can control Ignite instances regardless of the repository lifecycle. It also allows users to use custom logic for obtaining Ignite Instance associated with a repository (see EnableIgniteRepositories#igniteResourceProvider).
##########
File path: modules/spring-data-ext/src/main/java/org/apache/ignite/springdata/repository/support/IgniteResourceProviderImpl.java
##########
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.springdata.repository.support;
+
+import org.apache.ignite.Ignite;
+import org.apache.ignite.Ignition;
+import org.apache.ignite.client.IgniteClient;
+import org.apache.ignite.configuration.ClientConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.springdata.proxy.ClosableIgniteClientProxy;
+import org.apache.ignite.springdata.proxy.ClosableIgniteProxy;
+import org.apache.ignite.springdata.proxy.IgniteClientProxy;
+import org.apache.ignite.springdata.proxy.IgniteProxy;
+import org.apache.ignite.springdata.proxy.IgniteProxyImpl;
+import org.apache.ignite.springdata.repository.config.RepositoryConfig;
+import org.springframework.beans.BeansException;
+import org.springframework.beans.factory.DisposableBean;
+import org.springframework.context.ApplicationContext;
+import org.springframework.context.ApplicationContextAware;
+
+/** Represents default implementation of {@link IgniteResourceProvider} */
+public class IgniteResourceProviderImpl implements IgniteResourceProvider, ApplicationContextAware, DisposableBean {
+ /** Spring application context. */
+ @SuppressWarnings("FieldAccessedSynchronizedAndUnsynchronized")
+ private ApplicationContext ctx;
+
+ /** Ignite proxy. */
+ private volatile IgniteProxy igniteProxy;
+
+ /** {@inheritDoc} */
+ @Override public IgniteProxy igniteProxy() {
+ IgniteProxy res = igniteProxy;
+
+ if (res == null) {
+ synchronized (this) {
+ res = igniteProxy;
+
+ if (res == null) {
+ res = createIgniteProxy();
+
+ igniteProxy = res;
+ }
+ }
+ }
+
+ return igniteProxy;
+ }
+
+ /** {@inheritDoc} */
+ @Override public void setApplicationContext(ApplicationContext ctx) throws BeansException {
+ this.ctx = ctx;
+ }
+
+ /** {@inheritDoc} */
+ @Override public void destroy() throws Exception {
+ IgniteProxy proxy = igniteProxy;
+
+ if (proxy instanceof AutoCloseable)
+ ((AutoCloseable)proxy).close();
+ }
+
+ /**
+ * Creates {@link IgniteProxy} to be used for providing access to the Ignite cluster for all repositories.
+ *
+ * @return Instance of {@link IgniteProxy}.
+ *
+ * @see RepositoryConfig
+ */
+ private IgniteProxy createIgniteProxy() {
+ try {
+ Object igniteInstanceBean = ctx.getBean("igniteInstance");
Review comment:
I keep the existing logic unchanged just moved it into a separate bean. Currently, configuration of multiple Ignite instances is not supported for this version of Spring Data integration.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-extensions] timoninmaxim commented on a change in pull request #31: IGNITE-13722 Adds correct closing of Spring Data Ignite resources.
Posted by GitBox <gi...@apache.org>.
timoninmaxim commented on a change in pull request #31:
URL: https://github.com/apache/ignite-extensions/pull/31#discussion_r531495407
##########
File path: modules/spring-data-ext/src/main/java/org/apache/ignite/springdata/repository/support/IgniteResourceProviderImpl.java
##########
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.springdata.repository.support;
+
+import org.apache.ignite.Ignite;
+import org.apache.ignite.Ignition;
+import org.apache.ignite.client.IgniteClient;
+import org.apache.ignite.configuration.ClientConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.springdata.proxy.ClosableIgniteClientProxy;
+import org.apache.ignite.springdata.proxy.ClosableIgniteProxy;
+import org.apache.ignite.springdata.proxy.IgniteClientProxy;
+import org.apache.ignite.springdata.proxy.IgniteProxy;
+import org.apache.ignite.springdata.proxy.IgniteProxyImpl;
+import org.apache.ignite.springdata.repository.config.RepositoryConfig;
+import org.springframework.beans.BeansException;
+import org.springframework.beans.factory.DisposableBean;
+import org.springframework.context.ApplicationContext;
+import org.springframework.context.ApplicationContextAware;
+
+/** Represents default implementation of {@link IgniteResourceProvider} */
+public class IgniteResourceProviderImpl implements IgniteResourceProvider, ApplicationContextAware, DisposableBean {
Review comment:
Is there any issues with managing Ignite as a bean in every case? In case with igniteCfg it is possible to create Ignite bean with method, isn't it?
I mean smth like that:
`@Bean public Ignite ignite(@Autowired IgniteConfiguration cfg)`
`@Bean public IgniteProxy igniteProxy(@Autowired Ignite ignite)`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-extensions] ololo3000 commented on a change in pull request #31: IGNITE-13722 Adds correct closing of Spring Data Ignite resources.
Posted by GitBox <gi...@apache.org>.
ololo3000 commented on a change in pull request #31:
URL: https://github.com/apache/ignite-extensions/pull/31#discussion_r540323928
##########
File path: modules/spring-data-2.0-ext/src/main/java/org/apache/ignite/springdata20/repository/config/EnableIgniteRepositories.java
##########
@@ -116,4 +118,10 @@
* repositories infrastructure.
*/
boolean considerNestedRepositories() default false;
+
+ /**
+ * @return Implementation of the Ignite proxy factory to be used to create {@link IgniteProxy} instances
+ * that provide access to the Ignite cluster for each Spring Data repository.
+ */
+ Class<?> igniteProxyFactoryClass() default IgniteProxyFactory.class;
Review comment:
Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-extensions] ololo3000 commented on a change in pull request #31: IGNITE-13722 Adds correct closing of Spring Data Ignite resources.
Posted by GitBox <gi...@apache.org>.
ololo3000 commented on a change in pull request #31:
URL: https://github.com/apache/ignite-extensions/pull/31#discussion_r531237786
##########
File path: modules/spring-data-ext/src/test/java/org/apache/ignite/springdata/IgniteSpringDataConfigurationTest.java
##########
@@ -0,0 +1,215 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.springdata;
+
+import org.apache.ignite.Ignite;
+import org.apache.ignite.Ignition;
+import org.apache.ignite.configuration.CacheConfiguration;
+import org.apache.ignite.configuration.ClientConfiguration;
+import org.apache.ignite.configuration.ClientConnectorConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.spi.discovery.tcp.TcpDiscoverySpi;
+import org.apache.ignite.spi.discovery.tcp.ipfinder.TcpDiscoveryIpFinder;
+import org.apache.ignite.spi.discovery.tcp.ipfinder.vm.TcpDiscoveryVmIpFinder;
+import org.apache.ignite.springdata.misc.InvalidCacheRepository;
+import org.apache.ignite.springdata.misc.Person;
+import org.apache.ignite.springdata.misc.PersonRepository;
+import org.apache.ignite.springdata.repository.config.EnableIgniteRepositories;
+import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest;
+import org.junit.Test;
+import org.springframework.context.annotation.AnnotationConfigApplicationContext;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.ComponentScan.Filter;
+import org.springframework.context.annotation.Configuration;
+
+import static org.apache.ignite.testframework.GridTestUtils.assertThrowsAnyCause;
+import static org.springframework.context.annotation.FilterType.ASSIGNABLE_TYPE;
+
+/** Tests Sprign Data cluster access configurations. */
Review comment:
Done.
##########
File path: modules/spring-data-ext/src/test/resources/repository-ignite-config.xml
##########
@@ -0,0 +1,51 @@
+<?xml version="1.0" encoding="UTF-8"?>
+
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+
+<beans xmlns="http://www.springframework.org/schema/beans"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://www.springframework.org/schema/beans
+ http://www.springframework.org/schema/beans/spring-beans.xsd">
+ <bean id="ignite.cfg" class="org.apache.ignite.configuration.IgniteConfiguration">
+ <property name="igniteInstanceName" value="cli-node"/>
+
+ <property name="clientMode" value="true"/>
+
+ <property name="cacheConfiguration">
+ <list>
+ <bean class="org.apache.ignite.configuration.CacheConfiguration">
+ <property name="name" value="PersonCache"/>
+ </bean>
+ </list>
+ </property>
+
+ <property name="discoverySpi">
+ <bean class="org.apache.ignite.spi.discovery.tcp.TcpDiscoverySpi">
+ <property name="ipFinder">
+ <bean class="org.apache.ignite.spi.discovery.tcp.ipfinder.vm.TcpDiscoveryVmIpFinder">
+ <property name="addresses">
+ <list>
+ <value>127.0.0.1:47500..47509</value>
+ </list>
+ </property>
+ </bean>
+ </property>
+ </bean>
+ </property>
+ </bean>
+</beans>
Review comment:
Done.
##########
File path: modules/spring-data-ext/src/test/java/org/apache/ignite/springdata/IgniteSpringDataCrudSelfTest.java
##########
@@ -93,6 +94,8 @@
/** {@inheritDoc} */
@Override protected void afterTestsStopped() {
ctx.destroy();
+
+ assertTrue(Ignition.allGrids().isEmpty());
Review comment:
Done.
##########
File path: modules/spring-data-ext/src/test/java/org/apache/ignite/springdata/misc/IgniteClientApplicationConfiguration.java
##########
@@ -27,13 +27,17 @@
import org.apache.ignite.internal.IgniteEx;
import org.apache.ignite.springdata.repository.config.EnableIgniteRepositories;
import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.ComponentScan;
import org.springframework.context.annotation.Configuration;
import static org.apache.ignite.springdata.compoundkey.CompoundKeyApplicationConfiguration.CLI_CONN_PORT;
+import static org.springframework.context.annotation.FilterType.ASSIGNABLE_TYPE;
/** Spring Application configuration for repository testing in case thin client is used for accessing the cluster. */
@Configuration
-@EnableIgniteRepositories({"org.apache.ignite.springdata.compoundkey", "org.apache.ignite.springdata.misc"})
+@EnableIgniteRepositories(
+ value = {"org.apache.ignite.springdata.compoundkey", "org.apache.ignite.springdata.misc"},
+ excludeFilters = @ComponentScan.Filter(type = ASSIGNABLE_TYPE, classes = InvalidCacheRepository.class))
Review comment:
Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-extensions] asfgit closed pull request #31: IGNITE-13722 Adds correct closing of Spring Data Ignite resources.
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #31:
URL: https://github.com/apache/ignite-extensions/pull/31
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-extensions] ololo3000 commented on a change in pull request #31: IGNITE-13722 Adds correct closing of Spring Data Ignite resources.
Posted by GitBox <gi...@apache.org>.
ololo3000 commented on a change in pull request #31:
URL: https://github.com/apache/ignite-extensions/pull/31#discussion_r531222895
##########
File path: modules/spring-data-2.0-ext/src/main/java/org/apache/ignite/springdata20/repository/support/IgniteRepositoryFactory.java
##########
@@ -62,85 +52,45 @@
* @author Manuel Núñez (manuel.nunez@hawkore.com)
*/
public class IgniteRepositoryFactory extends RepositoryFactorySupport {
- /** Spring application context */
- private final ApplicationContext ctx;
-
- /** Spring application bean factory */
- private final DefaultListableBeanFactory beanFactory;
-
/** Spring application expression resolver */
private final StandardBeanExpressionResolver resolver = new StandardBeanExpressionResolver();
/** Spring application bean expression context */
private final BeanExpressionContext beanExpressionContext;
- /** Mapping of a repository to a cache. */
- private final Map<Class<?>, String> repoToCache = new HashMap<>();
+ /** Ignite cache proxy instance associated with the current repository. */
+ private final IgniteCacheProxy<?, ?> cache;
- /** Mapping of a repository to a ignite instance. */
- private final Map<Class<?>, IgniteProxy> repoToIgnite = new HashMap<>();
+ /** Ignite proxy instance associated with the current repository. */
+ private final IgniteProxy ignite;
Review comment:
Please double-check. Used to create cache and repository.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-extensions] ololo3000 commented on pull request #31: IGNITE-13722 Adds correct closing of Spring Data Ignite resources.
Posted by GitBox <gi...@apache.org>.
ololo3000 commented on pull request #31:
URL: https://github.com/apache/ignite-extensions/pull/31#issuecomment-742649333
@alex-plekhanov The review comments were fixed. Could you, please, take a look.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-extensions] timoninmaxim commented on a change in pull request #31: IGNITE-13722 Adds correct closing of Spring Data Ignite resources.
Posted by GitBox <gi...@apache.org>.
timoninmaxim commented on a change in pull request #31:
URL: https://github.com/apache/ignite-extensions/pull/31#discussion_r532596102
##########
File path: modules/spring-data-commons/src/main/java/org/apache/ignite/springdata/proxy/ClosableIgniteClientProxy.java
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.springdata.proxy;
+
+import org.apache.ignite.client.IgniteClient;
+
+/** Extends {@link IgniteClientProxy} with the ability to close underlying thin client instance. */
+public class ClosableIgniteClientProxy extends IgniteClientProxy implements AutoCloseable {
Review comment:
Also, I have check and it looks like Ignite.stop() is safe to run multiple times. It checks status and just write warn to a log if close invoked multiple times. So is there any other issues to introduce this class?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org