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/12/10 06:37:12 UTC
[GitHub] [ignite-extensions] alex-plekhanov commented on a change in pull request #31: IGNITE-13722 Adds correct closing of Spring Data Ignite resources.
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