You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "sergeyuttsel (via GitHub)" <gi...@apache.org> on 2023/05/11 09:16:58 UTC
[GitHub] [ignite-3] sergeyuttsel opened a new pull request, #2059: IGNITE-19343 Some of DistributionZoneAwaitDataNodesTest are enabled.
sergeyuttsel opened a new pull request, #2059:
URL: https://github.com/apache/ignite-3/pull/2059
* DistributedConfigurationStorage is used in BaseDistributionZoneManagerTest to fix an issue when metaStorageManager and zonesConfiguration use different storage and have different storage revisions.
--
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
[GitHub] [ignite-3] sergeyuttsel 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.
Posted by "sergeyuttsel (via GitHub)" <gi...@apache.org>.
sergeyuttsel commented on code in PR #2059:
URL: https://github.com/apache/ignite-3/pull/2059#discussion_r1191338939
##########
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:
Yes, these are redundant steps.
--
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
[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.
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
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
[GitHub] [ignite-3] sergeyuttsel 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.
Posted by "sergeyuttsel (via GitHub)" <gi...@apache.org>.
sergeyuttsel commented on code in PR #2059:
URL: https://github.com/apache/ignite-3/pull/2059#discussion_r1191267921
##########
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:
Fixed
--
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
[GitHub] [ignite-3] sergeyuttsel 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.
Posted by "sergeyuttsel (via GitHub)" <gi...@apache.org>.
sergeyuttsel commented on code in PR #2059:
URL: https://github.com/apache/ignite-3/pull/2059#discussion_r1191224783
##########
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:
DistributedConfigurationStorage
--
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
[GitHub] [ignite-3] sergeyuttsel 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.
Posted by "sergeyuttsel (via GitHub)" <gi...@apache.org>.
sergeyuttsel commented on code in PR #2059:
URL: https://github.com/apache/ignite-3/pull/2059#discussion_r1193413712
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/DistributionZoneManagerScaleUpTest.java:
##########
@@ -1351,8 +1353,8 @@ private void preparePrerequisites() throws Exception {
assertDataNodesForZone(1, clusterNodesNames, keyValueStorage);
- assertZoneScaleUpChangeTriggerKey(1, 1, keyValueStorage);
- assertZoneScaleDownChangeTriggerKey(1, 1, keyValueStorage);
+ assertZoneScaleUpChangeTriggerKey(PREREQUISITE_REVISION, 1, keyValueStorage);
Review Comment:
Fixed
--
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
[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.
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #2059:
URL: https://github.com/apache/ignite-3/pull/2059#discussion_r1191127924
##########
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. Meaning 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
[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.
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #2059:
URL: https://github.com/apache/ignite-3/pull/2059#discussion_r1191161452
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/DistributionZoneAwaitDataNodesTest.java:
##########
@@ -367,20 +377,30 @@ void testRemoveZoneWhileAwaitingDataNodes() throws Exception {
CompletableFuture<Set<String>> dataNodesFut1 = distributionZoneManager.topologyVersionedDataNodes(zoneId, 106);
+ doAnswer(invocation -> {
Review Comment:
What are we trying to achieve with this mock? Why it's not enough to simply remove zone, meaning just call
`distributionZoneManager.dropZone(zoneName).get();`
By the way if I do this, test might fail because instead of expected `DistributionZoneWasRemovedException` `DistributionZoneNotFoundException` will be thrown from `org.apache.ignite.internal.distributionzones.DistributionZonesUtil#getZoneById`. We should also fix that.
--
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
[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.
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #2059:
URL: https://github.com/apache/ignite-3/pull/2059#discussion_r1191166974
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/DistributionZoneAwaitDataNodesTest.java:
##########
@@ -367,20 +377,30 @@ void testRemoveZoneWhileAwaitingDataNodes() throws Exception {
CompletableFuture<Set<String>> dataNodesFut1 = distributionZoneManager.topologyVersionedDataNodes(zoneId, 106);
+ doAnswer(invocation -> {
+ If iif = invocation.getArgument(0);
+
+ byte[] key = zoneScaleUpChangeTriggerKey(zoneId).bytes();
+
+ if (Arrays.stream(iif.cond().keys()).anyMatch(k -> Arrays.equals(key, k))) {
+ //Drop the zone while dataNodesFut1 is awaiting a data nodes update.
+ distributionZoneManager.dropZone(zoneName).get();
Review Comment:
It's better to bound future awaiting time either with `.get(3, SECONDS);` or with
```
// Drop zone.
assertThat(distributionZoneManager.dropZone(zoneName), willSucceedIn(3, SECONDS));
```
From my point of view second approach is generally better.
--
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
[GitHub] [ignite-3] sergeyuttsel 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.
Posted by "sergeyuttsel (via GitHub)" <gi...@apache.org>.
sergeyuttsel commented on code in PR #2059:
URL: https://github.com/apache/ignite-3/pull/2059#discussion_r1192048005
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/DistributionZoneAwaitDataNodesTest.java:
##########
@@ -367,20 +377,30 @@ void testRemoveZoneWhileAwaitingDataNodes() throws Exception {
CompletableFuture<Set<String>> dataNodesFut1 = distributionZoneManager.topologyVersionedDataNodes(zoneId, 106);
+ doAnswer(invocation -> {
Review Comment:
It's a different test cases, so I added another test. testRemoveZoneWhileAwaitingTopologyVersion checks that DistributionZoneNotFoundException will be thrown if the zone is removed on topology version awaiting.
testScaleUpScaleDownAreChangedWhileAwaitingDataNodes checks that DistributionZoneWasRemovedException will be thrown if the zone is removed after topology version awaiting.
--
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
[GitHub] [ignite-3] sergeyuttsel 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.
Posted by "sergeyuttsel (via GitHub)" <gi...@apache.org>.
sergeyuttsel commented on code in PR #2059:
URL: https://github.com/apache/ignite-3/pull/2059#discussion_r1191227594
##########
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:
Thanks. I've added.
--
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
[GitHub] [ignite-3] sergeyuttsel 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.
Posted by "sergeyuttsel (via GitHub)" <gi...@apache.org>.
sergeyuttsel commented on code in PR #2059:
URL: https://github.com/apache/ignite-3/pull/2059#discussion_r1191260576
##########
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:
Fixed
--
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
[GitHub] [ignite-3] sergeyuttsel 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.
Posted by "sergeyuttsel (via GitHub)" <gi...@apache.org>.
sergeyuttsel commented on code in PR #2059:
URL: https://github.com/apache/ignite-3/pull/2059#discussion_r1191227594
##########
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:
I see that ConfigurationManager.start() invokes ConfigurationRegistry.start(). Also IgniteImpl used ConfigurationManager.start() only. So I added ConfigurationManager.start() and removed ConfigurationRegistry.start().
--
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
[GitHub] [ignite-3] alievmirza 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.
Posted by "alievmirza (via GitHub)" <gi...@apache.org>.
alievmirza commented on code in PR #2059:
URL: https://github.com/apache/ignite-3/pull/2059#discussion_r1191249715
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/DistributionZoneManagerScaleUpTest.java:
##########
@@ -1351,8 +1353,8 @@ private void preparePrerequisites() throws Exception {
assertDataNodesForZone(1, clusterNodesNames, keyValueStorage);
- assertZoneScaleUpChangeTriggerKey(1, 1, keyValueStorage);
- assertZoneScaleDownChangeTriggerKey(1, 1, keyValueStorage);
+ assertZoneScaleUpChangeTriggerKey(PREREQUISITE_REVISION, 1, keyValueStorage);
Review Comment:
let's check here that `assertZoneScaleUp/DownChangeTriggerKey` are not null and equal to each other, and after that
Lets initialise here `PREREQUISITE_REVISION` with the value from trigger key
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/DistributionZoneManagerScaleUpTest.java:
##########
@@ -721,29 +723,29 @@ void testCleanUpAfterSchedulers() throws Exception {
ZoneState zoneState = distributionZoneManager.zonesTimers().get(1);
- zoneState.nodesToAddToDataNodes(Set.of("D"), 3);
- zoneState.nodesToAddToDataNodes(Set.of("E"), 4);
- zoneState.nodesToRemoveFromDataNodes(Set.of("C"), 7);
+ zoneState.nodesToAddToDataNodes(Set.of("D"), PREREQUISITE_REVISION + 2);
Review Comment:
please, for this and all test like testVariousScaleUpScaleDownScenarios*
do not use PREREQUISITE_REVISION + x construction, but use
1000 + n as a revision, where n is a value before your changes
--
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
[GitHub] [ignite-3] sergeyuttsel 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.
Posted by "sergeyuttsel (via GitHub)" <gi...@apache.org>.
sergeyuttsel commented on code in PR #2059:
URL: https://github.com/apache/ignite-3/pull/2059#discussion_r1193414222
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/DistributionZoneManagerScaleUpTest.java:
##########
@@ -721,29 +723,29 @@ void testCleanUpAfterSchedulers() throws Exception {
ZoneState zoneState = distributionZoneManager.zonesTimers().get(1);
- zoneState.nodesToAddToDataNodes(Set.of("D"), 3);
- zoneState.nodesToAddToDataNodes(Set.of("E"), 4);
- zoneState.nodesToRemoveFromDataNodes(Set.of("C"), 7);
+ zoneState.nodesToAddToDataNodes(Set.of("D"), PREREQUISITE_REVISION + 2);
Review Comment:
Fixed
--
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
[GitHub] [ignite-3] sergeyuttsel 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.
Posted by "sergeyuttsel (via GitHub)" <gi...@apache.org>.
sergeyuttsel commented on code in PR #2059:
URL: https://github.com/apache/ignite-3/pull/2059#discussion_r1191229262
##########
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:
Fixed
--
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
[GitHub] [ignite-3] sergeyuttsel 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.
Posted by "sergeyuttsel (via GitHub)" <gi...@apache.org>.
sergeyuttsel commented on code in PR #2059:
URL: https://github.com/apache/ignite-3/pull/2059#discussion_r1191269255
##########
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:
DistributedConfigurationStorage
--
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
[GitHub] [ignite-3] sergeyuttsel 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.
Posted by "sergeyuttsel (via GitHub)" <gi...@apache.org>.
sergeyuttsel commented on code in PR #2059:
URL: https://github.com/apache/ignite-3/pull/2059#discussion_r1191275487
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/DistributionZoneAwaitDataNodesTest.java:
##########
@@ -367,20 +377,30 @@ void testRemoveZoneWhileAwaitingDataNodes() throws Exception {
CompletableFuture<Set<String>> dataNodesFut1 = distributionZoneManager.topologyVersionedDataNodes(zoneId, 106);
+ doAnswer(invocation -> {
+ If iif = invocation.getArgument(0);
+
+ byte[] key = zoneScaleUpChangeTriggerKey(zoneId).bytes();
+
+ if (Arrays.stream(iif.cond().keys()).anyMatch(k -> Arrays.equals(key, k))) {
+ //Drop the zone while dataNodesFut1 is awaiting a data nodes update.
+ distributionZoneManager.dropZone(zoneName).get();
Review Comment:
Fixed
--
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
[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.
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #2059:
URL: https://github.com/apache/ignite-3/pull/2059#discussion_r1193454232
##########
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:
Yep, that was an expected one.
--
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
[GitHub] [ignite-3] sanpwc merged 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.
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc merged PR #2059:
URL: https://github.com/apache/ignite-3/pull/2059
--
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