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-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
