SammyVimes commented on code in PR #1604:
URL: https://github.com/apache/ignite-3/pull/1604#discussion_r1092922968
##########
modules/api/src/main/java/org/apache/ignite/network/TopologyService.java:
##########
@@ -22,6 +22,14 @@
/**
* Entry point for obtaining information about a cluster's physical topology.
+ *
+ * <p>Physical topology (PT) is the set of nodes that are thought to be
reachable (from the point of view of this node)
+ * for message sending. More precisely, a node appears in the PT when it gets
discovered by an underlying
+ * discovery mechanism (like SWIM) and disappears from the PT when it
disappears from the Logical Topology (LT).
+ * This means that a node might still be in the PT when it is not actually
reachable anymore (i.e. due to a network partition).
+ *
+ * <p>A node that was removed from the PT due to being removed from the LT
cannot reappear in the PT with same node ID
Review Comment:
We should probably decide on the naming (I mean node id vs launch id)
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/compute/ItLogicalTopologyTest.java:
##########
@@ -170,6 +175,96 @@ public void onDisappeared(ClusterNode disappearedNode,
LogicalTopologySnapshot n
assertTrue(secondIgniteDisappeared.await(10, TimeUnit.SECONDS), "Did
not see second node leaving in time");
}
+ @Test
+
@BootstrapConfigTemplateMethod("templateWithVeryLongDelayToRemoveFromLogicalTopology")
Review Comment:
I personally think that it's ok for test. The clearer -- the better.
##########
modules/network/src/integrationTest/java/org/apache/ignite/network/scalecube/ItScaleCubeNetworkMessagingTest.java:
##########
@@ -320,6 +322,39 @@ public void testMessageGroupsHandlers(TestInfo testInfo)
throws Exception {
assertThat(networkMessageFuture, willBe(equalTo(networkMessage)));
}
+ /**
+ * Tests removal from physical topology.
+ *
+ * @param testInfo Test info.
+ * @throws Exception If failed.
+ */
+ @Test
+ public void removalFromPhysicalTopologyRemovesAndFiresEvent(TestInfo
testInfo) throws Exception {
+ testCluster = new Cluster(2, testInfo);
+ testCluster.startAwait();
+
+ ClusterService alice = testCluster.members.get(0);
+ ClusterService bob = testCluster.members.get(1);
+ String aliceName = alice.localConfiguration().getName();
+
+ var aliceShutdownLatch = new CountDownLatch(1);
Review Comment:
It seems like there is now await on this latch
##########
modules/raft/src/test/java/org/apache/ignite/raft/jraft/rpc/IgniteRpcTest.java:
##########
@@ -107,6 +110,15 @@ public IgniteRpcTest(TestInfo testInfo) {
service.start();
+ // Remove a node from Physical Topology as soon as it gets lost by
SWIM.
+ InternalTopologyService internalTopologyService =
(InternalTopologyService) service.topologyService();
+ internalTopologyService.addDiscoveryEventListener(new
DiscoveryTopologyEventListener() {
+ @Override
+ public void onDisappeared(ClusterNode member) {
+ internalTopologyService.removeFromPhysicalTopology(member);
Review Comment:
I don't get it. We set a listener in the internalTopologyService to update
internalTopologyService? Why can't internalTopologyService do it by itself?
--
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]
