alievmirza commented on code in PR #6651:
URL: https://github.com/apache/ignite-3/pull/6651#discussion_r2415845639
##########
modules/compatibility-tests/src/integrationTest/java/org/apache/ignite/internal/ItApplyPartitionRaftLogOnAnotherNodesCompatibilityTest.java:
##########
@@ -74,9 +83,77 @@ void testIncreaseReplicas() throws Exception {
sql(String.format("ALTER ZONE %s SET REPLICAS=3,
DATA_NODES_FILTER='$..*'", ZONE_NAME));
// Let's wait for replication to complete on other nodes.
- Thread.sleep(3_000);
+ waitForZoneState(ZONE_NAME, zone -> zone.replicas() == 3);
assertThat(sql(cluster.node(1), String.format("SELECT * FROM %s",
TABLE_NAME)), hasSize(10));
assertThat(sql(cluster.node(2), String.format("SELECT * FROM %s",
TABLE_NAME)), hasSize(10));
}
+
+ @Test
+ void testDataNodesChange() throws Exception {
Review Comment:
This test should be in a different class, your test has nothing in common
with raft logic that is stated in the class name
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/DataNodesManager.java:
##########
@@ -258,7 +280,17 @@ CompletableFuture<Void> startAsync(
onScaleDownTimerChange(zone, scaleDownTimer);
restorePartitionResetTimer(zone.id(), scaleDownTimer,
recoveryRevision);
}
- });
+ })
+ .thenCompose(unused ->
allOf(legacyInitFutures.stream().map(IgniteBiTuple::getValue).collect(toList()))
+ .handle((v, e) -> {
+ if (e != null) {
+ LOG.error("Could not recover legacy data nodes
for zone.", e);
Review Comment:
Any error that we print must be actionable, meaning that user must
understand what to do in case of error. This message doesn't bring any value
##########
modules/compatibility-tests/src/integrationTest/java/org/apache/ignite/internal/ItApplyPartitionRaftLogOnAnotherNodesCompatibilityTest.java:
##########
@@ -74,9 +83,77 @@ void testIncreaseReplicas() throws Exception {
sql(String.format("ALTER ZONE %s SET REPLICAS=3,
DATA_NODES_FILTER='$..*'", ZONE_NAME));
// Let's wait for replication to complete on other nodes.
- Thread.sleep(3_000);
+ waitForZoneState(ZONE_NAME, zone -> zone.replicas() == 3);
assertThat(sql(cluster.node(1), String.format("SELECT * FROM %s",
TABLE_NAME)), hasSize(10));
assertThat(sql(cluster.node(2), String.format("SELECT * FROM %s",
TABLE_NAME)), hasSize(10));
}
+
+ @Test
+ void testDataNodesChange() throws Exception {
Review Comment:
I would expect at least tow tests: the one that tests replica
number/filter/adjust change and that data nodes are restored correctly, the
second test must check opposite situation when none of replica
number/filter/adjust change happens and even in that case data nodes restored
correctly. Also we should check that further cluster restarts won't rewrite
correct data nodes with old value.
##########
modules/compatibility-tests/src/integrationTest/java/org/apache/ignite/internal/ItApplyPartitionRaftLogOnAnotherNodesCompatibilityTest.java:
##########
@@ -74,9 +83,77 @@ void testIncreaseReplicas() throws Exception {
sql(String.format("ALTER ZONE %s SET REPLICAS=3,
DATA_NODES_FILTER='$..*'", ZONE_NAME));
// Let's wait for replication to complete on other nodes.
- Thread.sleep(3_000);
+ waitForZoneState(ZONE_NAME, zone -> zone.replicas() == 3);
assertThat(sql(cluster.node(1), String.format("SELECT * FROM %s",
TABLE_NAME)), hasSize(10));
assertThat(sql(cluster.node(2), String.format("SELECT * FROM %s",
TABLE_NAME)), hasSize(10));
}
+
+ @Test
+ void testDataNodesChange() throws Exception {
Review Comment:
also, please write javadocs in tests and explain briefly test case.
--
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]
