You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "rpuch (via GitHub)" <gi...@apache.org> on 2024/08/29 18:11:49 UTC
[PR] IGNITE-22808 Migrate running node missed during CMG reparation to new cluster [ignite-3]
rpuch opened a new pull request, #4315:
URL: https://github.com/apache/ignite-3/pull/4315
https://issues.apache.org/jira/browse/IGNITE-22808
--
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
Re: [PR] IGNITE-22808 Migrate running node missed during CMG reparation to new cluster [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #4315:
URL: https://github.com/apache/ignite-3/pull/4315#discussion_r1739836915
##########
modules/system-disaster-recovery/src/integrationTest/java/org/apache/ignite/internal/disaster/system/ItCmgDisasterRecoveryTest.java:
##########
@@ -111,6 +128,11 @@ private CompletableFuture<Void> restartOrShutdownFuture(int nodeIndex) {
return ((IgniteServerImpl) cluster.server(nodeIndex)).restartOrShutdownFuture();
}
+ private static ClusterState clusterState(IgniteImpl restartedIgniteImpl1)
+ throws InterruptedException, ExecutionException, TimeoutException {
+ return restartedIgniteImpl1.clusterManagementGroupManager().clusterState().get(10, SECONDS);
Review Comment:
`.get(...)` declares checked exceptions. In `findLeaderNode()` we would have to handle them with a try/catch, bloating the 'just one line' to 'many lines'. But I used this in contexts where we can just declare those `throws` ourselves, so the bloating does not happen.
--
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
Re: [PR] IGNITE-22808 Migrate running node missed during CMG reparation to new cluster [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch merged PR #4315:
URL: https://github.com/apache/ignite-3/pull/4315
--
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
Re: [PR] IGNITE-22808 Migrate running node missed during CMG reparation to new cluster [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #4315:
URL: https://github.com/apache/ignite-3/pull/4315#discussion_r1739636068
##########
modules/system-disaster-recovery/src/main/java/org/apache/ignite/internal/disaster/system/SystemDisasterRecoveryManagerImpl.java:
##########
@@ -255,6 +257,37 @@ private static boolean isMajorityOfCmgAreSuccesses(
return successes >= (futuresFromNewCmg.size() + 1) / 2;
}
+ @Override
+ public CompletableFuture<Void> migrate(ClusterState targetClusterState) {
+ Collection<ClusterNode> nodesInTopology = topologyService.allMembers();
+
+ ResetClusterMessage message = buildResetClusterMessageForMigrate(targetClusterState);
+
+ Map<String, CompletableFuture<NetworkMessage>> responseFutures = sendResetClusterMessageTo(nodesInTopology, message);
+
+ return allOf(responseFutures.values())
+ .handleAsync((res, ex) -> {
+ // We ignore upstream exceptions on purpose.
+ rethrowIfError(ex);
+
+ restarter.initiateRestart();
+ return null;
+ }, restartExecutor);
+ }
+
+ private ResetClusterMessage buildResetClusterMessageForMigrate(ClusterState clusterState) {
+ List<UUID> formerClusterIds = clusterState.formerClusterIds();
+ assert formerClusterIds != null;
Review Comment:
Added validation and improved assertion message
--
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
Re: [PR] IGNITE-22808 Migrate running node missed during CMG reparation to new cluster [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #4315:
URL: https://github.com/apache/ignite-3/pull/4315#discussion_r1739636499
##########
modules/system-disaster-recovery/src/integrationTest/java/org/apache/ignite/internal/disaster/system/ItCmgDisasterRecoveryTest.java:
##########
@@ -223,4 +245,110 @@ void nodesThatSawNoReparationHaveSeparatePhysicalTopology() throws Exception {
"Nodes from different clusters were able to establish a connection"
);
}
+
+ @Test
+ void migratesNodesThatSawNoReparationToNewCluster() throws Exception {
+ cluster.startAndInit(2, paramsBuilder -> {
+ paramsBuilder.cmgNodeNames(nodeNames(0));
+ paramsBuilder.metaStorageNodeNames(nodeNames(1));
+ });
+ waitTillClusterStateIsSavedToVaultOnConductor(1);
+
+ // This makes the CMG majority go away.
+ cluster.stopNode(0);
+
+ // Repair CMG with just node 1.
+ initiateCmgRepairVia(igniteImpl(1), 1);
+ IgniteImpl restartedIgniteImpl1 = waitTillNodeRestartsInternally(1);
+ waitTillCmgHasMajority(restartedIgniteImpl1);
+
+ migrate(0, 1);
+
+ LogicalTopologySnapshot topologySnapshot = igniteImpl(1).logicalTopologyService().logicalTopologyOnLeader().get(10, SECONDS);
+ assertTopologyContainsNode(0, topologySnapshot);
+ }
+
+ private void assertTopologyContainsNode(int nodeIndex, LogicalTopologySnapshot topologySnapshot) {
+ assertTrue(topologySnapshot.nodes().stream().anyMatch(node -> node.name().equals(cluster.nodeName(nodeIndex))));
+ }
+
+ private void migrate(int oldClusterNodeIndex, int newClusterNodeIndex) throws Exception {
+ // Starting the node that did not see the repair.
+ IgniteImpl nodeMissingRepair = ((IgniteServerImpl) cluster.startEmbeddedNode(oldClusterNodeIndex).server()).igniteImpl();
+
+ initiateMigrationToNewCluster(nodeMissingRepair, igniteImpl(newClusterNodeIndex));
+
+ waitTillNodeRestartsInternally(oldClusterNodeIndex);
+ }
+
+ private static void initiateMigrationToNewCluster(IgniteImpl nodeMissingRepair, IgniteImpl repairedNode) throws Exception {
+ // TODO: IGNITE-22879 - initiate migration via CLI.
+
+ ClusterState newClusterState = clusterState(repairedNode);
+
+ CompletableFuture<Void> migrationFuture = nodeMissingRepair.systemDisasterRecoveryManager().migrate(newClusterState);
+ assertThat(migrationFuture, willCompleteSuccessfully());
+ }
+
+ @Test
+ void migratesManyNodesThatSawNoReparationToNewCluster() throws Exception {
+ cluster.startAndInit(5, paramsBuilder -> {
+ paramsBuilder.cmgNodeNames(nodeNames(0, 1, 2));
+ paramsBuilder.metaStorageNodeNames(nodeNames(2, 3, 4));
+ });
+ waitTillClusterStateIsSavedToVaultOnConductor(2);
+
+ // Stop the majority of CMG.
+ IntStream.of(0, 1).parallel().forEach(cluster::stopNode);
+
+ // Repair CMG with nodes 2, 3, 4.
+ initiateCmgRepairVia(igniteImpl(2), 2, 3, 4);
+ IgniteImpl restartedIgniteImpl2 = waitTillNodeRestartsInternally(2);
+ waitTillCmgHasMajority(restartedIgniteImpl2);
+
+ // Starting the nodes that did not see the repair.
+ List<IgniteImpl> partialNodes = IntStream.of(0, 1).parallel()
Review Comment:
Please leave comment about 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
Re: [PR] IGNITE-22808 Migrate running node missed during CMG reparation to new cluster [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #4315:
URL: https://github.com/apache/ignite-3/pull/4315#discussion_r1739619251
##########
modules/system-disaster-recovery/src/test/java/org/apache/ignite/internal/disaster/system/SystemDisasterRecoveryManagerImplTest.java:
##########
@@ -464,11 +462,72 @@ void initiatesRestartWhenGetsMessageFromOtherNode() throws Exception {
void doesNotInitiateRestartWhenGetsMessageFromSelf() throws Exception {
NetworkMessageHandler handler = extractMessageHandler();
- handler.onReceived(resetClusterMessageOn2Nodes(thisNodeName), thisNode, 0L);
+ handler.onReceived(resetClusterMessageOn2Nodes(), thisNode, 0L);
verify(restarter, never()).initiateRestart();
// Wait till it gets saved to Vault to avoid an attempt to write to it after the after-each method stops the Vault.
waitTillResetClusterMessageGetsSavedToVault();
}
+
+ @Test
+ void migrateSendsMessagesToAllNodes() {
+ ClusterState newState = newClusterState();
+
+ ArgumentCaptor<ResetClusterMessage> messageCaptor = ArgumentCaptor.forClass(ResetClusterMessage.class);
+
+ when(topologyService.allMembers()).thenReturn(List.of(thisNode, node2, node3));
+ when(messagingService.invoke(any(ClusterNode.class), any(), anyLong()))
+ .thenReturn(completedFuture(successResponseMessage));
+
+ assertThat(manager.migrate(newState), willCompleteSuccessfully());
+
+ verify(messagingService).invoke(eq(thisNode), messageCaptor.capture(), anyLong());
+ ResetClusterMessage messageToSelf = messageCaptor.getValue();
+ assertThatResetClusterMessageContentIsAsExpectedAfterMigrate(messageToSelf, newState);
+
+ verify(messagingService).invoke(eq(node2), messageCaptor.capture(), anyLong());
+ ResetClusterMessage messageToOtherNewCmgNode = messageCaptor.getValue();
+ assertThatResetClusterMessageContentIsAsExpectedAfterMigrate(messageToOtherNewCmgNode, newState);
+
+ verify(messagingService).invoke(eq(node3), messageCaptor.capture(), anyLong());
+ ResetClusterMessage messageToOtherNonCmgNode = messageCaptor.getValue();
+ assertThatResetClusterMessageContentIsAsExpectedAfterMigrate(messageToOtherNonCmgNode, newState);
+ }
+
+ private ClusterState newClusterState() {
+ return cmgMessagesFactory.clusterState()
+ .cmgNodes(Set.of("node5"))
+ .metaStorageNodes(Set.of("node6"))
+ .version(IgniteProductVersion.CURRENT_VERSION.toString())
+ .clusterTag(randomClusterTag(cmgMessagesFactory, CLUSTER_NAME))
+ .formerClusterIds(List.of(randomUUID(), randomUUID()))
+ .build();
+ }
+
+ private static void assertThatResetClusterMessageContentIsAsExpectedAfterMigrate(
+ @Nullable ResetClusterMessage message,
Review Comment:
It's a bit strange, `@Nullable` seems to me that they can pass `null` and this is valid and one of the expected values, but here it turns out to be an invalid value. I would remove the annotation, at your discretion.
##########
modules/system-disaster-recovery/src/main/java/org/apache/ignite/internal/disaster/system/SystemDisasterRecoveryManagerImpl.java:
##########
@@ -255,6 +257,37 @@ private static boolean isMajorityOfCmgAreSuccesses(
return successes >= (futuresFromNewCmg.size() + 1) / 2;
}
+ @Override
+ public CompletableFuture<Void> migrate(ClusterState targetClusterState) {
+ Collection<ClusterNode> nodesInTopology = topologyService.allMembers();
+
+ ResetClusterMessage message = buildResetClusterMessageForMigrate(targetClusterState);
+
+ Map<String, CompletableFuture<NetworkMessage>> responseFutures = sendResetClusterMessageTo(nodesInTopology, message);
+
+ return allOf(responseFutures.values())
+ .handleAsync((res, ex) -> {
+ // We ignore upstream exceptions on purpose.
+ rethrowIfError(ex);
+
+ restarter.initiateRestart();
+ return null;
+ }, restartExecutor);
+ }
+
+ private ResetClusterMessage buildResetClusterMessageForMigrate(ClusterState clusterState) {
+ List<UUID> formerClusterIds = clusterState.formerClusterIds();
+ assert formerClusterIds != null;
+
+ return messagesFactory.resetClusterMessage()
+ .cmgNodes(new HashSet<>(clusterState.cmgNodes()))
Review Comment:
Why `new HashSet<>`, looks like it's not needed
##########
modules/system-disaster-recovery/src/integrationTest/java/org/apache/ignite/internal/disaster/system/ItCmgDisasterRecoveryTest.java:
##########
@@ -111,6 +128,11 @@ private CompletableFuture<Void> restartOrShutdownFuture(int nodeIndex) {
return ((IgniteServerImpl) cluster.server(nodeIndex)).restartOrShutdownFuture();
}
+ private static ClusterState clusterState(IgniteImpl restartedIgniteImpl1)
+ throws InterruptedException, ExecutionException, TimeoutException {
+ return restartedIgniteImpl1.clusterManagementGroupManager().clusterState().get(10, SECONDS);
Review Comment:
Why not the classic `assertThat(fut, willCompleteSuccessfully())` and then `fut.join()`?
##########
modules/system-disaster-recovery/src/main/java/org/apache/ignite/internal/disaster/system/SystemDisasterRecoveryManagerImpl.java:
##########
@@ -255,6 +257,37 @@ private static boolean isMajorityOfCmgAreSuccesses(
return successes >= (futuresFromNewCmg.size() + 1) / 2;
}
+ @Override
+ public CompletableFuture<Void> migrate(ClusterState targetClusterState) {
+ Collection<ClusterNode> nodesInTopology = topologyService.allMembers();
+
+ ResetClusterMessage message = buildResetClusterMessageForMigrate(targetClusterState);
+
+ Map<String, CompletableFuture<NetworkMessage>> responseFutures = sendResetClusterMessageTo(nodesInTopology, message);
+
+ return allOf(responseFutures.values())
+ .handleAsync((res, ex) -> {
+ // We ignore upstream exceptions on purpose.
+ rethrowIfError(ex);
+
+ restarter.initiateRestart();
+ return null;
+ }, restartExecutor);
+ }
+
+ private ResetClusterMessage buildResetClusterMessageForMigrate(ClusterState clusterState) {
+ List<UUID> formerClusterIds = clusterState.formerClusterIds();
+ assert formerClusterIds != null;
Review Comment:
Maybe we can add an error message, for example why `null` is bad and what it means and on which node it happened? What do you think?
##########
modules/system-disaster-recovery/src/integrationTest/java/org/apache/ignite/internal/disaster/system/ItCmgDisasterRecoveryTest.java:
##########
@@ -223,4 +245,110 @@ void nodesThatSawNoReparationHaveSeparatePhysicalTopology() throws Exception {
"Nodes from different clusters were able to establish a connection"
);
}
+
+ @Test
+ void migratesNodesThatSawNoReparationToNewCluster() throws Exception {
+ cluster.startAndInit(2, paramsBuilder -> {
+ paramsBuilder.cmgNodeNames(nodeNames(0));
+ paramsBuilder.metaStorageNodeNames(nodeNames(1));
+ });
+ waitTillClusterStateIsSavedToVaultOnConductor(1);
+
+ // This makes the CMG majority go away.
+ cluster.stopNode(0);
+
+ // Repair CMG with just node 1.
+ initiateCmgRepairVia(igniteImpl(1), 1);
+ IgniteImpl restartedIgniteImpl1 = waitTillNodeRestartsInternally(1);
+ waitTillCmgHasMajority(restartedIgniteImpl1);
+
+ migrate(0, 1);
+
+ LogicalTopologySnapshot topologySnapshot = igniteImpl(1).logicalTopologyService().logicalTopologyOnLeader().get(10, SECONDS);
+ assertTopologyContainsNode(0, topologySnapshot);
+ }
+
+ private void assertTopologyContainsNode(int nodeIndex, LogicalTopologySnapshot topologySnapshot) {
+ assertTrue(topologySnapshot.nodes().stream().anyMatch(node -> node.name().equals(cluster.nodeName(nodeIndex))));
+ }
+
+ private void migrate(int oldClusterNodeIndex, int newClusterNodeIndex) throws Exception {
+ // Starting the node that did not see the repair.
+ IgniteImpl nodeMissingRepair = ((IgniteServerImpl) cluster.startEmbeddedNode(oldClusterNodeIndex).server()).igniteImpl();
+
+ initiateMigrationToNewCluster(nodeMissingRepair, igniteImpl(newClusterNodeIndex));
+
+ waitTillNodeRestartsInternally(oldClusterNodeIndex);
+ }
+
+ private static void initiateMigrationToNewCluster(IgniteImpl nodeMissingRepair, IgniteImpl repairedNode) throws Exception {
+ // TODO: IGNITE-22879 - initiate migration via CLI.
+
+ ClusterState newClusterState = clusterState(repairedNode);
+
+ CompletableFuture<Void> migrationFuture = nodeMissingRepair.systemDisasterRecoveryManager().migrate(newClusterState);
+ assertThat(migrationFuture, willCompleteSuccessfully());
+ }
+
+ @Test
+ void migratesManyNodesThatSawNoReparationToNewCluster() throws Exception {
+ cluster.startAndInit(5, paramsBuilder -> {
+ paramsBuilder.cmgNodeNames(nodeNames(0, 1, 2));
+ paramsBuilder.metaStorageNodeNames(nodeNames(2, 3, 4));
+ });
+ waitTillClusterStateIsSavedToVaultOnConductor(2);
+
+ // Stop the majority of CMG.
+ IntStream.of(0, 1).parallel().forEach(cluster::stopNode);
+
+ // Repair CMG with nodes 2, 3, 4.
+ initiateCmgRepairVia(igniteImpl(2), 2, 3, 4);
+ IgniteImpl restartedIgniteImpl2 = waitTillNodeRestartsInternally(2);
+ waitTillCmgHasMajority(restartedIgniteImpl2);
+
+ // Starting the nodes that did not see the repair.
+ List<IgniteImpl> partialNodes = IntStream.of(0, 1).parallel()
Review Comment:
why `.parallel()` ?
##########
modules/system-disaster-recovery/src/integrationTest/java/org/apache/ignite/internal/disaster/system/ItCmgDisasterRecoveryTest.java:
##########
@@ -223,4 +245,110 @@ void nodesThatSawNoReparationHaveSeparatePhysicalTopology() throws Exception {
"Nodes from different clusters were able to establish a connection"
);
}
+
+ @Test
+ void migratesNodesThatSawNoReparationToNewCluster() throws Exception {
+ cluster.startAndInit(2, paramsBuilder -> {
+ paramsBuilder.cmgNodeNames(nodeNames(0));
+ paramsBuilder.metaStorageNodeNames(nodeNames(1));
+ });
+ waitTillClusterStateIsSavedToVaultOnConductor(1);
+
+ // This makes the CMG majority go away.
+ cluster.stopNode(0);
+
+ // Repair CMG with just node 1.
+ initiateCmgRepairVia(igniteImpl(1), 1);
+ IgniteImpl restartedIgniteImpl1 = waitTillNodeRestartsInternally(1);
+ waitTillCmgHasMajority(restartedIgniteImpl1);
+
+ migrate(0, 1);
+
+ LogicalTopologySnapshot topologySnapshot = igniteImpl(1).logicalTopologyService().logicalTopologyOnLeader().get(10, SECONDS);
+ assertTopologyContainsNode(0, topologySnapshot);
+ }
+
+ private void assertTopologyContainsNode(int nodeIndex, LogicalTopologySnapshot topologySnapshot) {
+ assertTrue(topologySnapshot.nodes().stream().anyMatch(node -> node.name().equals(cluster.nodeName(nodeIndex))));
+ }
+
+ private void migrate(int oldClusterNodeIndex, int newClusterNodeIndex) throws Exception {
+ // Starting the node that did not see the repair.
+ IgniteImpl nodeMissingRepair = ((IgniteServerImpl) cluster.startEmbeddedNode(oldClusterNodeIndex).server()).igniteImpl();
+
+ initiateMigrationToNewCluster(nodeMissingRepair, igniteImpl(newClusterNodeIndex));
+
+ waitTillNodeRestartsInternally(oldClusterNodeIndex);
+ }
+
+ private static void initiateMigrationToNewCluster(IgniteImpl nodeMissingRepair, IgniteImpl repairedNode) throws Exception {
+ // TODO: IGNITE-22879 - initiate migration via CLI.
+
+ ClusterState newClusterState = clusterState(repairedNode);
+
+ CompletableFuture<Void> migrationFuture = nodeMissingRepair.systemDisasterRecoveryManager().migrate(newClusterState);
+ assertThat(migrationFuture, willCompleteSuccessfully());
+ }
+
+ @Test
+ void migratesManyNodesThatSawNoReparationToNewCluster() throws Exception {
+ cluster.startAndInit(5, paramsBuilder -> {
+ paramsBuilder.cmgNodeNames(nodeNames(0, 1, 2));
+ paramsBuilder.metaStorageNodeNames(nodeNames(2, 3, 4));
+ });
+ waitTillClusterStateIsSavedToVaultOnConductor(2);
+
+ // Stop the majority of CMG.
+ IntStream.of(0, 1).parallel().forEach(cluster::stopNode);
+
+ // Repair CMG with nodes 2, 3, 4.
+ initiateCmgRepairVia(igniteImpl(2), 2, 3, 4);
+ IgniteImpl restartedIgniteImpl2 = waitTillNodeRestartsInternally(2);
+ waitTillCmgHasMajority(restartedIgniteImpl2);
+
+ // Starting the nodes that did not see the repair.
+ List<IgniteImpl> partialNodes = IntStream.of(0, 1).parallel()
+ .mapToObj(index -> ((IgniteServerImpl) cluster.startEmbeddedNode(index).server()).igniteImpl())
+ .collect(toList());
+
+ initiateMigrationToNewCluster(partialNodes.get(0), igniteImpl(2));
+
+ waitTillNodeRestartsInternally(0);
+ waitTillNodeRestartsInternally(1);
+
+ LogicalTopologySnapshot topologySnapshot = igniteImpl(2).logicalTopologyService().logicalTopologyOnLeader().get(10, SECONDS);
Review Comment:
Same about `assertThat(fut, ...)`
##########
modules/system-disaster-recovery/src/integrationTest/java/org/apache/ignite/internal/disaster/system/ItCmgDisasterRecoveryTest.java:
##########
@@ -223,4 +245,110 @@ void nodesThatSawNoReparationHaveSeparatePhysicalTopology() throws Exception {
"Nodes from different clusters were able to establish a connection"
);
}
+
+ @Test
+ void migratesNodesThatSawNoReparationToNewCluster() throws Exception {
+ cluster.startAndInit(2, paramsBuilder -> {
+ paramsBuilder.cmgNodeNames(nodeNames(0));
+ paramsBuilder.metaStorageNodeNames(nodeNames(1));
+ });
+ waitTillClusterStateIsSavedToVaultOnConductor(1);
+
+ // This makes the CMG majority go away.
+ cluster.stopNode(0);
+
+ // Repair CMG with just node 1.
+ initiateCmgRepairVia(igniteImpl(1), 1);
+ IgniteImpl restartedIgniteImpl1 = waitTillNodeRestartsInternally(1);
+ waitTillCmgHasMajority(restartedIgniteImpl1);
+
+ migrate(0, 1);
+
+ LogicalTopologySnapshot topologySnapshot = igniteImpl(1).logicalTopologyService().logicalTopologyOnLeader().get(10, SECONDS);
Review Comment:
Same about `assertThat(fut, ...)`
--
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
Re: [PR] IGNITE-22808 Migrate running node missed during CMG reparation to new cluster [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #4315:
URL: https://github.com/apache/ignite-3/pull/4315#discussion_r1739635012
##########
modules/system-disaster-recovery/src/integrationTest/java/org/apache/ignite/internal/disaster/system/ItCmgDisasterRecoveryTest.java:
##########
@@ -223,4 +245,110 @@ void nodesThatSawNoReparationHaveSeparatePhysicalTopology() throws Exception {
"Nodes from different clusters were able to establish a connection"
);
}
+
+ @Test
+ void migratesNodesThatSawNoReparationToNewCluster() throws Exception {
+ cluster.startAndInit(2, paramsBuilder -> {
+ paramsBuilder.cmgNodeNames(nodeNames(0));
+ paramsBuilder.metaStorageNodeNames(nodeNames(1));
+ });
+ waitTillClusterStateIsSavedToVaultOnConductor(1);
+
+ // This makes the CMG majority go away.
+ cluster.stopNode(0);
+
+ // Repair CMG with just node 1.
+ initiateCmgRepairVia(igniteImpl(1), 1);
+ IgniteImpl restartedIgniteImpl1 = waitTillNodeRestartsInternally(1);
+ waitTillCmgHasMajority(restartedIgniteImpl1);
+
+ migrate(0, 1);
+
+ LogicalTopologySnapshot topologySnapshot = igniteImpl(1).logicalTopologyService().logicalTopologyOnLeader().get(10, SECONDS);
+ assertTopologyContainsNode(0, topologySnapshot);
+ }
+
+ private void assertTopologyContainsNode(int nodeIndex, LogicalTopologySnapshot topologySnapshot) {
+ assertTrue(topologySnapshot.nodes().stream().anyMatch(node -> node.name().equals(cluster.nodeName(nodeIndex))));
+ }
+
+ private void migrate(int oldClusterNodeIndex, int newClusterNodeIndex) throws Exception {
+ // Starting the node that did not see the repair.
+ IgniteImpl nodeMissingRepair = ((IgniteServerImpl) cluster.startEmbeddedNode(oldClusterNodeIndex).server()).igniteImpl();
+
+ initiateMigrationToNewCluster(nodeMissingRepair, igniteImpl(newClusterNodeIndex));
+
+ waitTillNodeRestartsInternally(oldClusterNodeIndex);
+ }
+
+ private static void initiateMigrationToNewCluster(IgniteImpl nodeMissingRepair, IgniteImpl repairedNode) throws Exception {
+ // TODO: IGNITE-22879 - initiate migration via CLI.
+
+ ClusterState newClusterState = clusterState(repairedNode);
+
+ CompletableFuture<Void> migrationFuture = nodeMissingRepair.systemDisasterRecoveryManager().migrate(newClusterState);
+ assertThat(migrationFuture, willCompleteSuccessfully());
+ }
+
+ @Test
+ void migratesManyNodesThatSawNoReparationToNewCluster() throws Exception {
+ cluster.startAndInit(5, paramsBuilder -> {
+ paramsBuilder.cmgNodeNames(nodeNames(0, 1, 2));
+ paramsBuilder.metaStorageNodeNames(nodeNames(2, 3, 4));
+ });
+ waitTillClusterStateIsSavedToVaultOnConductor(2);
+
+ // Stop the majority of CMG.
+ IntStream.of(0, 1).parallel().forEach(cluster::stopNode);
+
+ // Repair CMG with nodes 2, 3, 4.
+ initiateCmgRepairVia(igniteImpl(2), 2, 3, 4);
+ IgniteImpl restartedIgniteImpl2 = waitTillNodeRestartsInternally(2);
+ waitTillCmgHasMajority(restartedIgniteImpl2);
+
+ // Starting the nodes that did not see the repair.
+ List<IgniteImpl> partialNodes = IntStream.of(0, 1).parallel()
Review Comment:
To start them in parallel and save some test time
--
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
Re: [PR] IGNITE-22808 Migrate running node missed during CMG reparation to new cluster [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #4315:
URL: https://github.com/apache/ignite-3/pull/4315#discussion_r1739634893
##########
modules/system-disaster-recovery/src/integrationTest/java/org/apache/ignite/internal/disaster/system/ItCmgDisasterRecoveryTest.java:
##########
@@ -111,6 +128,11 @@ private CompletableFuture<Void> restartOrShutdownFuture(int nodeIndex) {
return ((IgniteServerImpl) cluster.server(nodeIndex)).restartOrShutdownFuture();
}
+ private static ClusterState clusterState(IgniteImpl restartedIgniteImpl1)
+ throws InterruptedException, ExecutionException, TimeoutException {
+ return restartedIgniteImpl1.clusterManagementGroupManager().clusterState().get(10, SECONDS);
Review Comment:
What you suggest requires 3 lines:
```
CompletableFuture<T> future = blablaFuture();
assertThat(future, willCompleteSuccessfully());
return future.join();
```
But with timed get we only need one line:
```
return blablaFuture().get(...);
```
The matcher-based approach is good when you don't need the result of the future, but here produces a code that is too cumbersome.
--
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
Re: [PR] IGNITE-22808 Migrate running node missed during CMG reparation to new cluster [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #4315:
URL: https://github.com/apache/ignite-3/pull/4315#discussion_r1739636992
##########
modules/system-disaster-recovery/src/integrationTest/java/org/apache/ignite/internal/disaster/system/ItCmgDisasterRecoveryTest.java:
##########
@@ -111,6 +128,11 @@ private CompletableFuture<Void> restartOrShutdownFuture(int nodeIndex) {
return ((IgniteServerImpl) cluster.server(nodeIndex)).restartOrShutdownFuture();
}
+ private static ClusterState clusterState(IgniteImpl restartedIgniteImpl1)
+ throws InterruptedException, ExecutionException, TimeoutException {
+ return restartedIgniteImpl1.clusterManagementGroupManager().clusterState().get(10, SECONDS);
Review Comment:
We use this construction quite often, I agree that it requires more lines of code. I don't insist, it's up to you.
Example: `org.apache.ignite.internal.cluster.management.ItClusterManagerTest#findLeaderNode`
--
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