You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "JAkutenshi (via GitHub)" <gi...@apache.org> on 2025/02/26 13:24:08 UTC
[PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
JAkutenshi opened a new pull request, #5300:
URL: https://github.com/apache/ignite-3/pull/5300
JIRA Ticket: [IGNITE-24640](https://issues.apache.org/jira/browse/IGNITE-24640)
## The goal
## The reason
## The solution
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1978919821
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -2814,7 +2835,11 @@ private CompletableFuture<Void> stopPartition(TablePartitionId tablePartitionId,
CompletableFuture<Boolean> stopReplicaFuture;
try {
- stopReplicaFuture = replicaMgr.stopReplica(tablePartitionId);
+ // In case of colocation there shouldn't be any table replica and thus it shouldn't be stopped. Moreover the excessive replica
Review Comment:
I saw you made some related changes, but this part is still here, why?
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1976683513
##########
modules/partition-replicator/src/integrationTest/java/org/apache/ignite/internal/partition/replicator/ItReplicaLifecycleTest.java:
##########
@@ -636,6 +637,32 @@ public void testScanCloseReplicaRequest() throws Exception {
assertDoesNotThrow(tx::commit);
}
+ @Test
+ public void testNodeStop() throws Exception {
+ // Prepare a single node cluster.
+ startCluster(1);
+ Node node = getNode(0);
+
+ // Prepare a zone.
+ String zoneName = "test_zone";
+ createZone(node, zoneName, 1, 1);
+
+ // Create a table to work with.
+ String tableName = "test_table";
+ createTable(node, zoneName, tableName);
+ int tableId = TableTestUtils.getTableId(node.catalogManager, tableName, node.hybridClock.nowLong());
+ InternalTable internalTable = node.tableManager.table(tableId).internalTable();
+
+ // Stop the node
+ stopNode(0);
+
+ // Check that the storages close method was triggered
+ verify(internalTable.storage(), times(1))
Review Comment:
`times(1)` is redundant, because it's the default
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "JAkutenshi (via GitHub)" <gi...@apache.org>.
JAkutenshi commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1976602554
##########
modules/partition-replicator/src/integrationTest/java/org/apache/ignite/internal/partition/replicator/ItReplicaLifecycleTest.java:
##########
@@ -636,6 +636,32 @@ public void testScanCloseReplicaRequest() throws Exception {
assertDoesNotThrow(tx::commit);
}
+ @Test
+ public void testNodeStop() throws Exception {
+ // Prepare a single node cluster.
+ startCluster(1);
+ Node node = getNode(0);
+
+ // Prepare a zone.
+ String zoneName = "test_zone";
+ createZone(node, zoneName, 1, 1);
+
+ // Create a table to work with.
+ String tableName = "test_table";
+ createTable(node, zoneName, tableName);
+ int tableId = TableTestUtils.getTableId(node.catalogManager, tableName, node.hybridClock.nowLong());
+ InternalTable internalTable = node.tableManager.table(tableId).internalTable();
+
+ // Stop the node
+ stopNode(0);
+
+ // Check that the storages close method was triggered
+ verify(internalTable.storage(), timeout(AWAIT_TIMEOUT_MILLIS))
Review Comment:
For nothing, changed
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "JAkutenshi (via GitHub)" <gi...@apache.org>.
JAkutenshi commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1976603119
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/util/StorageUtils.java:
##########
@@ -137,6 +138,7 @@ public static void throwExceptionDependingOnStorageStateOnRebalance(StorageState
public static void throwExceptionDependingOnStorageState(StorageState state, String storageInfo) {
switch (state) {
case CLOSED:
+ assert !IgniteSystemProperties.enabledColocation() : createStorageClosedErrorMessage(storageInfo);
Review Comment:
Because the already stopped exception just will be shown in logs silently and all tests will be fine. With the assertion we will have a failed TC suite.
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1978867403
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/ZoneResourcesManager.java:
##########
@@ -182,8 +188,8 @@ TxStatePartitionStorage txStatePartitionStorage(int zoneId, int partitionId) {
return resources.txStateStorage.getPartitionStorage(partitionId);
}
-
Review Comment:
Why?
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "JAkutenshi (via GitHub)" <gi...@apache.org>.
JAkutenshi commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1978906688
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/util/StorageUtils.java:
##########
@@ -137,6 +138,7 @@ public static void throwExceptionDependingOnStorageStateOnRebalance(StorageState
public static void throwExceptionDependingOnStorageState(StorageState state, String storageInfo) {
switch (state) {
case CLOSED:
+ assert !IgniteSystemProperties.enabledColocation() : createStorageClosedErrorMessage(storageInfo);
Review Comment:
>why would we suddenly stop being interested in storages being stopped incorrectly?
Because after colocation we will cover all scenarios and we'll be sure with them, but
>I think what you want to do is to remove this line
Yes, done.
>fix TC settings to fail if this exception is detected in the logs
It should be a request to `it-assistance`?
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1978901754
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/util/StorageUtils.java:
##########
@@ -137,6 +138,7 @@ public static void throwExceptionDependingOnStorageStateOnRebalance(StorageState
public static void throwExceptionDependingOnStorageState(StorageState state, String storageInfo) {
switch (state) {
case CLOSED:
+ assert !IgniteSystemProperties.enabledColocation() : createStorageClosedErrorMessage(storageInfo);
Review Comment:
I'll explain why the current approach is weird:
1. This is an assertion that covers a line that throws an exception, which makes no sense in the real world. The only reason for it is to make TC fail. So the proper solution is to fix the TC behavior.
2. This method can throw 4 exceptions depending on the scenario, but we only add an assertion only for one of them.
3. This assertion is only enabled for the colocation track and is expected to be removed in the future, why would we suddenly stop being interested in storages being stopped incorrectly?
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "JAkutenshi (via GitHub)" <gi...@apache.org>.
JAkutenshi commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1976467333
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java:
##########
@@ -283,6 +283,12 @@ public int tableId() {
return tableId;
}
+ /** {@inheritDoc} */
Review Comment:
I see a lot of methods above are marked with it. Are you sure?
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "JAkutenshi (via GitHub)" <gi...@apache.org>.
JAkutenshi commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1976478785
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -670,7 +675,9 @@ public CompletableFuture<Void> startAsync(ComponentContext componentContext) {
attemptsObtainLock = txCfg.attemptsObtainLock().value();
- executorInclinedPlacementDriver.listen(PrimaryReplicaEvent.PRIMARY_REPLICA_EXPIRED, this::onPrimaryReplicaExpired);
+ if (!enabledColocation()) {
Review Comment:
Because this local event triggers table replica stop and the partition destruction, but under the colocation track we shouldn't even listen this.
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "JAkutenshi (via GitHub)" <gi...@apache.org>.
JAkutenshi commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1976483973
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -1936,6 +1950,13 @@ private CompletableFuture<Void> destroyTableLocally(int tableId) {
for (int partitionId = 0; partitionId < partitions; partitionId++) {
var replicationGroupId = new TablePartitionId(tableId, partitionId);
+ if (enabledColocation()) {
+ partitionReplicaLifecycleManager.unloadTableResourcesFromZoneReplica(
+ new ZonePartitionId(internalTable.zoneId(), replicationGroupId.partitionId()),
Review Comment:
Done
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/PartitionReplicaLifecycleManager.java:
##########
@@ -1428,6 +1453,27 @@ public void loadTableListenerToZoneReplica(
resources.snapshotStorageFactory().addMvPartition(tablePartitionId.tableId(), partitionMvStorageAccess);
}
+ /**
+ * Load a new table partition listener to the zone replica.
+ *
+ * @param zonePartitionId Zone partition id.
+ * @param tablePartitionId Table partition id.
+ */
+ public void unloadTableResourcesFromZoneReplica(
+ ZonePartitionId zonePartitionId,
+ TablePartitionId tablePartitionId
Review Comment:
Done
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "JAkutenshi (via GitHub)" <gi...@apache.org>.
JAkutenshi commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1976483883
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/ZonePartitionReplicaListener.java:
##########
@@ -273,6 +273,15 @@ public void addTableReplicaListener(TablePartitionId partitionId, Function<RaftC
replicas.put(partitionId.tableId(), replicaListener.apply(raftClient));
}
+ /**
+ * Remove table partition listener by table replication identifier from the current zone replica listener.
+ *
+ * @param partitionId Table partition id.
+ */
+ public void removeTableReplicaListener(TablePartitionId partitionId) {
Review Comment:
Done
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/raft/ZonePartitionRaftListener.java:
##########
@@ -371,6 +371,15 @@ public void addTableProcessor(TablePartitionId tablePartitionId, RaftTableProces
}
}
+ /**
+ * Removes a given Table Partition-level Raft processor from the set of managed processor.
+ */
+ public void removeTableProcessor(TablePartitionId tablePartitionId) {
Review Comment:
Done
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "JAkutenshi (via GitHub)" <gi...@apache.org>.
JAkutenshi commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1976483728
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java:
##########
@@ -283,6 +283,12 @@ public int tableId() {
return tableId;
}
+ /** {@inheritDoc} */
Review Comment:
Done
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "JAkutenshi (via GitHub)" <gi...@apache.org>.
JAkutenshi commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1976479015
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/PartitionReplicaLifecycleManager.java:
##########
@@ -1316,45 +1331,51 @@ public CompletableFuture<Void> stopAsync(ComponentContext componentContext) {
return Assignments.fromBytes(entry.value());
}
- private CompletableFuture<Void> weakStopAndDestroyPartition(ZonePartitionId zonePartitionId, long revision) {
- return replicaMgr.weakStopReplica(
- zonePartitionId,
- WeakReplicaStopReason.EXCLUDED_FROM_ASSIGNMENTS,
- () -> stopPartition(zonePartitionId).thenCompose(replicaWasStopped -> {
- if (!replicaWasStopped) {
- return nullCompletedFuture();
- }
-
- zoneResourcesManager.destroyZonePartitionResources(zonePartitionId);
-
- return fireEvent(
- LocalPartitionReplicaEvent.AFTER_REPLICA_DESTROYED,
- new LocalPartitionReplicaEventParameters(zonePartitionId, revision)
- );
- })
- );
- }
-
/**
- * Stops all resources associated with a given partition, like replicas and partition trackers.
+ * Stops zone replica, executes a given action and fires a given local event after if are not null.
+ *
+ * @param zonePartitionId Zone's replication group identifier.
+ * @param afterReplicaStopAction A consumer that will be executed if it is not null after zone replica stop process will be finished and
+ * stopping result will be given to the consumer.
+ * @param afterReplicaStoppedEvent A local event type that if not null should be fired in the end of the method.
+ * @param afterReplicaStoppedEventRevision A revision parameter of the local event if the last is presented. Must not be null if the
+ * event isn't null too.
*
- * @param zonePartitionId Partition ID.
- * @return Future that will be completed after all resources have been closed and the future's result answers was replica was stopped
- * correctly or not.
+ * @return Future that will be completed after zone replica was stopped and all given non-null actions are done, the future's result
+ * answers was replica was stopped correctly or not.
*/
- private CompletableFuture<Boolean> stopPartition(ZonePartitionId zonePartitionId) {
+ private CompletableFuture<Boolean> stopPartitionInternal(
+ ZonePartitionId zonePartitionId,
+ @Nullable Consumer<Boolean> afterReplicaStopAction,
+ @Nullable LocalPartitionReplicaEvent afterReplicaStoppedEvent,
Review Comment:
It possible to be null in future, but I'll remove it now, yes.
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "JAkutenshi (via GitHub)" <gi...@apache.org>.
JAkutenshi commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1978853188
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/PartitionReplicaLifecycleManager.java:
##########
@@ -1432,6 +1455,25 @@ public void loadTableListenerToZoneReplica(
resources.snapshotStorageFactory().addMvPartition(tablePartitionId.tableId(), partitionMvStorageAccess);
}
+ /**
+ * Load a new table partition listener to the zone replica.
+ *
+ * @param zonePartitionId Zone partition id.
+ * @param tableId Table's identifier.
+ */
+ public void unloadTableResourcesFromZoneReplica(
+ ZonePartitionId zonePartitionId,
+ int tableId
+ ) {
+ ZonePartitionResources resources = zoneResourcesManager.getZonePartitionResources(zonePartitionId);
+
+ resources.replicaListenerFuture().thenAccept(zoneReplicaListener -> zoneReplicaListener.removeTableReplicaListener(tableId));
Review Comment:
Done
##########
modules/partition-replicator/src/integrationTest/java/org/apache/ignite/internal/partition/replicator/ItReplicaLifecycleTest.java:
##########
@@ -636,6 +637,32 @@ public void testScanCloseReplicaRequest() throws Exception {
assertDoesNotThrow(tx::commit);
}
+ @Test
+ public void testNodeStop() throws Exception {
+ // Prepare a single node cluster.
+ startCluster(1);
+ Node node = getNode(0);
+
+ // Prepare a zone.
+ String zoneName = "test_zone";
+ createZone(node, zoneName, 1, 1);
+
+ // Create a table to work with.
+ String tableName = "test_table";
+ createTable(node, zoneName, tableName);
+ int tableId = TableTestUtils.getTableId(node.catalogManager, tableName, node.hybridClock.nowLong());
+ InternalTable internalTable = node.tableManager.table(tableId).internalTable();
+
+ // Stop the node
+ stopNode(0);
+
+ // Check that the storages close method was triggered
+ verify(internalTable.storage(), times(1))
Review Comment:
Done
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1978867403
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/ZoneResourcesManager.java:
##########
@@ -182,8 +188,8 @@ TxStatePartitionStorage txStatePartitionStorage(int zoneId, int partitionId) {
return resources.txStateStorage.getPartitionStorage(partitionId);
}
-
Review Comment:
Why?
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "JAkutenshi (via GitHub)" <gi...@apache.org>.
JAkutenshi commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1976601704
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -717,32 +724,38 @@ private CompletableFuture<Boolean> beforeZoneReplicaStarted(LocalPartitionReplic
);
}
- private CompletableFuture<Boolean> onZoneReplicaDestroyed(LocalPartitionReplicaEventParameters parameters) {
+ private CompletableFuture<Boolean> onZoneReplicaStopped(LocalPartitionReplicaEventParameters parameters) {
if (!enabledColocation()) {
return falseCompletedFuture();
}
- return inBusyLockAsync(busyLock, () -> {
- Set<TableImpl> zoneTables = zoneTables(parameters.zonePartitionId().zoneId());
+ Set<TableImpl> zoneTables = zoneTables(parameters.zonePartitionId().zoneId());
- CompletableFuture<?>[] futures = zoneTables.stream()
- .map(table -> {
- closePartitionTrackers(table.internalTable(), parameters.zonePartitionId().partitionId());
+ CompletableFuture<?>[] futures = zoneTables.stream()
+ .map(table -> supplyAsync(
Review Comment:
Ouch, my bad. 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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1976478841
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java:
##########
@@ -283,6 +283,12 @@ public int tableId() {
return tableId;
}
+ /** {@inheritDoc} */
Review Comment:
yes
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "JAkutenshi (via GitHub)" <gi...@apache.org>.
JAkutenshi commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1976481708
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -2814,7 +2835,11 @@ private CompletableFuture<Void> stopPartition(TablePartitionId tablePartitionId,
CompletableFuture<Boolean> stopReplicaFuture;
try {
- stopReplicaFuture = replicaMgr.stopReplica(tablePartitionId);
+ // In case of colocation there shouldn't be any table replica and thus it shouldn't be stopped. Moreover the excessive replica
Review Comment:
1. Huh, yes, I agree, but why I left it as is: because it was such before me and there might be a resource release protection (who knows why replica wasn't stopped, but there still shouldn't be the raft node).
2. We have a raft-node per zone partition, that starts in `PartitionReplicaLifecycleManager#createZonePartitionReplicationNode`. We shouldn't have the node with table and I want to play it safe there.
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "JAkutenshi (via GitHub)" <gi...@apache.org>.
JAkutenshi commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1976479449
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -717,32 +724,38 @@ private CompletableFuture<Boolean> beforeZoneReplicaStarted(LocalPartitionReplic
);
}
- private CompletableFuture<Boolean> onZoneReplicaDestroyed(LocalPartitionReplicaEventParameters parameters) {
+ private CompletableFuture<Boolean> onZoneReplicaStopped(LocalPartitionReplicaEventParameters parameters) {
if (!enabledColocation()) {
return falseCompletedFuture();
}
- return inBusyLockAsync(busyLock, () -> {
- Set<TableImpl> zoneTables = zoneTables(parameters.zonePartitionId().zoneId());
+ Set<TableImpl> zoneTables = zoneTables(parameters.zonePartitionId().zoneId());
Review Comment:
This code runs only for node stopping
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1978866458
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/PartitionReplicaLifecycleManager.java:
##########
@@ -1331,34 +1337,47 @@ public CompletableFuture<Void> stopAsync(ComponentContext componentContext) {
return Assignments.fromBytes(entry.value());
}
- private CompletableFuture<Void> weakStopAndDestroyPartition(ZonePartitionId zonePartitionId, long revision) {
- return replicaMgr.weakStopReplica(
- zonePartitionId,
- WeakReplicaStopReason.EXCLUDED_FROM_ASSIGNMENTS,
- () -> stopAndDestroyPartition(zonePartitionId, revision)
- );
- }
-
/**
- * Stops all resources associated with a given partition, like replicas and partition trackers.
+ * Stops zone replica, executes a given action and fires a given local event after if are not null.
Review Comment:
```suggestion
* Stops zone replica, executes a given action and fires a given local event after.
```
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1978935380
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -2814,7 +2835,11 @@ private CompletableFuture<Void> stopPartition(TablePartitionId tablePartitionId,
CompletableFuture<Boolean> stopReplicaFuture;
try {
- stopReplicaFuture = replicaMgr.stopReplica(tablePartitionId);
+ // In case of colocation there shouldn't be any table replica and thus it shouldn't be stopped. Moreover the excessive replica
Review Comment:
1. I think we can remove this change, because the replica should not exist and `replicaMgr.stopReplica` will be a no-op. But the current approach is also fine, you can leave it.
2. However, the comment is not correct, for example: `Moreover the excessive replica stop leads to raft node shutdown regardless was the table replica there or even didn't ever exist`
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "JAkutenshi (via GitHub)" <gi...@apache.org>.
JAkutenshi commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1978816637
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/util/StorageUtils.java:
##########
@@ -137,6 +138,7 @@ public static void throwExceptionDependingOnStorageStateOnRebalance(StorageState
public static void throwExceptionDependingOnStorageState(StorageState state, String storageInfo) {
switch (state) {
case CLOSED:
+ assert !IgniteSystemProperties.enabledColocation() : createStorageClosedErrorMessage(storageInfo);
Review Comment:
>Do you have a precedent for this?
E.g. I ran `testTableEstimatedSize` as `@RepeatedTest(20)` and 15 out of 20 contain `Storage is already closed`.
>Why does accessing a closed storage not result in test failures?
In most of runs previously there was the exception on a node stop, but it didn't affect on TC runs, because it happend on `@AfterEach` without fail behavior. Our TC reacts only for `AssertionError` and `NullPointerException` thus we couldn't detect the closed exception before.
>why do you only check the CLOSED status and no other statuses?
Because this state disturbs me most.
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "JAkutenshi (via GitHub)" <gi...@apache.org>.
JAkutenshi commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1976467333
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java:
##########
@@ -283,6 +283,12 @@ public int tableId() {
return tableId;
}
+ /** {@inheritDoc} */
Review Comment:
I see a lot of methods above a marked with it. Are you sure?
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "JAkutenshi (via GitHub)" <gi...@apache.org>.
JAkutenshi commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1976483821
##########
modules/partition-replicator/src/integrationTest/java/org/apache/ignite/internal/partition/replicator/ItReplicaLifecycleTest.java:
##########
@@ -649,6 +649,33 @@ public void testScanCloseReplicaRequest() throws Exception {
assertDoesNotThrow(tx::commit);
}
+ @Test
+ public void testNodeStop() throws Exception {
+ // Prepare a single node cluster.
+ startCluster(1);
+ Node node = getNode(0);
+ placementDriver.setPrimary(node.clusterService.topologyService().localMember());
Review Comment:
Agree, but I had to merge the current main
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/LocalPartitionReplicaEvent.java:
##########
@@ -28,6 +28,11 @@ public enum LocalPartitionReplicaEvent implements Event {
*/
BEFORE_REPLICA_STARTED,
+ /**
+ * Fired when partition replica has been just stopped and the related partition shouldn't be destroyed then e.g. on Ignite node stop.
Review Comment:
Fixed, thanks!
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "JAkutenshi (via GitHub)" <gi...@apache.org>.
JAkutenshi commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1978872593
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/util/StorageUtils.java:
##########
@@ -137,6 +138,7 @@ public static void throwExceptionDependingOnStorageStateOnRebalance(StorageState
public static void throwExceptionDependingOnStorageState(StorageState state, String storageInfo) {
switch (state) {
case CLOSED:
+ assert !IgniteSystemProperties.enabledColocation() : createStorageClosedErrorMessage(storageInfo);
Review Comment:
I try last argument why we may need it a little more: because there are several uncovered stop\destroy triggers for partition resources (primary replica changes, restart, table drop) and I suppose it's useful to check the scenario. If you still think that this is excessive -- I'll delete the line
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1978901754
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/util/StorageUtils.java:
##########
@@ -137,6 +138,7 @@ public static void throwExceptionDependingOnStorageStateOnRebalance(StorageState
public static void throwExceptionDependingOnStorageState(StorageState state, String storageInfo) {
switch (state) {
case CLOSED:
+ assert !IgniteSystemProperties.enabledColocation() : createStorageClosedErrorMessage(storageInfo);
Review Comment:
I'll explain why the current approach is weird:
1. This is an assertion that covers a line that throws an exception, which makes no sense in the real world. The only reason for it is to make TC fail. So the proper solution is to fix the TC behavior.
2. This method can throw 4 exceptions depending on the scenario, but we only add an assertion only for one of them.
3. This assertion is only enabled for the colocation track and is expected to be removed in the future, why would we suddenly stop being interested in storages being stopped incorrectly?
So I think what you want to do is to remove this line and fix TC settings to fail if this exception is detected in the logs.
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "JAkutenshi (via GitHub)" <gi...@apache.org>.
JAkutenshi commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1978897438
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/PartitionReplicaLifecycleManager.java:
##########
@@ -1331,34 +1337,47 @@ public CompletableFuture<Void> stopAsync(ComponentContext componentContext) {
return Assignments.fromBytes(entry.value());
}
- private CompletableFuture<Void> weakStopAndDestroyPartition(ZonePartitionId zonePartitionId, long revision) {
- return replicaMgr.weakStopReplica(
- zonePartitionId,
- WeakReplicaStopReason.EXCLUDED_FROM_ASSIGNMENTS,
- () -> stopAndDestroyPartition(zonePartitionId, revision)
- );
- }
-
/**
- * Stops all resources associated with a given partition, like replicas and partition trackers.
+ * Stops zone replica, executes a given action and fires a given local event after if are not null.
Review Comment:
Done
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "JAkutenshi (via GitHub)" <gi...@apache.org>.
JAkutenshi commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1976483792
##########
modules/partition-replicator/src/integrationTest/java/org/apache/ignite/internal/partition/replicator/ItReplicaLifecycleTest.java:
##########
@@ -649,6 +649,33 @@ public void testScanCloseReplicaRequest() throws Exception {
assertDoesNotThrow(tx::commit);
}
+ @Test
+ public void testNodeStop() throws Exception {
+ // Prepare a single node cluster.
+ startCluster(1);
+ Node node = getNode(0);
+ placementDriver.setPrimary(node.clusterService.topologyService().localMember());
+
+ // Prepare a zone.
+ String zoneName = "test_zone";
+ createZone(node, zoneName, 1, 1);
+
+ // Create a table to work with.
+ String tableName = "test_table";
+ createTable(node, zoneName, tableName);
+ int tableId = TableTestUtils.getTableId(node.catalogManager, tableName, node.hybridClock.nowLong());
+ InternalTable internalTable = node.tableManager.table(tableId).internalTable();
+
+ // Stop the node
+ stopNode(0);
+
+ // Check that the storages close method was triggered
+ verify(internalTable.storage(), timeout(AWAIT_TIMEOUT_MILLIS).atLeast(1))
Review Comment:
Fixed, by default `timeout` checks as `times(1)`.
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "JAkutenshi (via GitHub)" <gi...@apache.org>.
JAkutenshi commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1976602554
##########
modules/partition-replicator/src/integrationTest/java/org/apache/ignite/internal/partition/replicator/ItReplicaLifecycleTest.java:
##########
@@ -636,6 +636,32 @@ public void testScanCloseReplicaRequest() throws Exception {
assertDoesNotThrow(tx::commit);
}
+ @Test
+ public void testNodeStop() throws Exception {
+ // Prepare a single node cluster.
+ startCluster(1);
+ Node node = getNode(0);
+
+ // Prepare a zone.
+ String zoneName = "test_zone";
+ createZone(node, zoneName, 1, 1);
+
+ // Create a table to work with.
+ String tableName = "test_table";
+ createTable(node, zoneName, tableName);
+ int tableId = TableTestUtils.getTableId(node.catalogManager, tableName, node.hybridClock.nowLong());
+ InternalTable internalTable = node.tableManager.table(tableId).internalTable();
+
+ // Stop the node
+ stopNode(0);
+
+ // Check that the storages close method was triggered
+ verify(internalTable.storage(), timeout(AWAIT_TIMEOUT_MILLIS))
Review Comment:
For nothing, both are changed
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1976683379
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/util/StorageUtils.java:
##########
@@ -137,6 +138,7 @@ public static void throwExceptionDependingOnStorageStateOnRebalance(StorageState
public static void throwExceptionDependingOnStorageState(StorageState state, String storageInfo) {
switch (state) {
case CLOSED:
+ assert !IgniteSystemProperties.enabledColocation() : createStorageClosedErrorMessage(storageInfo);
Review Comment:
Do you have a precedent for this? Because it looks very strange now. For example, why do you only check the CLOSED status and no other statuses? Why does accessing a closed storage not result in test failures?
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "JAkutenshi (via GitHub)" <gi...@apache.org>.
JAkutenshi commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1976479110
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/PartitionReplicaLifecycleManager.java:
##########
@@ -1316,45 +1331,51 @@ public CompletableFuture<Void> stopAsync(ComponentContext componentContext) {
return Assignments.fromBytes(entry.value());
}
- private CompletableFuture<Void> weakStopAndDestroyPartition(ZonePartitionId zonePartitionId, long revision) {
- return replicaMgr.weakStopReplica(
- zonePartitionId,
- WeakReplicaStopReason.EXCLUDED_FROM_ASSIGNMENTS,
- () -> stopPartition(zonePartitionId).thenCompose(replicaWasStopped -> {
- if (!replicaWasStopped) {
- return nullCompletedFuture();
- }
-
- zoneResourcesManager.destroyZonePartitionResources(zonePartitionId);
-
- return fireEvent(
- LocalPartitionReplicaEvent.AFTER_REPLICA_DESTROYED,
- new LocalPartitionReplicaEventParameters(zonePartitionId, revision)
- );
- })
- );
- }
-
/**
- * Stops all resources associated with a given partition, like replicas and partition trackers.
+ * Stops zone replica, executes a given action and fires a given local event after if are not null.
+ *
+ * @param zonePartitionId Zone's replication group identifier.
+ * @param afterReplicaStopAction A consumer that will be executed if it is not null after zone replica stop process will be finished and
+ * stopping result will be given to the consumer.
+ * @param afterReplicaStoppedEvent A local event type that if not null should be fired in the end of the method.
+ * @param afterReplicaStoppedEventRevision A revision parameter of the local event if the last is presented. Must not be null if the
+ * event isn't null too.
*
- * @param zonePartitionId Partition ID.
- * @return Future that will be completed after all resources have been closed and the future's result answers was replica was stopped
- * correctly or not.
+ * @return Future that will be completed after zone replica was stopped and all given non-null actions are done, the future's result
+ * answers was replica was stopped correctly or not.
*/
- private CompletableFuture<Boolean> stopPartition(ZonePartitionId zonePartitionId) {
+ private CompletableFuture<Boolean> stopPartitionInternal(
+ ZonePartitionId zonePartitionId,
+ @Nullable Consumer<Boolean> afterReplicaStopAction,
Review Comment:
You propose to pass empty lambdas there?
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "JAkutenshi (via GitHub)" <gi...@apache.org>.
JAkutenshi commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1976472066
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/util/StorageUtils.java:
##########
@@ -137,6 +138,7 @@ public static void throwExceptionDependingOnStorageStateOnRebalance(StorageState
public static void throwExceptionDependingOnStorageState(StorageState state, String storageInfo) {
switch (state) {
case CLOSED:
+ assert !IgniteSystemProperties.enabledColocation() : createStorageClosedErrorMessage(storageInfo);
Review Comment:
I want to be sure that while colocation track we'll see every preventive storage closes. I add TODO for colocation cleanup
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "JAkutenshi (via GitHub)" <gi...@apache.org>.
JAkutenshi commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1978853577
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/PartitionReplicaLifecycleManager.java:
##########
@@ -1316,45 +1331,51 @@ public CompletableFuture<Void> stopAsync(ComponentContext componentContext) {
return Assignments.fromBytes(entry.value());
}
- private CompletableFuture<Void> weakStopAndDestroyPartition(ZonePartitionId zonePartitionId, long revision) {
- return replicaMgr.weakStopReplica(
- zonePartitionId,
- WeakReplicaStopReason.EXCLUDED_FROM_ASSIGNMENTS,
- () -> stopPartition(zonePartitionId).thenCompose(replicaWasStopped -> {
- if (!replicaWasStopped) {
- return nullCompletedFuture();
- }
-
- zoneResourcesManager.destroyZonePartitionResources(zonePartitionId);
-
- return fireEvent(
- LocalPartitionReplicaEvent.AFTER_REPLICA_DESTROYED,
- new LocalPartitionReplicaEventParameters(zonePartitionId, revision)
- );
- })
- );
- }
-
/**
- * Stops all resources associated with a given partition, like replicas and partition trackers.
+ * Stops zone replica, executes a given action and fires a given local event after if are not null.
+ *
+ * @param zonePartitionId Zone's replication group identifier.
+ * @param afterReplicaStopAction A consumer that will be executed if it is not null after zone replica stop process will be finished and
+ * stopping result will be given to the consumer.
+ * @param afterReplicaStoppedEvent A local event type that if not null should be fired in the end of the method.
+ * @param afterReplicaStoppedEventRevision A revision parameter of the local event if the last is presented. Must not be null if the
+ * event isn't null too.
*
- * @param zonePartitionId Partition ID.
- * @return Future that will be completed after all resources have been closed and the future's result answers was replica was stopped
- * correctly or not.
+ * @return Future that will be completed after zone replica was stopped and all given non-null actions are done, the future's result
+ * answers was replica was stopped correctly or not.
*/
- private CompletableFuture<Boolean> stopPartition(ZonePartitionId zonePartitionId) {
+ private CompletableFuture<Boolean> stopPartitionInternal(
+ ZonePartitionId zonePartitionId,
+ @Nullable Consumer<Boolean> afterReplicaStopAction,
Review Comment:
Done
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "JAkutenshi (via GitHub)" <gi...@apache.org>.
JAkutenshi commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1978919246
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/raft/ZonePartitionRaftListener.java:
##########
@@ -378,6 +378,17 @@ public void addTableProcessor(TablePartitionId tablePartitionId, RaftTableProces
}
}
+ /**
+ * Removes a given Table Partition-level Raft processor from the set of managed processors.
+ *
+ * @param tableId Table's identifier.
+ */
+ public void removeTableProcessor(int tableId) {
+ synchronized (tableProcessorsStateLock) {
Review Comment:
I think we don't, removed. The lock intended for `lastAppliedTerm` updates
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "JAkutenshi (via GitHub)" <gi...@apache.org>.
JAkutenshi commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1976480385
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -717,32 +724,38 @@ private CompletableFuture<Boolean> beforeZoneReplicaStarted(LocalPartitionReplic
);
}
- private CompletableFuture<Boolean> onZoneReplicaDestroyed(LocalPartitionReplicaEventParameters parameters) {
+ private CompletableFuture<Boolean> onZoneReplicaStopped(LocalPartitionReplicaEventParameters parameters) {
if (!enabledColocation()) {
return falseCompletedFuture();
}
- return inBusyLockAsync(busyLock, () -> {
- Set<TableImpl> zoneTables = zoneTables(parameters.zonePartitionId().zoneId());
+ Set<TableImpl> zoneTables = zoneTables(parameters.zonePartitionId().zoneId());
- CompletableFuture<?>[] futures = zoneTables.stream()
- .map(table -> {
- closePartitionTrackers(table.internalTable(), parameters.zonePartitionId().partitionId());
+ CompletableFuture<?>[] futures = zoneTables.stream()
+ .map(table -> supplyAsync(
Review Comment:
Why? `map -> supplyAsync -> tableStopFuture -> CompletableFuture<Void>`
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1976585671
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -2814,7 +2835,11 @@ private CompletableFuture<Void> stopPartition(TablePartitionId tablePartitionId,
CompletableFuture<Boolean> stopReplicaFuture;
try {
- stopReplicaFuture = replicaMgr.stopReplica(tablePartitionId);
+ // In case of colocation there shouldn't be any table replica and thus it shouldn't be stopped. Moreover the excessive replica
Review Comment:
> Huh, yes, I agree, but why I left it as is: because it was such before me and there might be a resource release protection (who knows why replica wasn't stopped, but there still shouldn't be the raft node).
I don't like this approach, please find out why it was ignored, because it might be a bug. Just because someone wrote it before doesn't mean it's correct. I agree that it can be fixed in a different PR, if you want.
> We have a raft-node per zone partition, that starts in PartitionReplicaLifecycleManager#createZonePartitionReplicationNode. We shouldn't have the node with table and I want to play it safe there.
Yes, but here we are stopping a table-partition replica that shouldn't exist, therefore stopping of the Raft node is a no-op. Therefore I don't understand the problem described in the comment.
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1976585166
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -717,32 +724,38 @@ private CompletableFuture<Boolean> beforeZoneReplicaStarted(LocalPartitionReplic
);
}
- private CompletableFuture<Boolean> onZoneReplicaDestroyed(LocalPartitionReplicaEventParameters parameters) {
+ private CompletableFuture<Boolean> onZoneReplicaStopped(LocalPartitionReplicaEventParameters parameters) {
if (!enabledColocation()) {
return falseCompletedFuture();
}
- return inBusyLockAsync(busyLock, () -> {
- Set<TableImpl> zoneTables = zoneTables(parameters.zonePartitionId().zoneId());
+ Set<TableImpl> zoneTables = zoneTables(parameters.zonePartitionId().zoneId());
- CompletableFuture<?>[] futures = zoneTables.stream()
- .map(table -> {
- closePartitionTrackers(table.internalTable(), parameters.zonePartitionId().partitionId());
+ CompletableFuture<?>[] futures = zoneTables.stream()
+ .map(table -> supplyAsync(
Review Comment:
Because the return type of `supplyAsync(() -> tableStopFuture(table))` is `CompletableFuture<CompletableFuture<Void>>`
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "JAkutenshi (via GitHub)" <gi...@apache.org>.
JAkutenshi commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1976602755
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/PartitionReplicaLifecycleManager.java:
##########
@@ -1331,34 +1337,47 @@ public CompletableFuture<Void> stopAsync(ComponentContext componentContext) {
return Assignments.fromBytes(entry.value());
}
- private CompletableFuture<Void> weakStopAndDestroyPartition(ZonePartitionId zonePartitionId, long revision) {
- return replicaMgr.weakStopReplica(
- zonePartitionId,
- WeakReplicaStopReason.EXCLUDED_FROM_ASSIGNMENTS,
- () -> stopAndDestroyPartition(zonePartitionId, revision)
- );
- }
-
/**
- * Stops all resources associated with a given partition, like replicas and partition trackers.
+ * Stops zone replica, executes a given action and fires a given local event after if are not null.
+ *
+ * @param zonePartitionId Zone's replication group identifier.
+ * @param afterReplicaStopAction A consumer that will be executed if it is not null after zone replica stop process will be finished and
+ * stopping result will be given to the consumer.
+ * @param afterReplicaStoppedEvent A local event type that if not null should be fired in the end of the method.
+ * @param afterReplicaStoppedEventRevision A revision parameter of the local event if the last is presented. Must not be null if the
+ * event isn't null too.
*
- * @param zonePartitionId Partition ID.
- * @return Future that will be completed after all resources have been closed and the future's result answers was replica was stopped
- * correctly or not.
+ * @return Future that will be completed after zone replica was stopped and all given non-null actions are done, the future's result
+ * answers was replica was stopped correctly or not.
*/
- private CompletableFuture<Boolean> stopPartition(ZonePartitionId zonePartitionId) {
+ private CompletableFuture<Boolean> stopPartitionInternal(
+ ZonePartitionId zonePartitionId,
+ @Nullable Consumer<Boolean> afterReplicaStopAction,
+ LocalPartitionReplicaEvent afterReplicaStoppedEvent,
+ Long afterReplicaStoppedEventRevision
Review Comment:
Done
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1978865317
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/util/StorageUtils.java:
##########
@@ -137,6 +138,7 @@ public static void throwExceptionDependingOnStorageStateOnRebalance(StorageState
public static void throwExceptionDependingOnStorageState(StorageState state, String storageInfo) {
switch (state) {
case CLOSED:
+ assert !IgniteSystemProperties.enabledColocation() : createStorageClosedErrorMessage(storageInfo);
Review Comment:
Why does it only disturb you in the colocation track? This code is fine as a temporary debug statement, but otherwise it is completely bizarre. If you fixed the problem that was causing this and this statement helped you - it's fine. But it's not needed anymore. We can ask for a third opinion, if you really want to leave it here.
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "JAkutenshi (via GitHub)" <gi...@apache.org>.
JAkutenshi commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1978942769
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -2814,7 +2835,11 @@ private CompletableFuture<Void> stopPartition(TablePartitionId tablePartitionId,
CompletableFuture<Boolean> stopReplicaFuture;
try {
- stopReplicaFuture = replicaMgr.stopReplica(tablePartitionId);
+ // In case of colocation there shouldn't be any table replica and thus it shouldn't be stopped. Moreover the excessive replica
Review Comment:
Aha, agree, the comment isn't related, removed
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "JAkutenshi (via GitHub)" <gi...@apache.org>.
JAkutenshi commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1978919246
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/raft/ZonePartitionRaftListener.java:
##########
@@ -378,6 +378,17 @@ public void addTableProcessor(TablePartitionId tablePartitionId, RaftTableProces
}
}
+ /**
+ * Removes a given Table Partition-level Raft processor from the set of managed processors.
+ *
+ * @param tableId Table's identifier.
+ */
+ public void removeTableProcessor(int tableId) {
+ synchronized (tableProcessorsStateLock) {
Review Comment:
I think we don't, removed. The lock intended for `lastAppliedTerm` usages
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1978916993
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/util/StorageUtils.java:
##########
@@ -137,6 +138,7 @@ public static void throwExceptionDependingOnStorageStateOnRebalance(StorageState
public static void throwExceptionDependingOnStorageState(StorageState state, String storageInfo) {
switch (state) {
case CLOSED:
+ assert !IgniteSystemProperties.enabledColocation() : createStorageClosedErrorMessage(storageInfo);
Review Comment:
> It should be a request to it-assistance?
I don't know, but let's try
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1976586211
##########
modules/partition-replicator/src/integrationTest/java/org/apache/ignite/internal/partition/replicator/ItReplicaLifecycleTest.java:
##########
@@ -636,6 +636,32 @@ public void testScanCloseReplicaRequest() throws Exception {
assertDoesNotThrow(tx::commit);
}
+ @Test
+ public void testNodeStop() throws Exception {
+ // Prepare a single node cluster.
+ startCluster(1);
+ Node node = getNode(0);
+
+ // Prepare a zone.
+ String zoneName = "test_zone";
+ createZone(node, zoneName, 1, 1);
+
+ // Create a table to work with.
+ String tableName = "test_table";
+ createTable(node, zoneName, tableName);
+ int tableId = TableTestUtils.getTableId(node.catalogManager, tableName, node.hybridClock.nowLong());
+ InternalTable internalTable = node.tableManager.table(tableId).internalTable();
+
+ // Stop the node
+ stopNode(0);
+
+ // Check that the storages close method was triggered
+ verify(internalTable.storage(), timeout(AWAIT_TIMEOUT_MILLIS))
Review Comment:
Why do we need to verify with a timeout?
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/util/StorageUtils.java:
##########
@@ -137,6 +138,7 @@ public static void throwExceptionDependingOnStorageStateOnRebalance(StorageState
public static void throwExceptionDependingOnStorageState(StorageState state, String storageInfo) {
switch (state) {
case CLOSED:
+ assert !IgniteSystemProperties.enabledColocation() : createStorageClosedErrorMessage(storageInfo);
Review Comment:
But this code already throws an exception, why do you change it to an assertion instead?
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/raft/ZonePartitionRaftListener.java:
##########
@@ -378,6 +378,17 @@ public void addTableProcessor(TablePartitionId tablePartitionId, RaftTableProces
}
}
+ /**
+ * Removes a given Table Partition-level Raft processor from the set of managed processors.
+ *
+ * @param tableId Table's identifier.
+ */
+ public void removeTableProcessor(int tableId) {
+ synchronized (tableProcessorsStateLock) {
Review Comment:
Why do you need to take the lock here?
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/PartitionReplicaLifecycleManager.java:
##########
@@ -1432,6 +1455,25 @@ public void loadTableListenerToZoneReplica(
resources.snapshotStorageFactory().addMvPartition(tablePartitionId.tableId(), partitionMvStorageAccess);
}
+ /**
+ * Load a new table partition listener to the zone replica.
+ *
+ * @param zonePartitionId Zone partition id.
+ * @param tableId Table's identifier.
+ */
+ public void unloadTableResourcesFromZoneReplica(
+ ZonePartitionId zonePartitionId,
+ int tableId
+ ) {
+ ZonePartitionResources resources = zoneResourcesManager.getZonePartitionResources(zonePartitionId);
+
+ resources.replicaListenerFuture().thenAccept(zoneReplicaListener -> zoneReplicaListener.removeTableReplicaListener(tableId));
Review Comment:
I propose to move this code to the `ZoneResourcesManager`, to something like `zoneResourcesManager.removeTableResources(tableId)`
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/PartitionReplicaLifecycleManager.java:
##########
@@ -1331,34 +1337,47 @@ public CompletableFuture<Void> stopAsync(ComponentContext componentContext) {
return Assignments.fromBytes(entry.value());
}
- private CompletableFuture<Void> weakStopAndDestroyPartition(ZonePartitionId zonePartitionId, long revision) {
- return replicaMgr.weakStopReplica(
- zonePartitionId,
- WeakReplicaStopReason.EXCLUDED_FROM_ASSIGNMENTS,
- () -> stopAndDestroyPartition(zonePartitionId, revision)
- );
- }
-
/**
- * Stops all resources associated with a given partition, like replicas and partition trackers.
+ * Stops zone replica, executes a given action and fires a given local event after if are not null.
+ *
+ * @param zonePartitionId Zone's replication group identifier.
+ * @param afterReplicaStopAction A consumer that will be executed if it is not null after zone replica stop process will be finished and
+ * stopping result will be given to the consumer.
+ * @param afterReplicaStoppedEvent A local event type that if not null should be fired in the end of the method.
+ * @param afterReplicaStoppedEventRevision A revision parameter of the local event if the last is presented. Must not be null if the
+ * event isn't null too.
*
- * @param zonePartitionId Partition ID.
- * @return Future that will be completed after all resources have been closed and the future's result answers was replica was stopped
- * correctly or not.
+ * @return Future that will be completed after zone replica was stopped and all given non-null actions are done, the future's result
+ * answers was replica was stopped correctly or not.
*/
- private CompletableFuture<Boolean> stopPartition(ZonePartitionId zonePartitionId) {
+ private CompletableFuture<Boolean> stopPartitionInternal(
+ ZonePartitionId zonePartitionId,
+ @Nullable Consumer<Boolean> afterReplicaStopAction,
+ LocalPartitionReplicaEvent afterReplicaStoppedEvent,
+ Long afterReplicaStoppedEventRevision
Review Comment:
```suggestion
long afterReplicaStoppedEventRevision
```
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1976585825
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/PartitionReplicaLifecycleManager.java:
##########
@@ -1316,45 +1331,51 @@ public CompletableFuture<Void> stopAsync(ComponentContext componentContext) {
return Assignments.fromBytes(entry.value());
}
- private CompletableFuture<Void> weakStopAndDestroyPartition(ZonePartitionId zonePartitionId, long revision) {
- return replicaMgr.weakStopReplica(
- zonePartitionId,
- WeakReplicaStopReason.EXCLUDED_FROM_ASSIGNMENTS,
- () -> stopPartition(zonePartitionId).thenCompose(replicaWasStopped -> {
- if (!replicaWasStopped) {
- return nullCompletedFuture();
- }
-
- zoneResourcesManager.destroyZonePartitionResources(zonePartitionId);
-
- return fireEvent(
- LocalPartitionReplicaEvent.AFTER_REPLICA_DESTROYED,
- new LocalPartitionReplicaEventParameters(zonePartitionId, revision)
- );
- })
- );
- }
-
/**
- * Stops all resources associated with a given partition, like replicas and partition trackers.
+ * Stops zone replica, executes a given action and fires a given local event after if are not null.
+ *
+ * @param zonePartitionId Zone's replication group identifier.
+ * @param afterReplicaStopAction A consumer that will be executed if it is not null after zone replica stop process will be finished and
+ * stopping result will be given to the consumer.
+ * @param afterReplicaStoppedEvent A local event type that if not null should be fired in the end of the method.
+ * @param afterReplicaStoppedEventRevision A revision parameter of the local event if the last is presented. Must not be null if the
+ * event isn't null too.
*
- * @param zonePartitionId Partition ID.
- * @return Future that will be completed after all resources have been closed and the future's result answers was replica was stopped
- * correctly or not.
+ * @return Future that will be completed after zone replica was stopped and all given non-null actions are done, the future's result
+ * answers was replica was stopped correctly or not.
*/
- private CompletableFuture<Boolean> stopPartition(ZonePartitionId zonePartitionId) {
+ private CompletableFuture<Boolean> stopPartitionInternal(
+ ZonePartitionId zonePartitionId,
+ @Nullable Consumer<Boolean> afterReplicaStopAction,
Review Comment:
yes
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1975433506
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/LocalPartitionReplicaEvent.java:
##########
@@ -28,6 +28,11 @@ public enum LocalPartitionReplicaEvent implements Event {
*/
BEFORE_REPLICA_STARTED,
+ /**
+ * Fired when partition replica has been just stopped and the related partition shouldn't be destroyed then e.g. on Ignite node stop.
Review Comment:
```suggestion
* Fired when partition replica has just been stopped and the related partition shouldn't be destroyed e.g. on Ignite node stop.
```
##########
modules/partition-replicator/src/integrationTest/java/org/apache/ignite/internal/partition/replicator/ItReplicaLifecycleTest.java:
##########
@@ -649,6 +649,33 @@ public void testScanCloseReplicaRequest() throws Exception {
assertDoesNotThrow(tx::commit);
}
+ @Test
+ public void testNodeStop() throws Exception {
+ // Prepare a single node cluster.
+ startCluster(1);
+ Node node = getNode(0);
+ placementDriver.setPrimary(node.clusterService.topologyService().localMember());
+
+ // Prepare a zone.
+ String zoneName = "test_zone";
+ createZone(node, zoneName, 1, 1);
+
+ // Create a table to work with.
+ String tableName = "test_table";
+ createTable(node, zoneName, tableName);
+ int tableId = TableTestUtils.getTableId(node.catalogManager, tableName, node.hybridClock.nowLong());
+ InternalTable internalTable = node.tableManager.table(tableId).internalTable();
+
+ // Stop the node
+ stopNode(0);
+
+ // Check that the storages close method was triggered
+ verify(internalTable.storage(), timeout(AWAIT_TIMEOUT_MILLIS).atLeast(1))
Review Comment:
Why `atLeast`? Do we close it multiple times?
##########
modules/partition-replicator/src/integrationTest/java/org/apache/ignite/internal/partition/replicator/ItReplicaLifecycleTest.java:
##########
@@ -649,6 +649,33 @@ public void testScanCloseReplicaRequest() throws Exception {
assertDoesNotThrow(tx::commit);
}
+ @Test
+ public void testNodeStop() throws Exception {
+ // Prepare a single node cluster.
+ startCluster(1);
+ Node node = getNode(0);
+ placementDriver.setPrimary(node.clusterService.topologyService().localMember());
Review Comment:
I think this line is no longer needed
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/ZonePartitionReplicaListener.java:
##########
@@ -273,6 +273,15 @@ public void addTableReplicaListener(TablePartitionId partitionId, Function<RaftC
replicas.put(partitionId.tableId(), replicaListener.apply(raftClient));
}
+ /**
+ * Remove table partition listener by table replication identifier from the current zone replica listener.
Review Comment:
```suggestion
* Removes table partition listener by table replication identifier from the current zone replica listener.
```
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/PartitionReplicaLifecycleManager.java:
##########
@@ -1316,45 +1331,51 @@ public CompletableFuture<Void> stopAsync(ComponentContext componentContext) {
return Assignments.fromBytes(entry.value());
}
- private CompletableFuture<Void> weakStopAndDestroyPartition(ZonePartitionId zonePartitionId, long revision) {
- return replicaMgr.weakStopReplica(
- zonePartitionId,
- WeakReplicaStopReason.EXCLUDED_FROM_ASSIGNMENTS,
- () -> stopPartition(zonePartitionId).thenCompose(replicaWasStopped -> {
- if (!replicaWasStopped) {
- return nullCompletedFuture();
- }
-
- zoneResourcesManager.destroyZonePartitionResources(zonePartitionId);
-
- return fireEvent(
- LocalPartitionReplicaEvent.AFTER_REPLICA_DESTROYED,
- new LocalPartitionReplicaEventParameters(zonePartitionId, revision)
- );
- })
- );
- }
-
/**
- * Stops all resources associated with a given partition, like replicas and partition trackers.
+ * Stops zone replica, executes a given action and fires a given local event after if are not null.
+ *
+ * @param zonePartitionId Zone's replication group identifier.
+ * @param afterReplicaStopAction A consumer that will be executed if it is not null after zone replica stop process will be finished and
+ * stopping result will be given to the consumer.
+ * @param afterReplicaStoppedEvent A local event type that if not null should be fired in the end of the method.
+ * @param afterReplicaStoppedEventRevision A revision parameter of the local event if the last is presented. Must not be null if the
+ * event isn't null too.
*
- * @param zonePartitionId Partition ID.
- * @return Future that will be completed after all resources have been closed and the future's result answers was replica was stopped
- * correctly or not.
+ * @return Future that will be completed after zone replica was stopped and all given non-null actions are done, the future's result
+ * answers was replica was stopped correctly or not.
*/
- private CompletableFuture<Boolean> stopPartition(ZonePartitionId zonePartitionId) {
+ private CompletableFuture<Boolean> stopPartitionInternal(
+ ZonePartitionId zonePartitionId,
+ @Nullable Consumer<Boolean> afterReplicaStopAction,
+ @Nullable LocalPartitionReplicaEvent afterReplicaStoppedEvent,
Review Comment:
This parameter is never null
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/PartitionReplicaLifecycleManager.java:
##########
@@ -1316,45 +1331,51 @@ public CompletableFuture<Void> stopAsync(ComponentContext componentContext) {
return Assignments.fromBytes(entry.value());
}
- private CompletableFuture<Void> weakStopAndDestroyPartition(ZonePartitionId zonePartitionId, long revision) {
- return replicaMgr.weakStopReplica(
- zonePartitionId,
- WeakReplicaStopReason.EXCLUDED_FROM_ASSIGNMENTS,
- () -> stopPartition(zonePartitionId).thenCompose(replicaWasStopped -> {
- if (!replicaWasStopped) {
- return nullCompletedFuture();
- }
-
- zoneResourcesManager.destroyZonePartitionResources(zonePartitionId);
-
- return fireEvent(
- LocalPartitionReplicaEvent.AFTER_REPLICA_DESTROYED,
- new LocalPartitionReplicaEventParameters(zonePartitionId, revision)
- );
- })
- );
- }
-
/**
- * Stops all resources associated with a given partition, like replicas and partition trackers.
+ * Stops zone replica, executes a given action and fires a given local event after if are not null.
+ *
+ * @param zonePartitionId Zone's replication group identifier.
+ * @param afterReplicaStopAction A consumer that will be executed if it is not null after zone replica stop process will be finished and
+ * stopping result will be given to the consumer.
+ * @param afterReplicaStoppedEvent A local event type that if not null should be fired in the end of the method.
+ * @param afterReplicaStoppedEventRevision A revision parameter of the local event if the last is presented. Must not be null if the
+ * event isn't null too.
*
- * @param zonePartitionId Partition ID.
- * @return Future that will be completed after all resources have been closed and the future's result answers was replica was stopped
- * correctly or not.
+ * @return Future that will be completed after zone replica was stopped and all given non-null actions are done, the future's result
+ * answers was replica was stopped correctly or not.
*/
- private CompletableFuture<Boolean> stopPartition(ZonePartitionId zonePartitionId) {
+ private CompletableFuture<Boolean> stopPartitionInternal(
+ ZonePartitionId zonePartitionId,
+ @Nullable Consumer<Boolean> afterReplicaStopAction,
Review Comment:
I propose to not pass null here as well
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -717,32 +724,38 @@ private CompletableFuture<Boolean> beforeZoneReplicaStarted(LocalPartitionReplic
);
}
- private CompletableFuture<Boolean> onZoneReplicaDestroyed(LocalPartitionReplicaEventParameters parameters) {
+ private CompletableFuture<Boolean> onZoneReplicaStopped(LocalPartitionReplicaEventParameters parameters) {
if (!enabledColocation()) {
return falseCompletedFuture();
}
- return inBusyLockAsync(busyLock, () -> {
- Set<TableImpl> zoneTables = zoneTables(parameters.zonePartitionId().zoneId());
+ Set<TableImpl> zoneTables = zoneTables(parameters.zonePartitionId().zoneId());
Review Comment:
What about taking the busy lock?
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -1936,6 +1950,13 @@ private CompletableFuture<Void> destroyTableLocally(int tableId) {
for (int partitionId = 0; partitionId < partitions; partitionId++) {
var replicationGroupId = new TablePartitionId(tableId, partitionId);
+ if (enabledColocation()) {
+ partitionReplicaLifecycleManager.unloadTableResourcesFromZoneReplica(
+ new ZonePartitionId(internalTable.zoneId(), replicationGroupId.partitionId()),
Review Comment:
```suggestion
new ZonePartitionId(internalTable.zoneId(), partitionId),
```
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/raft/ZonePartitionRaftListener.java:
##########
@@ -371,6 +371,15 @@ public void addTableProcessor(TablePartitionId tablePartitionId, RaftTableProces
}
}
+ /**
+ * Removes a given Table Partition-level Raft processor from the set of managed processor.
Review Comment:
```suggestion
* Removes a given Table Partition-level Raft processor from the set of managed processors.
```
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -670,7 +675,9 @@ public CompletableFuture<Void> startAsync(ComponentContext componentContext) {
attemptsObtainLock = txCfg.attemptsObtainLock().value();
- executorInclinedPlacementDriver.listen(PrimaryReplicaEvent.PRIMARY_REPLICA_EXPIRED, this::onPrimaryReplicaExpired);
+ if (!enabledColocation()) {
Review Comment:
Why is this needed?
##########
modules/table/src/main/java/org/apache/ignite/internal/table/InternalTable.java:
##########
@@ -61,6 +61,13 @@ public interface InternalTable extends ManuallyCloseable {
*/
int tableId();
+ /**
+ * Gets a zone id what the table belongs.
Review Comment:
```suggestion
* Returns the zone ID that the table belongs to.
```
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/util/StorageUtils.java:
##########
@@ -137,6 +138,7 @@ public static void throwExceptionDependingOnStorageStateOnRebalance(StorageState
public static void throwExceptionDependingOnStorageState(StorageState state, String storageInfo) {
switch (state) {
case CLOSED:
+ assert !IgniteSystemProperties.enabledColocation() : createStorageClosedErrorMessage(storageInfo);
Review Comment:
Why is this line needed?
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/raft/ZonePartitionRaftListener.java:
##########
@@ -371,6 +371,15 @@ public void addTableProcessor(TablePartitionId tablePartitionId, RaftTableProces
}
}
+ /**
+ * Removes a given Table Partition-level Raft processor from the set of managed processor.
+ */
+ public void removeTableProcessor(TablePartitionId tablePartitionId) {
Review Comment:
Same question
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java:
##########
@@ -283,6 +283,12 @@ public int tableId() {
return tableId;
}
+ /** {@inheritDoc} */
Review Comment:
We don't use this annotation, please remove it
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/PartitionReplicaLifecycleManager.java:
##########
@@ -1428,6 +1453,27 @@ public void loadTableListenerToZoneReplica(
resources.snapshotStorageFactory().addMvPartition(tablePartitionId.tableId(), partitionMvStorageAccess);
}
+ /**
+ * Load a new table partition listener to the zone replica.
+ *
+ * @param zonePartitionId Zone partition id.
+ * @param tablePartitionId Table partition id.
+ */
+ public void unloadTableResourcesFromZoneReplica(
+ ZonePartitionId zonePartitionId,
+ TablePartitionId tablePartitionId
Review Comment:
This should be `int tableId`
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/ZonePartitionReplicaListener.java:
##########
@@ -273,6 +273,15 @@ public void addTableReplicaListener(TablePartitionId partitionId, Function<RaftC
replicas.put(partitionId.tableId(), replicaListener.apply(raftClient));
}
+ /**
+ * Remove table partition listener by table replication identifier from the current zone replica listener.
+ *
+ * @param partitionId Table partition id.
+ */
+ public void removeTableReplicaListener(TablePartitionId partitionId) {
Review Comment:
Shall we pass `int tableId` here instead?
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/PartitionReplicaLifecycleManager.java:
##########
@@ -1316,45 +1331,51 @@ public CompletableFuture<Void> stopAsync(ComponentContext componentContext) {
return Assignments.fromBytes(entry.value());
}
- private CompletableFuture<Void> weakStopAndDestroyPartition(ZonePartitionId zonePartitionId, long revision) {
- return replicaMgr.weakStopReplica(
- zonePartitionId,
- WeakReplicaStopReason.EXCLUDED_FROM_ASSIGNMENTS,
- () -> stopPartition(zonePartitionId).thenCompose(replicaWasStopped -> {
- if (!replicaWasStopped) {
- return nullCompletedFuture();
- }
-
- zoneResourcesManager.destroyZonePartitionResources(zonePartitionId);
-
- return fireEvent(
- LocalPartitionReplicaEvent.AFTER_REPLICA_DESTROYED,
- new LocalPartitionReplicaEventParameters(zonePartitionId, revision)
- );
- })
- );
- }
-
/**
- * Stops all resources associated with a given partition, like replicas and partition trackers.
+ * Stops zone replica, executes a given action and fires a given local event after if are not null.
+ *
+ * @param zonePartitionId Zone's replication group identifier.
+ * @param afterReplicaStopAction A consumer that will be executed if it is not null after zone replica stop process will be finished and
+ * stopping result will be given to the consumer.
+ * @param afterReplicaStoppedEvent A local event type that if not null should be fired in the end of the method.
+ * @param afterReplicaStoppedEventRevision A revision parameter of the local event if the last is presented. Must not be null if the
+ * event isn't null too.
*
- * @param zonePartitionId Partition ID.
- * @return Future that will be completed after all resources have been closed and the future's result answers was replica was stopped
- * correctly or not.
+ * @return Future that will be completed after zone replica was stopped and all given non-null actions are done, the future's result
+ * answers was replica was stopped correctly or not.
*/
- private CompletableFuture<Boolean> stopPartition(ZonePartitionId zonePartitionId) {
+ private CompletableFuture<Boolean> stopPartitionInternal(
+ ZonePartitionId zonePartitionId,
+ @Nullable Consumer<Boolean> afterReplicaStopAction,
+ @Nullable LocalPartitionReplicaEvent afterReplicaStoppedEvent,
+ @Nullable Long afterReplicaStoppedEventRevision
Review Comment:
This parameter is never null
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -2814,7 +2835,11 @@ private CompletableFuture<Void> stopPartition(TablePartitionId tablePartitionId,
CompletableFuture<Boolean> stopReplicaFuture;
try {
- stopReplicaFuture = replicaMgr.stopReplica(tablePartitionId);
+ // In case of colocation there shouldn't be any table replica and thus it shouldn't be stopped. Moreover the excessive replica
Review Comment:
This is weird for two reasons:
1. Why do we ignore the `isRemovedFuture` result inside `replicaMgr.stopReplica` and always stop the raft node?
2. Why do we have a raft node at all in the colocation case?
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -717,32 +724,38 @@ private CompletableFuture<Boolean> beforeZoneReplicaStarted(LocalPartitionReplic
);
}
- private CompletableFuture<Boolean> onZoneReplicaDestroyed(LocalPartitionReplicaEventParameters parameters) {
+ private CompletableFuture<Boolean> onZoneReplicaStopped(LocalPartitionReplicaEventParameters parameters) {
if (!enabledColocation()) {
return falseCompletedFuture();
}
- return inBusyLockAsync(busyLock, () -> {
- Set<TableImpl> zoneTables = zoneTables(parameters.zonePartitionId().zoneId());
+ Set<TableImpl> zoneTables = zoneTables(parameters.zonePartitionId().zoneId());
- CompletableFuture<?>[] futures = zoneTables.stream()
- .map(table -> {
- closePartitionTrackers(table.internalTable(), parameters.zonePartitionId().partitionId());
+ CompletableFuture<?>[] futures = zoneTables.stream()
+ .map(table -> supplyAsync(
Review Comment:
You have a bug here, must be:
```
CompletableFuture<?>[] futures = zoneTables.stream()
.map(table -> supplyAsync(() -> tableStopFuture(table), ioExecutor).thenCompose(identity()))
.toArray(CompletableFuture[]::new);
```
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "JAkutenshi (via GitHub)" <gi...@apache.org>.
JAkutenshi commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1976483911
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/raft/ZonePartitionRaftListener.java:
##########
@@ -371,6 +371,15 @@ public void addTableProcessor(TablePartitionId tablePartitionId, RaftTableProces
}
}
+ /**
+ * Removes a given Table Partition-level Raft processor from the set of managed processor.
Review Comment:
Fixed, thanks!
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/ZonePartitionReplicaListener.java:
##########
@@ -273,6 +273,15 @@ public void addTableReplicaListener(TablePartitionId partitionId, Function<RaftC
replicas.put(partitionId.tableId(), replicaListener.apply(raftClient));
}
+ /**
+ * Remove table partition listener by table replication identifier from the current zone replica listener.
Review Comment:
Fixed, thanks!
##########
modules/table/src/main/java/org/apache/ignite/internal/table/InternalTable.java:
##########
@@ -61,6 +61,13 @@ public interface InternalTable extends ManuallyCloseable {
*/
int tableId();
+ /**
+ * Gets a zone id what the table belongs.
Review Comment:
Fixed, thanks!
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1978861687
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/PartitionReplicaLifecycleManager.java:
##########
@@ -1432,6 +1455,25 @@ public void loadTableListenerToZoneReplica(
resources.snapshotStorageFactory().addMvPartition(tablePartitionId.tableId(), partitionMvStorageAccess);
}
+ /**
+ * Load a new table partition listener to the zone replica.
+ *
+ * @param zonePartitionId Zone partition id.
+ * @param tableId Table's identifier.
+ */
+ public void unloadTableResourcesFromZoneReplica(
+ ZonePartitionId zonePartitionId,
+ int tableId
+ ) {
+ ZonePartitionResources resources = zoneResourcesManager.getZonePartitionResources(zonePartitionId);
+
+ resources.replicaListenerFuture().thenAccept(zoneReplicaListener -> zoneReplicaListener.removeTableReplicaListener(tableId));
Review Comment:
Sorry, I meant to move all related code into a method, not just this line =)
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "JAkutenshi (via GitHub)" <gi...@apache.org>.
JAkutenshi commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1978867442
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/PartitionReplicaLifecycleManager.java:
##########
@@ -1432,6 +1455,25 @@ public void loadTableListenerToZoneReplica(
resources.snapshotStorageFactory().addMvPartition(tablePartitionId.tableId(), partitionMvStorageAccess);
}
+ /**
+ * Load a new table partition listener to the zone replica.
+ *
+ * @param zonePartitionId Zone partition id.
+ * @param tableId Table's identifier.
+ */
+ public void unloadTableResourcesFromZoneReplica(
+ ZonePartitionId zonePartitionId,
+ int tableId
+ ) {
+ ZonePartitionResources resources = zoneResourcesManager.getZonePartitionResources(zonePartitionId);
+
+ resources.replicaListenerFuture().thenAccept(zoneReplicaListener -> zoneReplicaListener.removeTableReplicaListener(tableId));
Review Comment:
Obviously, my mistake >< Moved
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "JAkutenshi (via GitHub)" <gi...@apache.org>.
JAkutenshi commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1978919246
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/raft/ZonePartitionRaftListener.java:
##########
@@ -378,6 +378,17 @@ public void addTableProcessor(TablePartitionId tablePartitionId, RaftTableProces
}
}
+ /**
+ * Removes a given Table Partition-level Raft processor from the set of managed processors.
+ *
+ * @param tableId Table's identifier.
+ */
+ public void removeTableProcessor(int tableId) {
+ synchronized (tableProcessorsStateLock) {
Review Comment:
I think we don't, removed.
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "sk0x50 (via GitHub)" <gi...@apache.org>.
sk0x50 merged PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300
--
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
Re: [PR] IGNITE-24640 Fix storages stoppings on PartitionReplicaLifecycleManager#beforeNodeStop [ignite-3]
Posted by "JAkutenshi (via GitHub)" <gi...@apache.org>.
JAkutenshi commented on code in PR #5300:
URL: https://github.com/apache/ignite-3/pull/5300#discussion_r1978923234
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -2814,7 +2835,11 @@ private CompletableFuture<Void> stopPartition(TablePartitionId tablePartitionId,
CompletableFuture<Boolean> stopReplicaFuture;
try {
- stopReplicaFuture = replicaMgr.stopReplica(tablePartitionId);
+ // In case of colocation there shouldn't be any table replica and thus it shouldn't be stopped. Moreover the excessive replica
Review Comment:
I changed the replica removal flag usage and run tests with it. Do you mean some another 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