sanpwc commented on code in PR #6243:
URL: https://github.com/apache/ignite-3/pull/6243#discussion_r2213700940
##########
modules/runner/src/testFixtures/java/org/apache/ignite/internal/BaseIgniteRestartTest.java:
##########
@@ -139,21 +146,93 @@ void setUp(TestInfo testInfo) {
public void afterEachTest() throws Exception {
var closeables = new ArrayList<AutoCloseable>();
- for (IgniteServer node : IGNITE_SERVERS) {
- if (node != null) {
- closeables.add(node::shutdown);
+ List<String> serverNames = IGNITE_SERVERS.stream()
+ .filter(Objects::nonNull)
+ .map(IgniteServer::name)
+ .collect(toList());
+
+ List<String> partialNodeNames = this.partialNodes.stream()
+ .filter(Objects::nonNull)
+ .map(PartialNode::name)
+ .collect(toList());
+
+ log.info("Shutting the cluster down [serverNodes={},
partialNodes={}]", serverNames, partialNodeNames);
+
+ Set<String> cmgMsNodesNames = new HashSet<>();
+
+ Set<String> cmgMsPartialNodesNames = new HashSet<>();
+
+ if (!partialNodes.isEmpty()) {
+ PartialNode anyPartialNode = partialNodes.get(0);
+
+ ClusterManagementGroupManager component = findComponent(
+ anyPartialNode.startedComponents(),
+ ClusterManagementGroupManager.class
+ );
+
+ cmgMsPartialNodesNames = msCmgNodes(component);
+
+ for (PartialNode partialNode : partialNodes) {
+ if (!cmgMsPartialNodesNames.contains(partialNode.name())) {
+ closeables.add(partialNode::stop);
+ }
}
}
+ if (!IGNITE_SERVERS.isEmpty()) {
+ IgniteServer anyNode = IGNITE_SERVERS.get(0);
+
+ IgniteImpl ignite = unwrapIgniteImpl(anyNode.api());
+
+ cmgMsNodesNames =
msCmgNodes(ignite.clusterManagementGroupManager());
+
+ for (IgniteServer node : IGNITE_SERVERS) {
+ if (node != null) {
+ if (!cmgMsNodesNames.contains(node.name())) {
+ closeables.add(node::shutdown);
+ }
+ }
+ }
+ }
+
+ // Add CMG/MS nodes at the end of the list to ensure that they are
stopped last.
if (!partialNodes.isEmpty()) {
for (PartialNode partialNode : partialNodes) {
Review Comment:
I'd rather have one iteration per partialNodes and one per common nodes
within which I'd prepare both cmgMG list and !cmgMg list. When iteration is
finished I'd populate closables with cmgMG and !cmgMg in proper order. Meaning
that there will be 2 iterations instead of 4.
Please pay attention to may next comment
> It's not guaranteed that closeAll will stop the nodes in proper order,
thus, I guess you will need to do it explicitly, meaning that you will need two
closable lists.
##########
modules/runner/src/testFixtures/java/org/apache/ignite/internal/BaseIgniteRestartTest.java:
##########
@@ -139,21 +146,93 @@ void setUp(TestInfo testInfo) {
public void afterEachTest() throws Exception {
var closeables = new ArrayList<AutoCloseable>();
- for (IgniteServer node : IGNITE_SERVERS) {
- if (node != null) {
- closeables.add(node::shutdown);
+ List<String> serverNames = IGNITE_SERVERS.stream()
+ .filter(Objects::nonNull)
+ .map(IgniteServer::name)
+ .collect(toList());
+
+ List<String> partialNodeNames = this.partialNodes.stream()
+ .filter(Objects::nonNull)
+ .map(PartialNode::name)
+ .collect(toList());
+
+ log.info("Shutting the cluster down [serverNodes={},
partialNodes={}]", serverNames, partialNodeNames);
+
+ Set<String> cmgMsNodesNames = new HashSet<>();
Review Comment:
Let's be consistent in msCmg and cmgMs naming.
##########
modules/runner/src/testFixtures/java/org/apache/ignite/internal/BaseIgniteRestartTest.java:
##########
@@ -139,21 +146,93 @@ void setUp(TestInfo testInfo) {
public void afterEachTest() throws Exception {
var closeables = new ArrayList<AutoCloseable>();
- for (IgniteServer node : IGNITE_SERVERS) {
- if (node != null) {
- closeables.add(node::shutdown);
+ List<String> serverNames = IGNITE_SERVERS.stream()
+ .filter(Objects::nonNull)
+ .map(IgniteServer::name)
+ .collect(toList());
+
+ List<String> partialNodeNames = this.partialNodes.stream()
+ .filter(Objects::nonNull)
+ .map(PartialNode::name)
+ .collect(toList());
+
+ log.info("Shutting the cluster down [serverNodes={},
partialNodes={}]", serverNames, partialNodeNames);
+
+ Set<String> cmgMsNodesNames = new HashSet<>();
+
+ Set<String> cmgMsPartialNodesNames = new HashSet<>();
+
+ if (!partialNodes.isEmpty()) {
+ PartialNode anyPartialNode = partialNodes.get(0);
+
+ ClusterManagementGroupManager component = findComponent(
+ anyPartialNode.startedComponents(),
+ ClusterManagementGroupManager.class
+ );
+
+ cmgMsPartialNodesNames = msCmgNodes(component);
+
+ for (PartialNode partialNode : partialNodes) {
+ if (!cmgMsPartialNodesNames.contains(partialNode.name())) {
+ closeables.add(partialNode::stop);
+ }
}
}
+ if (!IGNITE_SERVERS.isEmpty()) {
+ IgniteServer anyNode = IGNITE_SERVERS.get(0);
+
+ IgniteImpl ignite = unwrapIgniteImpl(anyNode.api());
+
+ cmgMsNodesNames =
msCmgNodes(ignite.clusterManagementGroupManager());
+
+ for (IgniteServer node : IGNITE_SERVERS) {
+ if (node != null) {
+ if (!cmgMsNodesNames.contains(node.name())) {
+ closeables.add(node::shutdown);
+ }
+ }
+ }
+ }
+
+ // Add CMG/MS nodes at the end of the list to ensure that they are
stopped last.
if (!partialNodes.isEmpty()) {
for (PartialNode partialNode : partialNodes) {
- closeables.add(partialNode::stop);
+ if (cmgMsPartialNodesNames.contains(partialNode.name())) {
+ closeables.add(partialNode::stop);
+ }
+ }
+ }
+
+ if (!IGNITE_SERVERS.isEmpty()) {
+ for (IgniteServer node : IGNITE_SERVERS) {
+ if (node != null) {
+ if (cmgMsNodesNames.contains(node.name())) {
+ closeables.add(node::shutdown);
+ }
+ }
}
}
closeAll(closeables);
IGNITE_SERVERS.clear();
+ partialNodes.clear();
+ }
+
+ private static Set<String> msCmgNodes(ClusterManagementGroupManager
ignite) throws Exception {
Review Comment:
Could you please add the javadoc?
##########
modules/runner/src/testFixtures/java/org/apache/ignite/internal/BaseIgniteRestartTest.java:
##########
@@ -139,21 +146,93 @@ void setUp(TestInfo testInfo) {
public void afterEachTest() throws Exception {
var closeables = new ArrayList<AutoCloseable>();
- for (IgniteServer node : IGNITE_SERVERS) {
- if (node != null) {
- closeables.add(node::shutdown);
+ List<String> serverNames = IGNITE_SERVERS.stream()
+ .filter(Objects::nonNull)
+ .map(IgniteServer::name)
+ .collect(toList());
+
+ List<String> partialNodeNames = this.partialNodes.stream()
+ .filter(Objects::nonNull)
+ .map(PartialNode::name)
+ .collect(toList());
+
+ log.info("Shutting the cluster down [serverNodes={},
partialNodes={}]", serverNames, partialNodeNames);
+
+ Set<String> cmgMsNodesNames = new HashSet<>();
+
+ Set<String> cmgMsPartialNodesNames = new HashSet<>();
+
+ if (!partialNodes.isEmpty()) {
+ PartialNode anyPartialNode = partialNodes.get(0);
+
+ ClusterManagementGroupManager component = findComponent(
+ anyPartialNode.startedComponents(),
+ ClusterManagementGroupManager.class
+ );
+
+ cmgMsPartialNodesNames = msCmgNodes(component);
+
+ for (PartialNode partialNode : partialNodes) {
+ if (!cmgMsPartialNodesNames.contains(partialNode.name())) {
+ closeables.add(partialNode::stop);
+ }
}
}
+ if (!IGNITE_SERVERS.isEmpty()) {
+ IgniteServer anyNode = IGNITE_SERVERS.get(0);
+
+ IgniteImpl ignite = unwrapIgniteImpl(anyNode.api());
+
+ cmgMsNodesNames =
msCmgNodes(ignite.clusterManagementGroupManager());
+
+ for (IgniteServer node : IGNITE_SERVERS) {
+ if (node != null) {
+ if (!cmgMsNodesNames.contains(node.name())) {
+ closeables.add(node::shutdown);
+ }
+ }
+ }
+ }
+
+ // Add CMG/MS nodes at the end of the list to ensure that they are
stopped last.
if (!partialNodes.isEmpty()) {
for (PartialNode partialNode : partialNodes) {
- closeables.add(partialNode::stop);
+ if (cmgMsPartialNodesNames.contains(partialNode.name())) {
+ closeables.add(partialNode::stop);
+ }
+ }
+ }
+
+ if (!IGNITE_SERVERS.isEmpty()) {
+ for (IgniteServer node : IGNITE_SERVERS) {
+ if (node != null) {
+ if (cmgMsNodesNames.contains(node.name())) {
+ closeables.add(node::shutdown);
+ }
+ }
}
}
closeAll(closeables);
Review Comment:
It's not guaranteed that closeAll will stop the nodes in proper order, thus,
I guess you will need to do it explicitly, meaning that you will need two
closable lists.
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/ItIgniteInMemoryNodeRestartTest.java:
##########
@@ -85,99 +74,15 @@ public class ItIgniteInMemoryNodeRestartTest extends
BaseIgniteRestartTest {
/** Test table name. */
private static final String TABLE_NAME = "Table1";
- /** Cluster nodes. */
- private static final List<Ignite> CLUSTER_NODES = new ArrayList<>();
-
- /**
- * Stops all started nodes.
- */
- @AfterEach
- public void afterEach() throws Exception {
- var closeables = new ArrayList<AutoCloseable>();
-
- for (IgniteServer node : IGNITE_SERVERS) {
- if (node != null) {
- closeables.add(node::shutdown);
- }
- }
-
- IgniteUtils.closeAll(closeables);
-
- CLUSTER_NODES.clear();
- }
-
- /**
- * Start node with the given parameters.
- *
- * @param idx Node index, is used to stop the node later, see {@link
#stopNode(int)}.
- * @param nodeName Node name.
- * @param cfgString Configuration string.
- * @param workDir Working directory.
- * @return Created node instance.
- */
- private static IgniteImpl startNode(int idx, String nodeName, @Nullable
String cfgString, Path workDir) {
Review Comment:
Probably I've missed that, why it's remvoved?
--
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-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
