ibessonov commented on code in PR #4437:
URL: https://github.com/apache/ignite-3/pull/4437#discussion_r1775153678
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/checkpoint/CheckpointPages.java:
##########
@@ -17,75 +17,190 @@
package org.apache.ignite.internal.pagememory.persistence.checkpoint;
+import static
org.apache.ignite.internal.pagememory.persistence.checkpoint.CheckpointState.PAGES_SORTED;
import static org.apache.ignite.internal.util.IgniteUtils.getUninterruptibly;
+import java.nio.ByteBuffer;
import java.util.Set;
import java.util.concurrent.CancellationException;
-import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ExecutionException;
import org.apache.ignite.internal.lang.IgniteInternalCheckedException;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.logger.Loggers;
import org.apache.ignite.internal.pagememory.FullPageId;
+import org.apache.ignite.internal.pagememory.persistence.GroupPartitionId;
+import org.apache.ignite.internal.pagememory.persistence.PageStoreWriter;
+import org.apache.ignite.internal.pagememory.persistence.PersistentPageMemory;
+import org.apache.ignite.internal.pagememory.persistence.store.FilePageStore;
+import
org.apache.ignite.internal.pagememory.persistence.store.FilePageStoreManager;
+import org.jetbrains.annotations.Nullable;
/**
- * View of pages which should be stored during current checkpoint.
+ * Class contains dirty pages of the segment that will need to be written at a
checkpoint or page replacement. It also contains helper
+ * methods before writing pages.
+ *
+ * <p>For correct parallel operation of the checkpoint writer and page
replacement, external synchronization must be used.</p>
+ *
+ * @see PersistentPageMemory#checkpointWritePage(FullPageId, ByteBuffer,
PageStoreWriter, CheckpointMetricsTracker)
+ * @see PersistentPageMemory.Segment#tryToRemovePage(FullPageId, long)
*/
public class CheckpointPages {
- private final Set<FullPageId> segmentPages;
+ private static final IgniteLogger LOG =
Loggers.forClass(IgniteLogger.class);
+
+ private final Set<FullPageId> pageIds;
- private final CompletableFuture<?> allowToReplace;
+ private final CheckpointProgressImpl checkpointProgress;
/**
* Constructor.
*
- * @param pages Pages which would be stored to disk in current checkpoint,
does not copy the set.
- * @param replaceFuture The sign which allows replacing pages from a
checkpoint by page replacer.
+ * @param pageIds Dirty page IDs in the segment that should be written at
a checkpoint or page replacement.
+ * @param checkpointProgress Progress of the current checkpoint at which
the object was created.
*/
- public CheckpointPages(Set<FullPageId> pages, CompletableFuture<?>
replaceFuture) {
- segmentPages = pages;
- allowToReplace = replaceFuture;
+ public CheckpointPages(Set<FullPageId> pageIds, CheckpointProgress
checkpointProgress) {
+ LOG.info(">>>>> CheckpointPages pageIds=" + pageIds);
Review Comment:
I think you forgot to remove this log message
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/PersistentPageMemory.java:
##########
@@ -1495,33 +1493,59 @@ private void resetDirtyPages() {
}
/**
- * Prepares a page removal for page replacement, if needed.
+ * Tries to replace the page.
*
- * @param fullPageId Candidate page full ID.
- * @param absPtr Absolute pointer of the page to evict.
- * @return {@code True} if it is ok to replace this page, {@code
false} if another page should be selected.
- * @throws IgniteInternalCheckedException If failed to write page to
the underlying store during eviction.
+ * <p>The replacement will be successful if the following conditions
are met:</p>
+ * <ul>
+ * <li>Page has acquired by checkpoint - nothing needs to be
done.</li>
Review Comment:
```suggestion
* <li>Page has been acquired by checkpoint - nothing needs to
be done.</li>
```
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/PersistentPageMemory.java:
##########
@@ -1495,33 +1493,59 @@ private void resetDirtyPages() {
}
/**
- * Prepares a page removal for page replacement, if needed.
+ * Tries to replace the page.
*
- * @param fullPageId Candidate page full ID.
- * @param absPtr Absolute pointer of the page to evict.
- * @return {@code True} if it is ok to replace this page, {@code
false} if another page should be selected.
- * @throws IgniteInternalCheckedException If failed to write page to
the underlying store during eviction.
+ * <p>The replacement will be successful if the following conditions
are met:</p>
+ * <ul>
+ * <li>Page has acquired by checkpoint - nothing needs to be
done.</li>
Review Comment:
What do you mean "acquired by checkpoint"? I don't really get it, maybe
that's even wrong. Please clarify the statement
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/PersistentPageMemory.java:
##########
@@ -1495,33 +1493,59 @@ private void resetDirtyPages() {
}
/**
- * Prepares a page removal for page replacement, if needed.
+ * Tries to replace the page.
*
- * @param fullPageId Candidate page full ID.
- * @param absPtr Absolute pointer of the page to evict.
- * @return {@code True} if it is ok to replace this page, {@code
false} if another page should be selected.
- * @throws IgniteInternalCheckedException If failed to write page to
the underlying store during eviction.
+ * <p>The replacement will be successful if the following conditions
are met:</p>
+ * <ul>
+ * <li>Page has acquired by checkpoint - nothing needs to be
done.</li>
+ * <li>Page is not dirty - just remove it from the loaded
pages.</li>
+ * <li>Page is dirty, there is a checkpoint in the process and the
following sub-conditions are met:</li>
+ * <ul>
+ * <li>Page belongs to current checkpoint.</li>
+ * <li>If the dirty page sorting phase is complete, otherwise
we wait for it. This is necessary so that we can safely
+ * create partition delta files in which the dirty page order
must be preserved.</li>
+ * <li>If the checkpoint dirty page writer has not started
writing the page or has already written it.</li>
+ * </ul>
+ * </ul>
+ *
+ * <p>It is expected that if the method returns {@code true}, it will
not be invoked again for the same page ID.</p>
+ *
+ * <p>If we intend to replace a page, it is important for us to block
the delta file fsync phase of the checkpoint to preserve data
+ * consistency. The phase should not start until all dirty pages are
written by the checkpoint writer, but for page replacement we
+ * must block it ourselves.</p>
+ *
+ * @param fullPageId Candidate page ID.
+ * @param absPtr Absolute pointer to the candidate page.
+ * @return {@code True} if the page replacement was successful,
otherwise need to try another one.
+ * @throws IgniteInternalCheckedException If any error occurred while
waiting for the dirty page sorting phase to complete at a
+ * checkpoint.
*/
public boolean tryToRemovePage(FullPageId fullPageId, long absPtr)
throws IgniteInternalCheckedException {
assert writeLock().isHeldByCurrentThread();
if (isAcquired(absPtr)) {
+ // Page is acquired by the checkpoint on the segment read lock
and will be written by it.
Review Comment:
Not necessarily, it might be acquired by any other thread for any other
reason. It may not even be a page that is it a checkpoint, your comment is not
accurate
--
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]
