ibessonov commented on code in PR #6343:
URL: https://github.com/apache/ignite-3/pull/6343#discussion_r2282642211
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/store/FilePageStore.java:
##########
@@ -149,14 +155,38 @@ public int pages() {
}
/**
- * Sets the page count.
+ * Init the page count.
*
* @param pageCount New page count.
*/
public void pages(int pageCount) {
assert pageCount >= 0 : pageCount;
this.pageCount = pageCount;
+ this.checkpointedPageCount = pageCount;
+ this.persistedPageCount = pageCount;
+ }
+
+ /** Returns number of pages that were successfully checkpointed. */
+ public int checkpointedPageCount() {
+ return checkpointedPageCount;
+ }
+
+ /**
+ * Sets number of pages that were successfully checkpointed.
+ *
+ * @param checkpointedPageCount New checkpointed page count.
+ */
+ public void checkpointedPageCount(int checkpointedPageCount) {
Review Comment:
I wonder if we can avoid this setter...
probably not
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/store/FilePageStore.java:
##########
@@ -191,9 +221,16 @@ public void read(long pageId, ByteBuffer pageBuf, boolean
keepCrc) throws Ignite
@Override
public void write(long pageId, ByteBuffer pageBuf) throws
IgniteInternalCheckedException {
- assert pageIndex(pageId) <= pageCount : "pageIdx=" + pageIndex(pageId)
+ ", pageCount=" + pageCount;
+ int pageIndex = pageIndex(pageId);
+
+ assert pageIndex <= pageCount : "pageIdx=" + pageIndex + ",
pageCount=" + pageCount;
filePageStoreIo.write(pageId, pageBuf);
+
+ // Indexes start from 0.
+ if (pageIndex >= persistedPageCount) {
+ persistedPageCount = pageIndex + 1;
+ }
Review Comment:
Please explain what's going on. Why do you need the max index of persisted
page. You should not need it. And what about concurrency?
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/checkpoint/CheckpointDirtyPages.java:
##########
@@ -177,6 +177,22 @@ public PersistentPageMemory pageMemory() {
public int size() {
return toPosition - fromPosition;
}
+
+ /** Returns number of modified (not newly allocated) pages. */
+ int modifiedPages(int persistedPages) {
+ FullPageId[] dirtyPages =
dirtyPagesAndPartitions.get(this.regionIndex).dirtyPages;
+ int partitionId = dirtyPages[fromPosition].partitionId();
+ int groupId = dirtyPages[fromPosition].groupId();
+
+ FullPageId endPageId = new FullPageId(pageId(partitionId, (byte)
0, persistedPages - 1), groupId);
+
+ int index = binarySearch(dirtyPages, fromPosition, toPosition,
endPageId, DIRTY_PAGE_COMPARATOR);
+
+ // If exact index is not found, binary search returns a bitwise
complement to a potential insertion point.
Review Comment:
You say "bitwise complement" but you never use it. This comment might be
confusing to the reader
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/checkpoint/CheckpointDirtyPages.java:
##########
@@ -177,6 +177,22 @@ public PersistentPageMemory pageMemory() {
public int size() {
return toPosition - fromPosition;
}
+
+ /** Returns number of modified (not newly allocated) pages. */
Review Comment:
This javadoc is lacking information. What is a `persistedPages`? Why is it
here? What if partition doesn't have dirty pages and only meta page has been
updated? Is it true that `fromPosition == persistedPages`?
We should improve this method. I understand what it does, but I'm not
convinced that it's properly implemented and used. You should not rely on
reading partition and group IDs from the array, that doesn't look good to me
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/store/FilePageStore.java:
##########
@@ -191,9 +221,16 @@ public void read(long pageId, ByteBuffer pageBuf, boolean
keepCrc) throws Ignite
@Override
public void write(long pageId, ByteBuffer pageBuf) throws
IgniteInternalCheckedException {
- assert pageIndex(pageId) <= pageCount : "pageIdx=" + pageIndex(pageId)
+ ", pageCount=" + pageCount;
+ int pageIndex = pageIndex(pageId);
+
+ assert pageIndex <= pageCount : "pageIdx=" + pageIndex + ",
pageCount=" + pageCount;
Review Comment:
Should it be strictly smaller?
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/checkpoint/Checkpointer.java:
##########
@@ -610,9 +610,20 @@ private void fsyncDeltaFile(
currentCheckpointProgress.blockPartitionDestruction(partitionId);
try {
- fsyncDeltaFilePageStoreOnCheckpointThread(filePageStore,
pagesWritten);
+ fsyncDeltaFilePageStoreOnCheckpointThread(filePageStore);
+
+ blockingSectionBegin();
+ try {
+ filePageStore.sync();
+ } finally {
+ blockingSectionEnd();
+ }
Review Comment:
I believe that adding new method like `fsync???OnCheckpointThread` would
make this code easier. Before your changes it looked better, `addAndGet` was
encapsulated
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/checkpoint/CheckpointDirtyPages.java:
##########
@@ -177,6 +177,22 @@ public PersistentPageMemory pageMemory() {
public int size() {
return toPosition - fromPosition;
}
+
+ /** Returns number of modified (not newly allocated) pages. */
+ int modifiedPages(int persistedPages) {
+ FullPageId[] dirtyPages =
dirtyPagesAndPartitions.get(this.regionIndex).dirtyPages;
+ int partitionId = dirtyPages[fromPosition].partitionId();
+ int groupId = dirtyPages[fromPosition].groupId();
+
+ FullPageId endPageId = new FullPageId(pageId(partitionId, (byte)
0, persistedPages - 1), groupId);
+
+ int index = binarySearch(dirtyPages, fromPosition, toPosition,
endPageId, DIRTY_PAGE_COMPARATOR);
+
+ // If exact index is not found, binary search returns a bitwise
complement to a potential insertion point.
+ return index >= 0
+ ? index - fromPosition + 1
+ : -1 * index - fromPosition - 1;
Review Comment:
`-1 * index` is the same as `-index`
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/store/FilePageStore.java:
##########
@@ -149,14 +155,38 @@ public int pages() {
}
/**
- * Sets the page count.
+ * Init the page count.
Review Comment:
```suggestion
* Initializes the page count.
```
--
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]
