rpuch commented on code in PR #1325:
URL: https://github.com/apache/ignite-3/pull/1325#discussion_r1032075319
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/checkpoint/Checkpointer.java:
##########
@@ -500,13 +501,13 @@ private void syncUpdatedPageStores(
FilePageStore filePageStore =
filePageStoreManager.getStore(partId.getGroupId(), partId.getPartitionId());
if (filePageStore != null &&
!filePageStore.isMarkedToDestroy()) {
-
currentCheckpointProgress.onStartPartitionProcessing(partId.getGroupId(),
partId.getPartitionId());
+
currentCheckpointProgress.onStartPartitionProcessing(partId);
fsyncDeltaFilePageStoreOnCheckpointThread(filePageStore, entry.getValue());
renameDeltaFileOnCheckpointThread(filePageStore, partId);
-
currentCheckpointProgress.onFinishPartitionProcessing(partId.getGroupId(),
partId.getPartitionId());
+
currentCheckpointProgress.onFinishPartitionProcessing(partId);
Review Comment:
Shouldn't this also be invoked in a `finally` block?
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/compaction/Compactor.java:
##########
@@ -42,15 +43,13 @@
import org.apache.ignite.internal.util.IgniteUtils;
import org.apache.ignite.internal.util.worker.IgniteWorker;
import org.apache.ignite.internal.util.worker.IgniteWorkerListener;
-import org.apache.ignite.lang.IgniteInternalCheckedException;
import org.apache.ignite.lang.IgniteInternalException;
-import org.apache.ignite.lang.IgniteStringFormatter;
import org.jetbrains.annotations.Nullable;
/**
* Entity to compact delta files.
*
- * <p>To start compacting delta files, you need to notify about the appearance
of {@link #onAddingDeltaFiles() delta files ready for
+ * <p>To start compacting delta files, you need to notify about the appearance
of {@link #triggerCompaction() delta files ready for
Review Comment:
Probably this javadoc needs to be reworded as the method seems to be now
about triggering the compaction, not about appearance of something
##########
modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/AbstractMvTableStorageTest.java:
##########
@@ -444,7 +444,7 @@ public void testDestroyPartition() throws Exception {
assertThrows(StorageClosedException.class, () ->
getAll(scanFromSortedIndexCursor));
// Let's check that nothing will happen if we try to destroy a
non-existing partition.
- tableStorage.destroyPartition(PARTITION_ID);
+ assertDoesNotThrow(() -> tableStorage.destroyPartition(PARTITION_ID));
Review Comment:
Let's also check that the returned future will successfully complete
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/snapshot/incoming/IncomingSnapshotCopier.java:
##########
@@ -195,8 +195,8 @@ private CompletableFuture<?>
prepareMvPartitionStorageForRebalance(Executor exec
return completedFuture(null);
}
- return CompletableFuture.supplyAsync(() ->
partitionSnapshotStorage.partition().reCreateMvPartitionStorage(), executor)
- .thenCompose(mvPartitionStorage -> {
+ return
partitionSnapshotStorage.partition().reCreateMvPartitionStorage()
Review Comment:
Looks like MV partition will be destroyed on the current thread. Before this
change, it would be executed on `executor`. Is this ok? Can partition destroy
process be heavy?
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/compaction/Compactor.java:
##########
@@ -401,38 +415,24 @@ void mergeDeltaFileToMainFile(
}
/**
- * Callback on destruction of the partition of the corresponding group.
+ * Prepares the compacter to destroy a partition.
Review Comment:
```suggestion
* Prepares the compactor to destroy a partition.
```
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/AbstractPageMemoryTableStorage.java:
##########
@@ -49,6 +51,8 @@ public abstract class AbstractPageMemoryTableStorage
implements MvTableStorage {
private volatile
AtomicReferenceArray<AbstractPageMemoryMvPartitionStorage> mvPartitions;
+ protected final ConcurrentMap<Integer, CompletableFuture<Void>>
partitionIdDestroyFuture = new ConcurrentHashMap<>();
Review Comment:
I suggest to name this field in plular form (futures) or append 'Map' to the
end so that the name gives a hint that it's not about just one future
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
