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 2021/10/17 00:46:55 UTC
[GitHub] [ignite-3] sanpwc commented on a change in pull request #395: IGNITE-15601 Implement stop for calcite module in 3.0
sanpwc commented on a change in pull request #395:
URL: https://github.com/apache/ignite-3/pull/395#discussion_r729887830
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/SqlQueryProcessor.java
##########
@@ -52,6 +55,12 @@
private final TableManager tableManager;
+ /** Busy lock for stop synchronisation. */
+ private final IgniteSpinBusyLock busyLock = new IgniteSpinBusyLock();
+
+ /** Event listeners to close. */
+ private final List<Pair<TableEvent, EventListener>> evtLsnrs = new ArrayList<>();
Review comment:
Seems that there's only one listener per each TableEvent type in SqlQueryProcessor. What about using Map instead of List<Pair<>>?
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/SqlQueryProcessor.java
##########
@@ -91,20 +90,42 @@ public SqlQueryProcessor(
ArrayRowHandler.INSTANCE
);
- tableManager.listen(TableEvent.CREATE, new TableCreatedListener(schemaHolder));
- tableManager.listen(TableEvent.ALTER, new TableUpdatedListener(schemaHolder));
- tableManager.listen(TableEvent.DROP, new TableDroppedListener(schemaHolder));
+ registerTableListener(TableEvent.CREATE, new TableCreatedListener(schemaHolder));
+ registerTableListener(TableEvent.ALTER, new TableUpdatedListener(schemaHolder));
+ registerTableListener(TableEvent.DROP, new TableDroppedListener(schemaHolder));
}
+ /** */
+ private void registerTableListener(TableEvent evt, AbstractTableEventListener lsnr) {
+ evtLsnrs.add(Pair.of(evt, lsnr));
+
+ tableManager.listen(evt, lsnr);
+ }
/** {@inheritDoc} */
- @Override public void stop() throws NodeStoppingException {
- // TODO: IGNITE-15161 Implement component's stop.
+ @SuppressWarnings("unchecked")
+ @Override public void stop() throws Exception {
+ busyLock.block();
+
+ IgniteUtils.closeAll(
+ () -> evtLsnrs.forEach(l -> tableManager.removeListener(l.left, l.right)),
+ executionSrvc,
+ msgSrvc,
+ taskExecutor
+ );
}
/** {@inheritDoc} */
@Override public List<SqlCursor<List<?>>> query(String schemaName, String qry, Object... params) {
Review comment:
query javadoc should declare that it throws IgniteException.
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ClosableIteratorsHolder.java
##########
@@ -66,27 +66,17 @@ public ClosableIteratorsHolder(IgniteLogger log) {
}
/** */
- public void init() {
- cleanWorker = new Thread(null, () -> cleanUp(true), "calciteIteratorsCleanWorker");
+ public void init(String nodeName) {
+ cleanWorker = new Thread(null, () -> cleanUp(true), "%" + nodeName + "%calciteIteratorsCleanWorker");
Review comment:
Did you consider moving IgniteThread from 2? At least it's naming part.
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ClosableIteratorsHolder.java
##########
@@ -66,27 +66,17 @@ public ClosableIteratorsHolder(IgniteLogger log) {
}
/** */
- public void init() {
- cleanWorker = new Thread(null, () -> cleanUp(true), "calciteIteratorsCleanWorker");
+ public void init(String nodeName) {
+ cleanWorker = new Thread(null, () -> cleanUp(true), "%" + nodeName + "%calciteIteratorsCleanWorker");
cleanWorker.setDaemon(true);
cleanWorker.start();
}
- /** */
- public void tearDown() {
- stopped = true;
- refMap.clear();
-
- Thread t = cleanWorker;
-
- if (t != null)
- t.interrupt();
- }
-
/** */
private void cleanUp(boolean blocking) {
- for (Reference<?> ref = nextRef(blocking); !stopped && ref != null; ref = nextRef(blocking))
+ for (Reference<?> ref = nextRef(blocking); !stopped && ref != null; ref = nextRef(blocking)) {
Review comment:
Seems that we don't need {} for one-liner.
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ClosableIteratorsHolder.java
##########
@@ -66,27 +66,17 @@ public ClosableIteratorsHolder(IgniteLogger log) {
}
/** */
- public void init() {
- cleanWorker = new Thread(null, () -> cleanUp(true), "calciteIteratorsCleanWorker");
+ public void init(String nodeName) {
+ cleanWorker = new Thread(null, () -> cleanUp(true), "%" + nodeName + "%calciteIteratorsCleanWorker");
Review comment:
cleanWorker should either be volatile or final (if it's created within constructor and not init).
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ExchangeService.java
##########
@@ -25,7 +25,7 @@
/**
*
*/
-public interface ExchangeService {
+public interface ExchangeService extends AutoCloseable {
Review comment:
Not related to given patch, however seems that ExchangeServiceImpl.close is no-op. Is that correct? I believe that taskExecutor and similar should be closed.
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ExchangeServiceImpl.java
##########
@@ -259,4 +255,9 @@ else if (LOG.isDebugEnabled()) {
null,
ImmutableMap.of());
}
+
+ /** {@inheritDoc} */
+ @Override public void close() throws Exception {
+ // No-op.
Review comment:
Is that correct? I believe that taskExecutor and similar should be closed.
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/SqlQueryProcessor.java
##########
@@ -62,17 +71,7 @@ public SqlQueryProcessor(
/** {@inheritDoc} */
@Override public void start() {
- String nodeName = clusterSrvc.localConfiguration().getName();
-
- taskExecutor = new QueryTaskExecutorImpl(
- new StripedThreadPoolExecutor(
- 4,
- NamedThreadFactory.threadPrefix(nodeName, "calciteQry"),
- null,
- true,
- DFLT_THREAD_KEEP_ALIVE_TIME
Review comment:
Let's also move constant to QueryTaskExecutorImpl
##########
File path: modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/AbstractExecutionTest.java
##########
@@ -21,6 +21,7 @@
import java.util.UUID;
import java.util.function.Function;
import java.util.stream.Collectors;
+
Review comment:
Do we really need this blank line?
##########
File path: modules/runner/src/integrationTest/java/org/apache/ignite/internal/calcite/ITStopCalciteModuleTest.java
##########
@@ -0,0 +1,88 @@
+/*
+ * 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.internal.calcite;
+
+import java.lang.management.ManagementFactory;
+import java.lang.management.ThreadInfo;
+import java.lang.management.ThreadMXBean;
+import java.util.Arrays;
+import java.util.List;
+import com.google.common.collect.Lists;
+import org.apache.ignite.Ignite;
+import org.apache.ignite.internal.app.IgniteImpl;
+import org.apache.ignite.internal.processors.query.calcite.QueryProcessor;
+import org.apache.ignite.internal.processors.query.calcite.SqlCursor;
+import org.apache.ignite.internal.util.IgniteUtils;
+import org.apache.ignite.lang.IgniteException;
+import org.apache.ignite.lang.NodeStoppingException;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Stop Calcite module integration test.
+ */
+public class ITStopCalciteModuleTest extends AbstractBasicIntegrationTest {
Review comment:
Do we really need integration test for component stop? What about unit tests with explicit component creation with dependencies been mocked?
I'd also check that cursor.close on stopped node works as expected and tests for concurrent component's stop and cursor operations if you think that it's worth 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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org