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/26 19:29:38 UTC
[GitHub] [ignite-extensions] timoninmaxim commented on a change in pull request #31: IGNITE-13722 Adds correct closing of Spring Data Ignite resources.
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