sanpwc commented on code in PR #4764:
URL: https://github.com/apache/ignite-3/pull/4764#discussion_r1853834172
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/DistributionZoneManager.java:
##########
@@ -985,22 +997,23 @@ private CompletableFuture<Void>
scheduleTimers(CatalogZoneDescriptor zone, boole
if (nodesRemoved) {
if (zone.consistencyMode() == HIGH_AVAILABILITY) {
- if (partitionReset == IMMEDIATE_TIMER_VALUE) {
- futures.add(
- // TODO: IGNITE-23599 Implement valid
behaviour here.
- nullCompletedFuture()
- );
- }
-
if (partitionReset != INFINITE_TIMER_VALUE) {
zonesState.get(zoneId).reschedulePartitionDistributionReset(
partitionReset,
- // TODO: IGNITE-23599 Implement valid
behaviour here.
- () -> {},
- zoneId
+ () -> {
+ fireEvent(
Review Comment:
I'd rather hide it intro private method.
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/DistributionZoneManager.java:
##########
@@ -409,7 +413,15 @@ private void onUpdatePartitionDistributionResetBusy(
}
if (partitionDistributionResetTimeoutSeconds ==
IMMEDIATE_TIMER_VALUE) {
- // TODO: IGNITE-23599 Implement valid behaviour here.
+ fireEvent(
Review Comment:
I thought that we've agreed that we won't call event in MS Watch thread,
aren't we?
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/events/HaZoneTopologyUpdateEventParams.java:
##########
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.distributionzones.events;
+
+import org.apache.ignite.internal.event.EventParameters;
+
+/**
+ * Parameters of {@link HaZoneTopologyUpdateEvent}.
+ */
+// TODO: https://issues.apache.org/jira/browse/IGNITE-23640 Remove this params
together with the event.
+@Deprecated
+public class HaZoneTopologyUpdateEventParams implements EventParameters {
+ /** Zone id. */
+ private final int zoneId;
+
+ /**
+ * Long view of{@link org.apache.ignite.internal.hlc.HybridTimestamp}.
Review Comment:
What a mix of
`/** ... */`
and
```
/**
*
*/
```
##########
modules/table/src/integrationTest/java/org/apache/ignite/internal/table/distributed/disaster/ItHighAvailablePartitionsRecoveryTest.java:
##########
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.table.distributed.disaster;
+
+import static
org.apache.ignite.internal.catalog.CatalogService.DEFAULT_STORAGE_PROFILE;
+import static
org.apache.ignite.internal.table.distributed.disaster.DisasterRecoveryManager.RECOVERY_TRIGGER_KEY;
+import static
org.apache.ignite.internal.testframework.IgniteTestUtils.waitForCondition;
+import static
org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.Set;
+import java.util.concurrent.CompletableFuture;
+import java.util.function.Supplier;
+import org.apache.ignite.internal.ClusterPerTestIntegrationTest;
+import org.apache.ignite.internal.app.IgniteImpl;
+import org.apache.ignite.internal.hlc.HybridClock;
+import org.apache.ignite.internal.hlc.HybridClockImpl;
+import org.apache.ignite.internal.metastorage.Entry;
+import org.apache.ignite.internal.versioned.VersionedSerialization;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+/** Test for the HA zones recovery. */
+public class ItHighAvailablePartitionsRecoveryTest extends
ClusterPerTestIntegrationTest {
+ private static String ZONE_NAME = "HA_ZONE";
+
+ private static String TABLE_NAME = "TEST_TABLE";
+
+ protected final HybridClock clock = new HybridClockImpl();
+
+ @Override
+ protected int initialNodes() {
+ return 2;
+ }
+
+ @BeforeEach
+ void setUp() {
+ executeSql(String.format(
+ "CREATE ZONE %s WITH REPLICAS=%s, PARTITIONS=%s,
STORAGE_PROFILES='%s', CONSISTENCY_MODE='HIGH_AVAILABILITY'",
+ ZONE_NAME, initialNodes(), 2, DEFAULT_STORAGE_PROFILE
+ ));
+
+ executeSql(String.format(
+ "CREATE TABLE %s (id INT PRIMARY KEY, val INT) ZONE %s",
+ TABLE_NAME, ZONE_NAME
+ ));
+ }
+
+ @Override
+ protected String getNodeBootstrapConfigTemplate() {
+ return FAST_FAILURE_DETECTION_NODE_BOOTSTRAP_CFG_TEMPLATE;
+ }
+
+ @Test
+ void testTopologyReduceEventPropagation() throws InterruptedException {
+ IgniteImpl node = igniteImpl(0);
+
+ Supplier<Entry> getRecoveryTriggerKey = () -> {
+ CompletableFuture<Entry> getFut =
node.metaStorageManager().get(RECOVERY_TRIGGER_KEY);
+
+ assertThat(getFut, willCompleteSuccessfully());
+
+ return getFut.join();
+ };
+
+ assertTrue(waitForCondition(() -> getRecoveryTriggerKey.get().empty(),
5_000));
+
+ stopNode(1);
+
+ assertFalse(waitForCondition(() ->
getRecoveryTriggerKey.get().empty(), 5_000));
+
+ GroupUpdateRequest request = (GroupUpdateRequest)
VersionedSerialization.fromBytes(
+ getRecoveryTriggerKey.get().value(),
DisasterRecoveryRequestSerializer.INSTANCE);
+
+ int zoneId = node.catalogManager().zone(ZONE_NAME,
clock.nowLong()).id();
+ int tableId = node.catalogManager().table(TABLE_NAME,
clock.nowLong()).id();
+
+ assertEquals(zoneId, request.zoneId());
+ assertEquals(tableId, request.tableId());
+ assertEquals(Set.of(0, 1), request.partitionIds());
+ assertFalse(request.manualUpdate());
+ }
+}
Review Comment:
Test for timers restoration is missing. I mean following situation:
Three Nodes [A, B, C].
[A] is a CMG/MG group.
Partition P1 = [A, B, C]
resetTimeout = 2.
1. At t1 C dies; reset timer is scheduled.
2. At t2 B dies; reset timer is rescheduler.
3. At t3 A dies; All nodes are dead.
4. [A] restores
We may expect that
org.apache.ignite.internal.distributionzones.DistributionZoneManager#restoreTimers
will do the trick and reschedule the resetTimer on node restart, but let's
check it. As usual I mean full fake integration test here.
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/events/HaZoneTopologyUpdateEventParams.java:
##########
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.distributionzones.events;
+
+import org.apache.ignite.internal.event.EventParameters;
+
+/**
+ * Parameters of {@link HaZoneTopologyUpdateEvent}.
+ */
+// TODO: https://issues.apache.org/jira/browse/IGNITE-23640 Remove this params
together with the event.
+@Deprecated
+public class HaZoneTopologyUpdateEventParams implements EventParameters {
+ /** Zone id. */
+ private final int zoneId;
+
+ /**
+ * Long view of{@link org.apache.ignite.internal.hlc.HybridTimestamp}.
+ */
+ private final long timestamp;
+
+ /**
+ * Constructor.
+ *
+ * @param causalityToken Causality token.
+ * @param zoneId Zone id.
+ * @param timestamp Long view of{@link
org.apache.ignite.internal.hlc.HybridTimestamp}.
+ */
+ public HaZoneTopologyUpdateEventParams(long causalityToken, int zoneId,
long timestamp) {
Review Comment:
So, you simply ignore causalityToken param.
##########
modules/table/src/integrationTest/java/org/apache/ignite/internal/table/distributed/disaster/ItHighAvailablePartitionsRecoveryTest.java:
##########
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.table.distributed.disaster;
+
+import static
org.apache.ignite.internal.catalog.CatalogService.DEFAULT_STORAGE_PROFILE;
+import static
org.apache.ignite.internal.table.distributed.disaster.DisasterRecoveryManager.RECOVERY_TRIGGER_KEY;
+import static
org.apache.ignite.internal.testframework.IgniteTestUtils.waitForCondition;
+import static
org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.Set;
+import java.util.concurrent.CompletableFuture;
+import java.util.function.Supplier;
+import org.apache.ignite.internal.ClusterPerTestIntegrationTest;
+import org.apache.ignite.internal.app.IgniteImpl;
+import org.apache.ignite.internal.hlc.HybridClock;
+import org.apache.ignite.internal.hlc.HybridClockImpl;
+import org.apache.ignite.internal.metastorage.Entry;
+import org.apache.ignite.internal.versioned.VersionedSerialization;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+/** Test for the HA zones recovery. */
+public class ItHighAvailablePartitionsRecoveryTest extends
ClusterPerTestIntegrationTest {
+ private static String ZONE_NAME = "HA_ZONE";
+
+ private static String TABLE_NAME = "TEST_TABLE";
+
+ protected final HybridClock clock = new HybridClockImpl();
+
+ @Override
+ protected int initialNodes() {
+ return 2;
+ }
+
+ @BeforeEach
+ void setUp() {
+ executeSql(String.format(
+ "CREATE ZONE %s WITH REPLICAS=%s, PARTITIONS=%s,
STORAGE_PROFILES='%s', CONSISTENCY_MODE='HIGH_AVAILABILITY'",
+ ZONE_NAME, initialNodes(), 2, DEFAULT_STORAGE_PROFILE
+ ));
+
+ executeSql(String.format(
+ "CREATE TABLE %s (id INT PRIMARY KEY, val INT) ZONE %s",
+ TABLE_NAME, ZONE_NAME
+ ));
+ }
+
+ @Override
+ protected String getNodeBootstrapConfigTemplate() {
+ return FAST_FAILURE_DETECTION_NODE_BOOTSTRAP_CFG_TEMPLATE;
+ }
+
+ @Test
+ void testTopologyReduceEventPropagation() throws InterruptedException {
+ IgniteImpl node = igniteImpl(0);
+
+ Supplier<Entry> getRecoveryTriggerKey = () -> {
+ CompletableFuture<Entry> getFut =
node.metaStorageManager().get(RECOVERY_TRIGGER_KEY);
+
+ assertThat(getFut, willCompleteSuccessfully());
+
+ return getFut.join();
+ };
+
+ assertTrue(waitForCondition(() -> getRecoveryTriggerKey.get().empty(),
5_000));
+
+ stopNode(1);
+
+ assertFalse(waitForCondition(() ->
getRecoveryTriggerKey.get().empty(), 5_000));
+
+ GroupUpdateRequest request = (GroupUpdateRequest)
VersionedSerialization.fromBytes(
+ getRecoveryTriggerKey.get().value(),
DisasterRecoveryRequestSerializer.INSTANCE);
+
+ int zoneId = node.catalogManager().zone(ZONE_NAME,
clock.nowLong()).id();
+ int tableId = node.catalogManager().table(TABLE_NAME,
clock.nowLong()).id();
+
+ assertEquals(zoneId, request.zoneId());
+ assertEquals(tableId, request.tableId());
+ assertEquals(Set.of(0, 1), request.partitionIds());
+ assertFalse(request.manualUpdate());
+ }
+}
Review Comment:
Firing event on resetTimeout update is not tested.
##########
modules/table/src/integrationTest/java/org/apache/ignite/internal/table/distributed/disaster/ItHighAvailablePartitionsRecoveryTest.java:
##########
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.table.distributed.disaster;
+
+import static
org.apache.ignite.internal.catalog.CatalogService.DEFAULT_STORAGE_PROFILE;
+import static
org.apache.ignite.internal.table.distributed.disaster.DisasterRecoveryManager.RECOVERY_TRIGGER_KEY;
+import static
org.apache.ignite.internal.testframework.IgniteTestUtils.waitForCondition;
+import static
org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.Set;
+import java.util.concurrent.CompletableFuture;
+import java.util.function.Supplier;
+import org.apache.ignite.internal.ClusterPerTestIntegrationTest;
+import org.apache.ignite.internal.app.IgniteImpl;
+import org.apache.ignite.internal.hlc.HybridClock;
+import org.apache.ignite.internal.hlc.HybridClockImpl;
+import org.apache.ignite.internal.metastorage.Entry;
+import org.apache.ignite.internal.versioned.VersionedSerialization;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+/** Test for the HA zones recovery. */
+public class ItHighAvailablePartitionsRecoveryTest extends
ClusterPerTestIntegrationTest {
+ private static String ZONE_NAME = "HA_ZONE";
+
+ private static String TABLE_NAME = "TEST_TABLE";
+
+ protected final HybridClock clock = new HybridClockImpl();
+
+ @Override
+ protected int initialNodes() {
+ return 2;
+ }
+
+ @BeforeEach
+ void setUp() {
+ executeSql(String.format(
+ "CREATE ZONE %s WITH REPLICAS=%s, PARTITIONS=%s,
STORAGE_PROFILES='%s', CONSISTENCY_MODE='HIGH_AVAILABILITY'",
+ ZONE_NAME, initialNodes(), 2, DEFAULT_STORAGE_PROFILE
+ ));
+
+ executeSql(String.format(
+ "CREATE TABLE %s (id INT PRIMARY KEY, val INT) ZONE %s",
+ TABLE_NAME, ZONE_NAME
+ ));
+ }
+
+ @Override
+ protected String getNodeBootstrapConfigTemplate() {
+ return FAST_FAILURE_DETECTION_NODE_BOOTSTRAP_CFG_TEMPLATE;
+ }
+
+ @Test
+ void testTopologyReduceEventPropagation() throws InterruptedException {
+ IgniteImpl node = igniteImpl(0);
+
+ Supplier<Entry> getRecoveryTriggerKey = () -> {
+ CompletableFuture<Entry> getFut =
node.metaStorageManager().get(RECOVERY_TRIGGER_KEY);
+
+ assertThat(getFut, willCompleteSuccessfully());
+
+ return getFut.join();
+ };
+
+ assertTrue(waitForCondition(() -> getRecoveryTriggerKey.get().empty(),
5_000));
+
+ stopNode(1);
+
+ assertFalse(waitForCondition(() ->
getRecoveryTriggerKey.get().empty(), 5_000));
+
+ GroupUpdateRequest request = (GroupUpdateRequest)
VersionedSerialization.fromBytes(
+ getRecoveryTriggerKey.get().value(),
DisasterRecoveryRequestSerializer.INSTANCE);
+
+ int zoneId = node.catalogManager().zone(ZONE_NAME,
clock.nowLong()).id();
+ int tableId = node.catalogManager().table(TABLE_NAME,
clock.nowLong()).id();
+
+ assertEquals(zoneId, request.zoneId());
+ assertEquals(tableId, request.tableId());
+ assertEquals(Set.of(0, 1), request.partitionIds());
+ assertFalse(request.manualUpdate());
+ }
+}
Review Comment:
Rescheduling logic is not tested.
I mean
Three Nodes [A, B, C].
[A] is a CMG/MG group.
Partition P1 = [A, B, C]
resetTimeout = 2.
1. At t1 C dies; reset timer is scheduled and not fired because
2. At t2 B dies; reset timer is rescheduler and fired.
--
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-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
