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/05/11 13:02:04 UTC
[GitHub] [ignite-3] sanpwc commented on a diff in pull request #2059: IGNITE-19343 Some of DistributionZoneAwaitDataNodesTest are enabled. DistributedConfigurationStorage is used in BaseDistributionZoneManagerTest to fix an issue when metaStorageManager and zonesConfiguration use different storage and have different storage revisions.
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org