You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "sanpwc (via GitHub)" <gi...@apache.org> on 2023/06/29 08:12:02 UTC
[GitHub] [ignite-3] sanpwc commented on a diff in pull request #2264: IGNITE-19857 Default zone replica count is not listened by rebalance trigger
sanpwc commented on code in PR #2264:
URL: https://github.com/apache/ignite-3/pull/2264#discussion_r1246240312
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/rebalance/DistributionZoneRebalanceEngine.java:
##########
@@ -85,6 +86,8 @@ public class DistributionZoneRebalanceEngine {
/** Meta storage listener for data nodes changes. */
private final WatchListener dataNodesListener;
+ private final ConfigurationListener<Integer> onUpdateReplicas = this::onUpdateReplicas;
Review Comment:
Just curious, why we need this? In order not to write this::onUpdateReplicas?
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/rebalance/DistributionZoneRebalanceEngineTest.java:
##########
@@ -393,6 +368,72 @@ void staleDataNodesEvent(
verify(keyValueStorage, timeout(1000).times(2)).invoke(any(), any());
}
+ @Test
+ void replicasTriggersAssignmentsChangingOnNonDefaultZones(
+ @InjectConfiguration
+ ("mock.tables {"
+ + "table0 = { zoneId = 1 },"
+ + "table1 = { zoneId = 1 },"
+ + "table2 = { zoneId = 2 }}")
+ TablesConfiguration tablesConfiguration
+ ) {
+ assignTableIds(tablesConfiguration);
+
+ when(distributionZoneManager.dataNodes(anyInt())).thenReturn(Set.of("node0"));
+
+ createRebalanceEngine(tablesConfiguration);
+
+ rebalanceEngine.start();
Review Comment:
Should we also stop it?
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/rebalance/DistributionZoneRebalanceEngineTest.java:
##########
@@ -393,6 +368,72 @@ void staleDataNodesEvent(
verify(keyValueStorage, timeout(1000).times(2)).invoke(any(), any());
}
+ @Test
+ void replicasTriggersAssignmentsChangingOnNonDefaultZones(
+ @InjectConfiguration
+ ("mock.tables {"
+ + "table0 = { zoneId = 1 },"
+ + "table1 = { zoneId = 1 },"
+ + "table2 = { zoneId = 2 }}")
+ TablesConfiguration tablesConfiguration
+ ) {
+ assignTableIds(tablesConfiguration);
+
+ when(distributionZoneManager.dataNodes(anyInt())).thenReturn(Set.of("node0"));
+
+ createRebalanceEngine(tablesConfiguration);
+
+ rebalanceEngine.start();
+
+ CompletableFuture<Void> changeFuture = distributionZonesConfiguration.change(zonesChange -> zonesChange.changeDistributionZones(
+ zoneListChange -> zoneListChange.update("zone0", zoneChange -> zoneChange.changeReplicas(2))
+ ));
+ assertThat(changeFuture, willCompleteSuccessfully());
+
+ verify(keyValueStorage, timeout(1000).times(2)).invoke(any(), any());
Review Comment:
So, we are checking that invoke is called on keyValueStorage assuming that within given test context it might be called only within rebalance assignments test, right? Looks unreliable to me.
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/rebalance/DistributionZoneRebalanceEngine.java:
##########
@@ -311,7 +318,7 @@ private CompletableFuture<?> onUpdateReplicas(ConfigurationNotificationEvent<Int
replicasCtx.storageRevision(),
metaStorageManager,
partId,
- tableAssignments.get(partId)
+ tableAssignments.isEmpty() ? emptySet() : tableAssignments.get(partId)
Review Comment:
I don't think that we support empty assignments processing. That should be addressed at least in https://issues.apache.org/jira/browse/IGNITE-19466 and https://issues.apache.org/jira/browse/IGNITE-19763, so for now it's better to get NPE here, because otherwise we may miss it.
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/rebalance/DistributionZoneRebalanceEngineTest.java:
##########
@@ -393,6 +368,72 @@ void staleDataNodesEvent(
verify(keyValueStorage, timeout(1000).times(2)).invoke(any(), any());
}
+ @Test
+ void replicasTriggersAssignmentsChangingOnNonDefaultZones(
+ @InjectConfiguration
+ ("mock.tables {"
+ + "table0 = { zoneId = 1 },"
+ + "table1 = { zoneId = 1 },"
+ + "table2 = { zoneId = 2 }}")
+ TablesConfiguration tablesConfiguration
+ ) {
+ assignTableIds(tablesConfiguration);
+
+ when(distributionZoneManager.dataNodes(anyInt())).thenReturn(Set.of("node0"));
+
+ createRebalanceEngine(tablesConfiguration);
+
+ rebalanceEngine.start();
+
+ CompletableFuture<Void> changeFuture = distributionZonesConfiguration.change(zonesChange -> zonesChange.changeDistributionZones(
+ zoneListChange -> zoneListChange.update("zone0", zoneChange -> zoneChange.changeReplicas(2))
+ ));
+ assertThat(changeFuture, willCompleteSuccessfully());
+
+ verify(keyValueStorage, timeout(1000).times(2)).invoke(any(), any());
+ }
+
+ @Test
+ void replicasTriggersAssignmentsChangingOnDefaultZone(
Review Comment:
Did you consider test parametrization for replicasTriggersAssignmentsChangingOnNonDefaultZones and replicasTriggersAssignmentsChangingOnDefaultZone?
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/rebalance/DistributionZoneRebalanceEngineTest.java:
##########
@@ -393,6 +368,72 @@ void staleDataNodesEvent(
verify(keyValueStorage, timeout(1000).times(2)).invoke(any(), any());
}
+ @Test
+ void replicasTriggersAssignmentsChangingOnNonDefaultZones(
+ @InjectConfiguration
+ ("mock.tables {"
+ + "table0 = { zoneId = 1 },"
+ + "table1 = { zoneId = 1 },"
+ + "table2 = { zoneId = 2 }}")
+ TablesConfiguration tablesConfiguration
+ ) {
+ assignTableIds(tablesConfiguration);
+
+ when(distributionZoneManager.dataNodes(anyInt())).thenReturn(Set.of("node0"));
+
+ createRebalanceEngine(tablesConfiguration);
+
+ rebalanceEngine.start();
+
+ CompletableFuture<Void> changeFuture = distributionZonesConfiguration.change(zonesChange -> zonesChange.changeDistributionZones(
+ zoneListChange -> zoneListChange.update("zone0", zoneChange -> zoneChange.changeReplicas(2))
+ ));
+ assertThat(changeFuture, willCompleteSuccessfully());
+
+ verify(keyValueStorage, timeout(1000).times(2)).invoke(any(), any());
+ }
+
+ @Test
+ void replicasTriggersAssignmentsChangingOnDefaultZone(
+ @InjectConfiguration
+ ("mock.tables {"
+ + "table0 = { zoneId = 0 },"
+ + "table1 = { zoneId = 0 },"
+ + "table2 = { zoneId = 2 }}")
+ TablesConfiguration tablesConfiguration
+ ) {
+ assignTableIds(tablesConfiguration);
+
+ when(distributionZoneManager.dataNodes(anyInt())).thenReturn(Set.of("node0"));
+
+ createRebalanceEngine(tablesConfiguration);
+
+ rebalanceEngine.start();
+
+ CompletableFuture<Void> changeFuture = distributionZonesConfiguration.change(zonesChange ->
+ zonesChange.changeDefaultDistributionZone(zoneChange -> {
+ zoneChange.changeReplicas(2);
+
+ // Also change partitions to be independent from the defaults.
+ zoneChange.changePartitions(1);
Review Comment:
Not sure that I get if. Is initial zone creation? Because partitions count is immutable.
--
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