[GitHub] [ignite-extensions] ololo3000 commented on a change in pull request #31: IGNITE-13722 Adds correct closing of Spring Data Ignite resources.

[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