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/15 15:39:28 UTC
[GitHub] [ignite-3] sergeyuttsel opened a new pull request, #2072: IGNITE-19343 Some tests from DistributionZoneManagerScaleUpTest were fixed and enabled.
sergeyuttsel opened a new pull request, #2072:
URL: https://github.com/apache/ignite-3/pull/2072
https://issues.apache.org/jira/browse/IGNITE-19342
--
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 #2072: IGNITE-19342 Some tests from DistributionZoneManagerScaleUpTest were fixed and enabled.
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #2072:
URL: https://github.com/apache/ignite-3/pull/2072#discussion_r1194626732
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/DistributionZoneManagerWatchListenerTest.java:
##########
@@ -36,74 +36,18 @@
import static org.mockito.Mockito.timeout;
import static org.mockito.Mockito.verify;
-import java.util.Collections;
import java.util.Set;
import java.util.concurrent.CompletableFuture;
import org.apache.ignite.internal.hlc.HybridTimestamp;
import org.apache.ignite.internal.metastorage.dsl.Conditions;
import org.apache.ignite.internal.metastorage.dsl.Operations;
-import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
/**
* Tests distribution zones logical topology changes and reaction to that changes.
*/
//TODO: IGNITE-18564 Add tests with not default distribution zones, when distributionZones.change.trigger per zone will be created.
public class DistributionZoneManagerWatchListenerTest extends BaseDistributionZoneManagerTest {
- @Disabled("https://issues.apache.org/jira/browse/IGNITE-19255")
- @Test
- void testDataNodesOfDefaultZoneUpdatedOnWatchListenerEvent() throws Exception {
Review Comment:
Why it's valid to remove given test?
--
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 #2072: IGNITE-19342 Some tests from DistributionZoneManagerScaleUpTest were fixed and enabled.
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #2072:
URL: https://github.com/apache/ignite-3/pull/2072#discussion_r1194611891
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/DistributionZoneManagerScaleUpTest.java:
##########
@@ -114,7 +114,13 @@ void testDataNodesPropagationAfterScaleUpTriggered() throws Exception {
assertDataNodesForZone(1, clusterNodes2.stream().map(ClusterNode::name).collect(Collectors.toSet()), keyValueStorage);
- assertZoneScaleUpChangeTriggerKey(1, 1, keyValueStorage);
+ topology.putNode(NODE_3);
+
+ Set<LogicalNode> clusterNodes3 = Set.of(NODE_1, NODE_2, NODE_3);
+
+ assertLogicalTopology(clusterNodes3, keyValueStorage);
+
+ assertDataNodesForZone(1, clusterNodes3.stream().map(ClusterNode::name).collect(Collectors.toSet()), keyValueStorage);
Review Comment:
It worth to mention that
- It's better to have `private static final int ZONE_1_ID = 1;` instead of 1.
- In some places, e.g. here it also worth to have same assertion for default zone.
--
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 #2072: IGNITE-19342 Some tests from DistributionZoneManagerScaleUpTest were fixed and enabled.
Posted by "alievmirza (via GitHub)" <gi...@apache.org>.
alievmirza commented on code in PR #2072:
URL: https://github.com/apache/ignite-3/pull/2072#discussion_r1194748622
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/DistributionZoneManagerScaleUpTest.java:
##########
@@ -672,7 +689,7 @@ void testUpdateZoneScaleUpTriggersDataNodePropagation() throws Exception {
assertDataNodesForZone(1, Set.of(), keyValueStorage);
- assertZoneScaleDownChangeTriggerKey(1, 1, keyValueStorage);
+ assertZoneScaleDownChangeTriggerKey(3L, 1, keyValueStorage);
Review Comment:
This check is useless. Let's check that this key become non-null after zone creation or remove check completely
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/DistributionZoneManagerScaleUpTest.java:
##########
@@ -672,7 +689,7 @@ void testUpdateZoneScaleUpTriggersDataNodePropagation() throws Exception {
assertDataNodesForZone(1, Set.of(), keyValueStorage);
- assertZoneScaleDownChangeTriggerKey(1, 1, keyValueStorage);
+ assertZoneScaleDownChangeTriggerKey(3L, 1, keyValueStorage);
Review Comment:
please check also the same logic below
--
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 #2072: IGNITE-19342 Some tests from DistributionZoneManagerScaleUpTest were fixed and enabled.
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #2072:
URL: https://github.com/apache/ignite-3/pull/2072#discussion_r1194624986
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/DistributionZoneManagerScaleUpTest.java:
##########
@@ -260,14 +265,20 @@ void testDropZoneDoNotPropagateDataNodesAfterScaleUp() throws Exception {
assertDataNodesForZone(1, clusterNodesNames2, keyValueStorage);
- assertZoneScaleUpChangeTriggerKey(1, 1, keyValueStorage);
+ assertNotNull(keyValueStorage.get(zoneDataNodesKey(1).bytes()).value(),
Review Comment:
Seems that we already checked exact equality of data nodes. See line above. So that NotNull check check dataNodes is useless.
--
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 #2072: IGNITE-19342 Some tests from DistributionZoneManagerScaleUpTest were fixed and enabled.
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #2072:
URL: https://github.com/apache/ignite-3/pull/2072#discussion_r1194733542
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/DistributionZoneManagerScaleUpTest.java:
##########
@@ -114,7 +114,13 @@ void testDataNodesPropagationAfterScaleUpTriggered() throws Exception {
assertDataNodesForZone(1, clusterNodes2.stream().map(ClusterNode::name).collect(Collectors.toSet()), keyValueStorage);
- assertZoneScaleUpChangeTriggerKey(1, 1, keyValueStorage);
+ topology.putNode(NODE_3);
+
+ Set<LogicalNode> clusterNodes3 = Set.of(NODE_1, NODE_2, NODE_3);
+
+ assertLogicalTopology(clusterNodes3, keyValueStorage);
+
+ assertDataNodesForZone(1, clusterNodes3.stream().map(ClusterNode::name).collect(Collectors.toSet()), keyValueStorage);
Review Comment:
Fixed.
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/DistributionZoneManagerScaleUpTest.java:
##########
@@ -260,14 +265,20 @@ void testDropZoneDoNotPropagateDataNodesAfterScaleUp() throws Exception {
assertDataNodesForZone(1, clusterNodesNames2, keyValueStorage);
- assertZoneScaleUpChangeTriggerKey(1, 1, keyValueStorage);
+ assertNotNull(keyValueStorage.get(zoneDataNodesKey(1).bytes()).value(),
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 pull request #2072: IGNITE-19343 Some tests from DistributionZoneManagerScaleUpTest were fixed and enabled.
Posted by "sergeyuttsel (via GitHub)" <gi...@apache.org>.
sergeyuttsel commented on PR #2072:
URL: https://github.com/apache/ignite-3/pull/2072#issuecomment-1548150741
testDataNodesOfDefaultZoneUpdatedOnWatchListenerEvent was removed because we have another tests for this logic, for example: testDataNodesPropagationAfterScaleUpTriggered and testDataNodesPropagationAfterScaleDownTriggered
--
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 #2072: IGNITE-19342 Some tests from DistributionZoneManagerScaleUpTest were fixed and enabled.
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #2072:
URL: https://github.com/apache/ignite-3/pull/2072#discussion_r1194625719
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/DistributionZoneManagerScaleUpTest.java:
##########
@@ -293,11 +304,18 @@ void testDropZoneDoNotPropagateDataNodesAfterScaleDown() throws Exception {
assertDataNodesForZone(1, clusterNodesNames2, keyValueStorage);
- assertZoneScaleDownChangeTriggerKey(1, 1, keyValueStorage);
+ assertNotNull(keyValueStorage.get(zoneDataNodesKey(1).bytes()).value(),
Review Comment:
Same as above.
--
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 #2072: IGNITE-19342 Some tests from DistributionZoneManagerScaleUpTest were fixed and enabled.
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #2072:
URL: https://github.com/apache/ignite-3/pull/2072#discussion_r1194734428
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/DistributionZoneManagerWatchListenerTest.java:
##########
@@ -36,74 +36,18 @@
import static org.mockito.Mockito.timeout;
import static org.mockito.Mockito.verify;
-import java.util.Collections;
import java.util.Set;
import java.util.concurrent.CompletableFuture;
import org.apache.ignite.internal.hlc.HybridTimestamp;
import org.apache.ignite.internal.metastorage.dsl.Conditions;
import org.apache.ignite.internal.metastorage.dsl.Operations;
-import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
/**
* Tests distribution zones logical topology changes and reaction to that changes.
*/
//TODO: IGNITE-18564 Add tests with not default distribution zones, when distributionZones.change.trigger per zone will be created.
public class DistributionZoneManagerWatchListenerTest extends BaseDistributionZoneManagerTest {
- @Disabled("https://issues.apache.org/jira/browse/IGNITE-19255")
- @Test
- void testDataNodesOfDefaultZoneUpdatedOnWatchListenerEvent() throws Exception {
Review Comment:
As Sergey told
"testDataNodesOfDefaultZoneUpdatedOnWatchListenerEvent was removed because we have another tests for this logic, for example: testDataNodesPropagationAfterScaleUpTriggered and testDataNodesPropagationAfterScaleDownTriggered. This test checked that data nodes value was changed after topology updating. Now we have many other tests for this logic."
--
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 #2072: IGNITE-19342 Some tests from DistributionZoneManagerScaleUpTest were fixed and enabled.
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #2072:
URL: https://github.com/apache/ignite-3/pull/2072#discussion_r1194611891
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/DistributionZoneManagerScaleUpTest.java:
##########
@@ -114,7 +114,13 @@ void testDataNodesPropagationAfterScaleUpTriggered() throws Exception {
assertDataNodesForZone(1, clusterNodes2.stream().map(ClusterNode::name).collect(Collectors.toSet()), keyValueStorage);
- assertZoneScaleUpChangeTriggerKey(1, 1, keyValueStorage);
+ topology.putNode(NODE_3);
+
+ Set<LogicalNode> clusterNodes3 = Set.of(NODE_1, NODE_2, NODE_3);
+
+ assertLogicalTopology(clusterNodes3, keyValueStorage);
+
+ assertDataNodesForZone(1, clusterNodes3.stream().map(ClusterNode::name).collect(Collectors.toSet()), keyValueStorage);
Review Comment:
It worth to mention that
- It's better to have `private static final int ZONE_1_ID = 1;` instead of 1.
- Almost everywhere were we assert assertDataNodesForZone for zone1 we may also assert data nodes for the default one, I'd rather add following method to `DistributionZonesTestUtil`
```
public static void assertDataNodesForZones(
@Nullable Set<String> clusterNodes,
KeyValueStorage keyValueStorage,
int... zoneIds
) throws InterruptedException {
for (int zoneId : zoneIds) {
assertDataNodesForZone(zoneId, clusterNodes, keyValueStorage);
}
}
```
--
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 #2072: IGNITE-19342 Some tests from DistributionZoneManagerScaleUpTest were fixed and enabled.
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc merged PR #2072:
URL: https://github.com/apache/ignite-3/pull/2072
--
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 #2072: IGNITE-19342 Some tests from DistributionZoneManagerScaleUpTest were fixed and enabled.
Posted by "sergeyuttsel (via GitHub)" <gi...@apache.org>.
sergeyuttsel commented on code in PR #2072:
URL: https://github.com/apache/ignite-3/pull/2072#discussion_r1194831250
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/DistributionZoneManagerScaleUpTest.java:
##########
@@ -672,7 +689,7 @@ void testUpdateZoneScaleUpTriggersDataNodePropagation() throws Exception {
assertDataNodesForZone(1, Set.of(), keyValueStorage);
- assertZoneScaleDownChangeTriggerKey(1, 1, keyValueStorage);
+ assertZoneScaleDownChangeTriggerKey(3L, 1, keyValueStorage);
Review Comment:
Fixed. I decided to remove this check.
--
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 #2072: IGNITE-19342 Some tests from DistributionZoneManagerScaleUpTest were fixed and enabled.
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #2072:
URL: https://github.com/apache/ignite-3/pull/2072#discussion_r1194733988
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/DistributionZoneManagerScaleUpTest.java:
##########
@@ -293,11 +304,18 @@ void testDropZoneDoNotPropagateDataNodesAfterScaleDown() throws Exception {
assertDataNodesForZone(1, clusterNodesNames2, keyValueStorage);
- assertZoneScaleDownChangeTriggerKey(1, 1, keyValueStorage);
+ assertNotNull(keyValueStorage.get(zoneDataNodesKey(1).bytes()).value(),
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