tkalkirill commented on code in PR #4437:
URL: https://github.com/apache/ignite-3/pull/4437#discussion_r1775189123
##########
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:
fix it
##########
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:
fix it
--
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]
