sanpwc commented on code in PR #2059:
URL: https://github.com/apache/ignite-3/pull/2059#discussion_r1191071110
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/BaseDistributionZoneManagerTest.java:
##########
@@ -70,6 +75,10 @@ public class BaseDistributionZoneManagerTest extends
BaseIgniteAbstractTest {
protected VaultManager vaultMgr;
+ private ConfigurationStorage cfgStorage;
Review Comment:
Idea claims that cfgStorage "Field can be converted to a local variable ".
Same for cfgMgr.
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/BaseDistributionZoneManagerTest.java:
##########
@@ -84,6 +93,20 @@ void setUp() {
components.add(metaStorageManager);
+ cfgStorage = new DistributedConfigurationStorage(metaStorageManager,
vaultMgr);
+
+ cfgMgr = new ConfigurationManager(
+ List.of(DistributionZonesConfiguration.KEY),
+ Set.of(),
+ cfgStorage,
+ List.of(),
+ List.of(TestPersistStorageConfigurationSchema.class)
+ );
+
+ ConfigurationRegistry registry = cfgMgr.configurationRegistry();
+
+ components.add(registry);
Review Comment:
Why we add register and not cfgMgr to the components list?
##########
modules/distribution-zones/build.gradle:
##########
@@ -51,6 +51,7 @@ dependencies {
testImplementation project(':ignite-network-api')
testImplementation project(':ignite-metastorage-api')
testImplementation project(':ignite-metastorage')
+ testImplementation project(':ignite-runner')
Review Comment:
What classed do we use from the runner module?
##########
modules/distribution-zones/build.gradle:
##########
@@ -51,6 +51,7 @@ dependencies {
testImplementation project(':ignite-network-api')
testImplementation project(':ignite-metastorage-api')
testImplementation project(':ignite-metastorage')
+ testImplementation project(':ignite-runner')
Review Comment:
Which classed do we use from runner?
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/DistributionZoneAwaitDataNodesTest.java:
##########
@@ -342,20 +351,21 @@ void testWithOutAwaiting() throws Exception {
/**
* Test checks that data nodes futures are completed exceptionally if the
zone was removed while data nodes awaiting.
*/
- @Disabled("https://issues.apache.org/jira/browse/IGNITE-19255";)
@Test
void testRemoveZoneWhileAwaitingDataNodes() throws Exception {
startZoneManager();
+ String zoneName = "zone0";
Review Comment:
Seems that we use such name and also zone1 and zone2 in multiple test within
given class. Let's extract them to constants.
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/DistributionZoneAwaitDataNodesTest.java:
##########
@@ -342,20 +351,21 @@ void testWithOutAwaiting() throws Exception {
/**
* Test checks that data nodes futures are completed exceptionally if the
zone was removed while data nodes awaiting.
*/
- @Disabled("https://issues.apache.org/jira/browse/IGNITE-19255";)
@Test
void testRemoveZoneWhileAwaitingDataNodes() throws Exception {
startZoneManager();
+ String zoneName = "zone0";
+
distributionZoneManager.createZone(
- new
DistributionZoneConfigurationParameters.Builder("zone0")
+ new
DistributionZoneConfigurationParameters.Builder(zoneName)
.dataNodesAutoAdjustScaleUp(IMMEDIATE_TIMER_VALUE)
.dataNodesAutoAdjustScaleDown(IMMEDIATE_TIMER_VALUE)
.build()
)
.get(3, SECONDS);
- int zoneId = distributionZoneManager.getZoneId("zone0");
+ int zoneId = distributionZoneManager.getZoneId(zoneName);
CompletableFuture<Set<String>> dataNodesFut0 =
distributionZoneManager.topologyVersionedDataNodes(zoneId, 5);
Review Comment:
Why do we need all that?
```
CompletableFuture<Set<String>> dataNodesFut0 =
distributionZoneManager.topologyVersionedDataNodes(zoneId, 5);
setLogicalTopologyInMetaStorage(Set.of("node0", "node1"), 100);
assertFalse(dataNodesFut0.isDone());
assertEquals(Set.of("node0", "node1"), dataNodesFut0.get(3,
SECONDS));
```
If I'll comment all that lines the test will still pass.
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/DistributionZoneAwaitDataNodesTest.java:
##########
@@ -342,20 +351,21 @@ void testWithOutAwaiting() throws Exception {
/**
* Test checks that data nodes futures are completed exceptionally if the
zone was removed while data nodes awaiting.
*/
- @Disabled("https://issues.apache.org/jira/browse/IGNITE-19255";)
@Test
void testRemoveZoneWhileAwaitingDataNodes() throws Exception {
startZoneManager();
+ String zoneName = "zone0";
+
distributionZoneManager.createZone(
- new
DistributionZoneConfigurationParameters.Builder("zone0")
+ new
DistributionZoneConfigurationParameters.Builder(zoneName)
.dataNodesAutoAdjustScaleUp(IMMEDIATE_TIMER_VALUE)
.dataNodesAutoAdjustScaleDown(IMMEDIATE_TIMER_VALUE)
.build()
)
.get(3, SECONDS);
- int zoneId = distributionZoneManager.getZoneId("zone0");
+ int zoneId = distributionZoneManager.getZoneId(zoneName);
Review Comment:
Seems that we don't need explicit zoneId retrieval, because
distributionZoneManager.createZone() will return future with zoneId. Meanaing
that it's enough to write following.
```
Integer zoneId = distributionZoneManager.createZone(
new
DistributionZoneConfigurationParameters.Builder("zone0")
new
DistributionZoneConfigurationParameters.Builder(zoneName)
.dataNodesAutoAdjustScaleUp(IMMEDIATE_TIMER_VALUE)
.dataNodesAutoAdjustScaleDown(IMMEDIATE_TIMER_VALUE)
.build()
)
.get(3, SECONDS);
```
And btw, let's update createZone javadoc in a way that it will explicitly
denote that CompletableFuture<Integer> is a future with zoneId.
--
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]
