sk0x50 commented on a change in pull request #230:
URL: https://github.com/apache/ignite-3/pull/230#discussion_r676505072
##########
File path:
modules/affinity/src/test/java/org/apache/ignite/internal/affinity/AffinityManagerTest.java
##########
@@ -189,6 +191,8 @@ public void testCalculatedAssignment() {
AffinityManager affinityManager = new AffinityManager(cfrMgr, mm, bm);
+ affinityManager.start();
Review comment:
Should it be stopped after the test?
##########
File path: modules/api/src/main/java/org/apache/ignite/app/Ignite.java
##########
@@ -24,6 +24,13 @@
* Ignite node interface. Main entry-point for all Ignite APIs.
*/
public interface Ignite extends AutoCloseable {
+ /**
+ * Returns ignite instance name.
Review comment:
It would be nice to have the same Javadoc description which already
exists for `Ignition.start()`
```
/**
* Starts an Ignite node with an optional bootstrap configuration from
an input stream with HOCON configs.
*
* @param name Name of the node. Must not be {@code null}.
* @param config Input stream from the node configuration in HOCON
format. Can be {@code null}.
* @param workDir Work directory for the started node. Must not be
{@code null}.
* @return Started Ignite node.
*/
public Ignite start(@NotNull String name, @Nullable InputStream config,
@NotNull Path workDir);
```
##########
File path:
modules/core/src/main/java/org/apache/ignite/lang/NodeStoppingException.java
##########
@@ -0,0 +1,75 @@
+/*
+ * 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.lang;
+
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Node stopping internal checked exception.
Review comment:
This sentence is not so good, I think.
`This exception is used in order to indicate that Ignite node is stopping
(already stopped) for some reason.`
##########
File path:
modules/runner/src/main/java/org/apache/ignite/internal/app/IgnitionImpl.java
##########
@@ -129,6 +147,24 @@
return doStart(name, null, workDir);
}
+ /** {@inheritDoc} */
+ @Override public void stop(@NotNull String name) {
Review comment:
It looks like we need to clearly specify the behavior in the case when a
node with the specified name is not found or empty.
Do we need to provide additional clarity for the end-user in this case
(return `false` value/ throw a new exception)?
##########
File path: modules/api/src/main/java/org/apache/ignite/app/Ignition.java
##########
@@ -55,4 +55,11 @@
* @return Started Ignite node.
*/
public Ignite start(@NotNull String name, @NotNull Path workDir);
+
+ /**
+ * Stops node with given node. It's possible to stop both already started
node or node that is currently starting.
Review comment:
Here and below: `Stops a node with the given {@code name}.`
##########
File path:
modules/runner/src/main/java/org/apache/ignite/internal/app/IgnitionImpl.java
##########
@@ -256,11 +390,79 @@ private static VaultManager createVault(String nodeName,
Path workDir) {
var vaultMgr = new VaultManager(new PersistentVaultService(vaultPath));
- vaultMgr.putName(nodeName).join();
-
return vaultMgr;
}
+ /**
+ * Checks node status. If it's STOPPING then prevents further starting and
throws NodeStoppingException
+ * that will lead to stopping already started components later on,
+ * otherwise starts component and add it to started components list.
+ *
+ * @param nodeName Node name.
+ * @param startedComponents List of already started components for given
node.
+ * @param component Ignite component to start.
+ * @param <T> Ignite component type.
+ * @return Started ignite component.
+ * @throws NodeStoppingException If node stopping intention was detected.
+ */
+ private static <T extends IgniteComponent> T doStartComponent(
+ @NotNull String nodeName,
+ @NotNull List<IgniteComponent> startedComponents,
+ @NotNull T component
+ ) throws NodeStoppingException {
+ if (nodesStatus.get(nodeName) == NodeState.STOPPING)
+ throw new NodeStoppingException("Node=[" + nodeName + "] was
stopped.");
+ else {
+ startedComponents.add(component);
+
+ component.start();
+
+ return component;
+ }
+ }
+
+ /**
+ * Calls {@link IgniteComponent#beforeNodeStop()} and then {@link
IgniteComponent#stop()} for all components
+ * in start-reverse-order. Cleanups node started components map and node
status map.
+ *
+ * @param nodeName Node name.
+ * @param startedComponents List of already started components for given
node.
+ */
+ private static void doStopNode(@NotNull String nodeName, @NotNull
List<IgniteComponent> startedComponents) {
+ ListIterator<IgniteComponent> componentsonNodeStopIter =
Review comment:
`componentson` :) Perhaps, `beforeStopIter` and `stopIter` will be
enough? WDYT?
##########
File path:
modules/core/src/main/java/org/apache/ignite/lang/NodeStoppingException.java
##########
@@ -0,0 +1,75 @@
+/*
+ * 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.lang;
+
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Node stopping internal checked exception.
+ */
+public class NodeStoppingException extends IgniteInternalCheckedException {
+ /** Serial version UID. */
+ private static final long serialVersionUID = 0L;
+
+ /**
+ * Creates an empty node stopping exception.
+ */
+ public NodeStoppingException() {
+ super("Node is stopping");
Review comment:
Please add a dot at the end of the sentence.
##########
File path:
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/IgnitionTest.java
##########
@@ -79,8 +78,8 @@
/** */
@AfterEach
- void tearDown() throws Exception {
- IgniteUtils.closeAll(startedNodes);
+ void tearDown() {
+ startedNodes.stream().map(Ignite::name).forEach(IgnitionManager::stop);
Review comment:
`closeAll` closes all components except those that could not be stopped.
`forEach` stops iteration immediately after the first component fils on stop.
Is it ok for the test?
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/manager/IgniteComponent.java
##########
@@ -0,0 +1,45 @@
+/*
+ * 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.manager;
+
+import org.apache.ignite.lang.NodeStoppingException;
+
+/**
+ * Common interface for ignite components that provides entry points for
component lifecycle flow.
+ */
+public interface IgniteComponent {
+ /**
+ * Starts the component. Depending on component flow both configuration
properties listeners,
+ * meta storage watch registration, starting thread pools and threads goes
here.
+ */
+ void start();
+
+ /**
+ * Triggers running before node stop logic.
Review comment:
To be honest, it is unclear from the Javadoc why this method is needed
and what should be done here.
Could it be called by the implementation, if the `start` method was not
executed?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
