rpuch commented on code in PR #6430:
URL: https://github.com/apache/ignite-3/pull/6430#discussion_r2282184292
##########
modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlCreateZoneTest.java:
##########
@@ -65,12 +78,67 @@ void
testCreateZoneSucceedWithCorrectStorageProfileOnSameNode() {
assertDoesNotThrow(() -> createZoneQuery(0, "default"));
}
+ Set<Class<? extends NetworkMessage>> allMessages =
ConcurrentHashMap.newKeySet();
+
@Test
void testCreateZoneSucceedWithCorrectStorageProfileOnDifferentNode() {
cluster.startNode(1, NODE_BOOTSTRAP_CFG_TEMPLATE_WITH_EXTRA_PROFILE);
assertDoesNotThrow(() -> createZoneQuery(0, EXTRA_PROFILE_NAME));
}
+ @Test
+ void
testCreateZoneSucceedWithCorrectStorageProfileOnDifferentNodeWithDistributedLogicalTopologyUpdate()
throws InterruptedException {
+ // Node 0 is CMG leader and Node 1 is a laggy query executor.
+ IgniteImpl node0 = unwrapIgniteImpl(node(0));
+ IgniteImpl node1 = unwrapIgniteImpl(cluster.startNode(1));
+
+ assertTrue(waitForCondition(
+ () ->
node1.logicalTopologyService().localLogicalTopology().nodes().size() == 2,
+ 10_000
+ ));
+
+ // Assert that we can't to create zone without a node with extra
profilel.
Review Comment:
```suggestion
// Assert that we can't create the zone without a node with extra
profile.
```
##########
modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlCreateZoneTest.java:
##########
@@ -65,12 +78,67 @@ void
testCreateZoneSucceedWithCorrectStorageProfileOnSameNode() {
assertDoesNotThrow(() -> createZoneQuery(0, "default"));
}
+ Set<Class<? extends NetworkMessage>> allMessages =
ConcurrentHashMap.newKeySet();
+
@Test
void testCreateZoneSucceedWithCorrectStorageProfileOnDifferentNode() {
cluster.startNode(1, NODE_BOOTSTRAP_CFG_TEMPLATE_WITH_EXTRA_PROFILE);
assertDoesNotThrow(() -> createZoneQuery(0, EXTRA_PROFILE_NAME));
}
+ @Test
+ void
testCreateZoneSucceedWithCorrectStorageProfileOnDifferentNodeWithDistributedLogicalTopologyUpdate()
throws InterruptedException {
+ // Node 0 is CMG leader and Node 1 is a laggy query executor.
+ IgniteImpl node0 = unwrapIgniteImpl(node(0));
+ IgniteImpl node1 = unwrapIgniteImpl(cluster.startNode(1));
+
+ assertTrue(waitForCondition(
+ () ->
node1.logicalTopologyService().localLogicalTopology().nodes().size() == 2,
+ 10_000
+ ));
+
+ // Assert that we can't to create zone without a node with extra
profilel.
+ assertThrowsWithCause(
+ () -> createZoneQuery(1, EXTRA_PROFILE_NAME),
+ SqlException.class,
+ "Some storage profiles don't exist [missedProfileNames=[" +
EXTRA_PROFILE_NAME + "]]."
+ );
+
+ // Node 1 won't see node 2 joined with extra profile because node 0 is
CMG leader and all RAFT-replicated messages to node 1 will be
Review Comment:
```suggestion
// Node 1 won't see node 2 joined with extra profile because node 0
is CMG leader and all CMG-related RAFT-replicated messages to node 1 will be
```
##########
modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlCreateZoneTest.java:
##########
@@ -65,12 +78,67 @@ void
testCreateZoneSucceedWithCorrectStorageProfileOnSameNode() {
assertDoesNotThrow(() -> createZoneQuery(0, "default"));
}
+ Set<Class<? extends NetworkMessage>> allMessages =
ConcurrentHashMap.newKeySet();
+
@Test
void testCreateZoneSucceedWithCorrectStorageProfileOnDifferentNode() {
cluster.startNode(1, NODE_BOOTSTRAP_CFG_TEMPLATE_WITH_EXTRA_PROFILE);
assertDoesNotThrow(() -> createZoneQuery(0, EXTRA_PROFILE_NAME));
}
+ @Test
+ void
testCreateZoneSucceedWithCorrectStorageProfileOnDifferentNodeWithDistributedLogicalTopologyUpdate()
throws InterruptedException {
+ // Node 0 is CMG leader and Node 1 is a laggy query executor.
+ IgniteImpl node0 = unwrapIgniteImpl(node(0));
+ IgniteImpl node1 = unwrapIgniteImpl(cluster.startNode(1));
+
+ assertTrue(waitForCondition(
+ () ->
node1.logicalTopologyService().localLogicalTopology().nodes().size() == 2,
+ 10_000
+ ));
+
+ // Assert that we can't to create zone without a node with extra
profilel.
+ assertThrowsWithCause(
+ () -> createZoneQuery(1, EXTRA_PROFILE_NAME),
+ SqlException.class,
+ "Some storage profiles don't exist [missedProfileNames=[" +
EXTRA_PROFILE_NAME + "]]."
+ );
+
+ // Node 1 won't see node 2 joined with extra profile because node 0 is
CMG leader and all RAFT-replicated messages to node 1 will be
+ // dropped after the code below.
+ node0.dropMessages((recipient, msg) -> msg instanceof
AppendEntriesRequest
+ && ((AppendEntriesRequest)
msg).groupId().equals(CmgGroupId.INSTANCE.toString())
+ && node1.name().equals(recipient));
+
+ // Then start node 2 with the desired extra profile.
+ cluster.startNode(2, NODE_BOOTSTRAP_CFG_TEMPLATE_WITH_EXTRA_PROFILE);
+
+ // Check that Node 1 and 2 will see all three nodes in local logical
topologies.
+ assertTrue(waitForCondition(
+ () ->
unwrapIgniteImpl(node(0)).logicalTopologyService().localLogicalTopology().nodes().size()
== 3,
+ 10_000
+ ));
+
+ assertTrue(waitForCondition(
+ () ->
unwrapIgniteImpl(node(2)).logicalTopologyService().localLogicalTopology().nodes().size()
== 3,
+ 10_000
+ ));
+
+ // And we expect that node 1 won't see node 2 in it's local logical
topology.
+ assertEquals(2,
node1.logicalTopologyService().localLogicalTopology().nodes().size());
+
+ // But still we're able to create zone with extra profile on node 2
because node 1 will try to ask CMG leader node 0 directly over
Review Comment:
```suggestion
// But still we're able to create zone with extra profile on node 2
because node 1 will try to ask CMG leader (node 0) directly over
```
##########
modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlCreateZoneTest.java:
##########
@@ -65,12 +78,67 @@ void
testCreateZoneSucceedWithCorrectStorageProfileOnSameNode() {
assertDoesNotThrow(() -> createZoneQuery(0, "default"));
}
+ Set<Class<? extends NetworkMessage>> allMessages =
ConcurrentHashMap.newKeySet();
+
@Test
void testCreateZoneSucceedWithCorrectStorageProfileOnDifferentNode() {
cluster.startNode(1, NODE_BOOTSTRAP_CFG_TEMPLATE_WITH_EXTRA_PROFILE);
assertDoesNotThrow(() -> createZoneQuery(0, EXTRA_PROFILE_NAME));
}
+ @Test
+ void
testCreateZoneSucceedWithCorrectStorageProfileOnDifferentNodeWithDistributedLogicalTopologyUpdate()
throws InterruptedException {
+ // Node 0 is CMG leader and Node 1 is a laggy query executor.
+ IgniteImpl node0 = unwrapIgniteImpl(node(0));
+ IgniteImpl node1 = unwrapIgniteImpl(cluster.startNode(1));
+
+ assertTrue(waitForCondition(
+ () ->
node1.logicalTopologyService().localLogicalTopology().nodes().size() == 2,
+ 10_000
+ ));
+
+ // Assert that we can't to create zone without a node with extra
profilel.
+ assertThrowsWithCause(
+ () -> createZoneQuery(1, EXTRA_PROFILE_NAME),
+ SqlException.class,
+ "Some storage profiles don't exist [missedProfileNames=[" +
EXTRA_PROFILE_NAME + "]]."
+ );
+
+ // Node 1 won't see node 2 joined with extra profile because node 0 is
CMG leader and all RAFT-replicated messages to node 1 will be
+ // dropped after the code below.
+ node0.dropMessages((recipient, msg) -> msg instanceof
AppendEntriesRequest
+ && ((AppendEntriesRequest)
msg).groupId().equals(CmgGroupId.INSTANCE.toString())
+ && node1.name().equals(recipient));
+
+ // Then start node 2 with the desired extra profile.
+ cluster.startNode(2, NODE_BOOTSTRAP_CFG_TEMPLATE_WITH_EXTRA_PROFILE);
+
+ // Check that Node 1 and 2 will see all three nodes in local logical
topologies.
+ assertTrue(waitForCondition(
+ () ->
unwrapIgniteImpl(node(0)).logicalTopologyService().localLogicalTopology().nodes().size()
== 3,
+ 10_000
+ ));
+
+ assertTrue(waitForCondition(
+ () ->
unwrapIgniteImpl(node(2)).logicalTopologyService().localLogicalTopology().nodes().size()
== 3,
+ 10_000
+ ));
+
+ // And we expect that node 1 won't see node 2 in it's local logical
topology.
+ assertEquals(2,
node1.logicalTopologyService().localLogicalTopology().nodes().size());
+
+ // But still we're able to create zone with extra profile on node 2
because node 1 will try to ask CMG leader node 0 directly over
+ // common network for it's up-to-date leader's local logical topology
and check this snapshot's storage profiles that should
Review Comment:
```suggestion
// the network for its up-to-date leader's local logical topology
and check this snapshot's storage profiles that should
```
##########
modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlCreateZoneTest.java:
##########
@@ -65,12 +78,67 @@ void
testCreateZoneSucceedWithCorrectStorageProfileOnSameNode() {
assertDoesNotThrow(() -> createZoneQuery(0, "default"));
}
+ Set<Class<? extends NetworkMessage>> allMessages =
ConcurrentHashMap.newKeySet();
+
@Test
void testCreateZoneSucceedWithCorrectStorageProfileOnDifferentNode() {
cluster.startNode(1, NODE_BOOTSTRAP_CFG_TEMPLATE_WITH_EXTRA_PROFILE);
assertDoesNotThrow(() -> createZoneQuery(0, EXTRA_PROFILE_NAME));
}
+ @Test
+ void
testCreateZoneSucceedWithCorrectStorageProfileOnDifferentNodeWithDistributedLogicalTopologyUpdate()
throws InterruptedException {
+ // Node 0 is CMG leader and Node 1 is a laggy query executor.
+ IgniteImpl node0 = unwrapIgniteImpl(node(0));
+ IgniteImpl node1 = unwrapIgniteImpl(cluster.startNode(1));
+
+ assertTrue(waitForCondition(
+ () ->
node1.logicalTopologyService().localLogicalTopology().nodes().size() == 2,
+ 10_000
+ ));
+
+ // Assert that we can't to create zone without a node with extra
profilel.
+ assertThrowsWithCause(
+ () -> createZoneQuery(1, EXTRA_PROFILE_NAME),
+ SqlException.class,
+ "Some storage profiles don't exist [missedProfileNames=[" +
EXTRA_PROFILE_NAME + "]]."
+ );
+
+ // Node 1 won't see node 2 joined with extra profile because node 0 is
CMG leader and all RAFT-replicated messages to node 1 will be
+ // dropped after the code below.
+ node0.dropMessages((recipient, msg) -> msg instanceof
AppendEntriesRequest
+ && ((AppendEntriesRequest)
msg).groupId().equals(CmgGroupId.INSTANCE.toString())
+ && node1.name().equals(recipient));
+
+ // Then start node 2 with the desired extra profile.
+ cluster.startNode(2, NODE_BOOTSTRAP_CFG_TEMPLATE_WITH_EXTRA_PROFILE);
+
+ // Check that Node 1 and 2 will see all three nodes in local logical
topologies.
+ assertTrue(waitForCondition(
+ () ->
unwrapIgniteImpl(node(0)).logicalTopologyService().localLogicalTopology().nodes().size()
== 3,
+ 10_000
+ ));
+
+ assertTrue(waitForCondition(
+ () ->
unwrapIgniteImpl(node(2)).logicalTopologyService().localLogicalTopology().nodes().size()
== 3,
+ 10_000
+ ));
+
+ // And we expect that node 1 won't see node 2 in it's local logical
topology.
+ assertEquals(2,
node1.logicalTopologyService().localLogicalTopology().nodes().size());
+
+ // But still we're able to create zone with extra profile on node 2
because node 1 will try to ask CMG leader node 0 directly over
+ // common network for it's up-to-date leader's local logical topology
and check this snapshot's storage profiles that should
+ // contains extra profile because 2nd node was accepted to cluster by
node 0 because it's the single CMG group follower and thus
Review Comment:
A node is either a follower or a leader, it can't be both. You probably mean
'the single CMG voting member'. Also, 'CMG' already contains 'group' in it (as
it's 'cluster management group'), so 'CMG group' is a repetition
##########
modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlCreateZoneTest.java:
##########
@@ -65,12 +78,67 @@ void
testCreateZoneSucceedWithCorrectStorageProfileOnSameNode() {
assertDoesNotThrow(() -> createZoneQuery(0, "default"));
}
+ Set<Class<? extends NetworkMessage>> allMessages =
ConcurrentHashMap.newKeySet();
+
@Test
void testCreateZoneSucceedWithCorrectStorageProfileOnDifferentNode() {
cluster.startNode(1, NODE_BOOTSTRAP_CFG_TEMPLATE_WITH_EXTRA_PROFILE);
assertDoesNotThrow(() -> createZoneQuery(0, EXTRA_PROFILE_NAME));
}
+ @Test
+ void
testCreateZoneSucceedWithCorrectStorageProfileOnDifferentNodeWithDistributedLogicalTopologyUpdate()
throws InterruptedException {
+ // Node 0 is CMG leader and Node 1 is a laggy query executor.
+ IgniteImpl node0 = unwrapIgniteImpl(node(0));
+ IgniteImpl node1 = unwrapIgniteImpl(cluster.startNode(1));
+
+ assertTrue(waitForCondition(
+ () ->
node1.logicalTopologyService().localLogicalTopology().nodes().size() == 2,
+ 10_000
+ ));
+
+ // Assert that we can't to create zone without a node with extra
profilel.
+ assertThrowsWithCause(
+ () -> createZoneQuery(1, EXTRA_PROFILE_NAME),
+ SqlException.class,
+ "Some storage profiles don't exist [missedProfileNames=[" +
EXTRA_PROFILE_NAME + "]]."
+ );
+
+ // Node 1 won't see node 2 joined with extra profile because node 0 is
CMG leader and all RAFT-replicated messages to node 1 will be
+ // dropped after the code below.
+ node0.dropMessages((recipient, msg) -> msg instanceof
AppendEntriesRequest
+ && ((AppendEntriesRequest)
msg).groupId().equals(CmgGroupId.INSTANCE.toString())
+ && node1.name().equals(recipient));
+
+ // Then start node 2 with the desired extra profile.
+ cluster.startNode(2, NODE_BOOTSTRAP_CFG_TEMPLATE_WITH_EXTRA_PROFILE);
+
+ // Check that Node 1 and 2 will see all three nodes in local logical
topologies.
+ assertTrue(waitForCondition(
+ () ->
unwrapIgniteImpl(node(0)).logicalTopologyService().localLogicalTopology().nodes().size()
== 3,
+ 10_000
+ ));
+
+ assertTrue(waitForCondition(
+ () ->
unwrapIgniteImpl(node(2)).logicalTopologyService().localLogicalTopology().nodes().size()
== 3,
+ 10_000
+ ));
+
+ // And we expect that node 1 won't see node 2 in it's local logical
topology.
+ assertEquals(2,
node1.logicalTopologyService().localLogicalTopology().nodes().size());
+
+ // But still we're able to create zone with extra profile on node 2
because node 1 will try to ask CMG leader node 0 directly over
+ // common network for it's up-to-date leader's local logical topology
and check this snapshot's storage profiles that should
+ // contains extra profile because 2nd node was accepted to cluster by
node 0 because it's the single CMG group follower and thus
+ // the leader.
+ assertDoesNotThrow(() -> createZoneQuery(1, EXTRA_PROFILE_NAME));
+ }
+
+ @AfterEach
+ void log() {
+ Loggers.forClass(ItSqlCreateZoneTest.class).info("!! All messages:
{}", allMessages);
Review Comment:
This looks like a temporary logging. Is the PR finished?
##########
modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlCreateZoneTest.java:
##########
@@ -65,12 +78,67 @@ void
testCreateZoneSucceedWithCorrectStorageProfileOnSameNode() {
assertDoesNotThrow(() -> createZoneQuery(0, "default"));
}
+ Set<Class<? extends NetworkMessage>> allMessages =
ConcurrentHashMap.newKeySet();
+
@Test
void testCreateZoneSucceedWithCorrectStorageProfileOnDifferentNode() {
cluster.startNode(1, NODE_BOOTSTRAP_CFG_TEMPLATE_WITH_EXTRA_PROFILE);
assertDoesNotThrow(() -> createZoneQuery(0, EXTRA_PROFILE_NAME));
}
+ @Test
+ void
testCreateZoneSucceedWithCorrectStorageProfileOnDifferentNodeWithDistributedLogicalTopologyUpdate()
throws InterruptedException {
+ // Node 0 is CMG leader and Node 1 is a laggy query executor.
+ IgniteImpl node0 = unwrapIgniteImpl(node(0));
+ IgniteImpl node1 = unwrapIgniteImpl(cluster.startNode(1));
+
+ assertTrue(waitForCondition(
+ () ->
node1.logicalTopologyService().localLogicalTopology().nodes().size() == 2,
+ 10_000
+ ));
+
+ // Assert that we can't to create zone without a node with extra
profilel.
+ assertThrowsWithCause(
+ () -> createZoneQuery(1, EXTRA_PROFILE_NAME),
+ SqlException.class,
+ "Some storage profiles don't exist [missedProfileNames=[" +
EXTRA_PROFILE_NAME + "]]."
+ );
+
+ // Node 1 won't see node 2 joined with extra profile because node 0 is
CMG leader and all RAFT-replicated messages to node 1 will be
+ // dropped after the code below.
+ node0.dropMessages((recipient, msg) -> msg instanceof
AppendEntriesRequest
+ && ((AppendEntriesRequest)
msg).groupId().equals(CmgGroupId.INSTANCE.toString())
+ && node1.name().equals(recipient));
+
+ // Then start node 2 with the desired extra profile.
+ cluster.startNode(2, NODE_BOOTSTRAP_CFG_TEMPLATE_WITH_EXTRA_PROFILE);
+
+ // Check that Node 1 and 2 will see all three nodes in local logical
topologies.
+ assertTrue(waitForCondition(
+ () ->
unwrapIgniteImpl(node(0)).logicalTopologyService().localLogicalTopology().nodes().size()
== 3,
+ 10_000
+ ));
+
+ assertTrue(waitForCondition(
+ () ->
unwrapIgniteImpl(node(2)).logicalTopologyService().localLogicalTopology().nodes().size()
== 3,
+ 10_000
+ ));
+
+ // And we expect that node 1 won't see node 2 in it's local logical
topology.
Review Comment:
```suggestion
// And we expect that node 1 won't see node 2 in its local logical
topology.
```
##########
modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlCreateZoneTest.java:
##########
@@ -65,12 +78,67 @@ void
testCreateZoneSucceedWithCorrectStorageProfileOnSameNode() {
assertDoesNotThrow(() -> createZoneQuery(0, "default"));
}
+ Set<Class<? extends NetworkMessage>> allMessages =
ConcurrentHashMap.newKeySet();
+
@Test
void testCreateZoneSucceedWithCorrectStorageProfileOnDifferentNode() {
cluster.startNode(1, NODE_BOOTSTRAP_CFG_TEMPLATE_WITH_EXTRA_PROFILE);
assertDoesNotThrow(() -> createZoneQuery(0, EXTRA_PROFILE_NAME));
}
+ @Test
+ void
testCreateZoneSucceedWithCorrectStorageProfileOnDifferentNodeWithDistributedLogicalTopologyUpdate()
throws InterruptedException {
+ // Node 0 is CMG leader and Node 1 is a laggy query executor.
+ IgniteImpl node0 = unwrapIgniteImpl(node(0));
+ IgniteImpl node1 = unwrapIgniteImpl(cluster.startNode(1));
+
+ assertTrue(waitForCondition(
+ () ->
node1.logicalTopologyService().localLogicalTopology().nodes().size() == 2,
+ 10_000
+ ));
+
+ // Assert that we can't to create zone without a node with extra
profilel.
+ assertThrowsWithCause(
+ () -> createZoneQuery(1, EXTRA_PROFILE_NAME),
+ SqlException.class,
+ "Some storage profiles don't exist [missedProfileNames=[" +
EXTRA_PROFILE_NAME + "]]."
+ );
+
+ // Node 1 won't see node 2 joined with extra profile because node 0 is
CMG leader and all RAFT-replicated messages to node 1 will be
+ // dropped after the code below.
+ node0.dropMessages((recipient, msg) -> msg instanceof
AppendEntriesRequest
+ && ((AppendEntriesRequest)
msg).groupId().equals(CmgGroupId.INSTANCE.toString())
+ && node1.name().equals(recipient));
+
+ // Then start node 2 with the desired extra profile.
+ cluster.startNode(2, NODE_BOOTSTRAP_CFG_TEMPLATE_WITH_EXTRA_PROFILE);
+
+ // Check that Node 1 and 2 will see all three nodes in local logical
topologies.
Review Comment:
```suggestion
// Check that Node 0 and 2 will see all three nodes in local logical
topologies.
```
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/ddl/ClusterWideStorageProfileValidator.java:
##########
@@ -47,6 +51,26 @@ public void validate(Collection<String> storageProfiles) {
localLogicalTopologySnapshot
);
+ if (missedStorageProfileNames.isEmpty()) {
+ return;
+ }
+
+ try {
+ missedStorageProfileNames =
logicalTopologyService.logicalTopologyOnLeader()
+ .thenApply(topologySnapshot ->
findStorageProfileNotPresentedInLogicalTopologySnapshot(
+ storageProfiles,
+ topologySnapshot
+ )).get(10, TimeUnit.SECONDS);
+ } catch (InterruptedException | ExecutionException | TimeoutException
e) {
+ String msg = format(
Review Comment:
If we catch an `InterruptedException`, we should call
`Thread.currentThread().interrupt()` to restore the interruption flag. But I
agree, blocking here is something that we should probably avoid.
--
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
