Copilot commented on code in PR #7023:
URL: https://github.com/apache/ignite-3/pull/7023#discussion_r2545266157
##########
modules/storage-page-memory/src/test/java/org/apache/ignite/internal/storage/pagememory/PersistentPageMemoryNoLoadTest.java:
##########
@@ -450,6 +453,111 @@ void testPageReplacement(@WorkDirectory Path workDir)
throws Exception {
}
}
+ /**
+ * Tests that {@link PersistentPageMemory#acquirePage(int, long)} works
correctly when multiple threads try to acquire the same page
+ * using different {@code padeId} values, assuming that one of the threads
simply has in invalid identifier from some obsolete source.
+ */
+ @Test
+ void testAcquireRace(@WorkDirectory Path workDir) throws Exception {
+ int pages = 100;
+
+ // Step 1. Start the region, allocate a number of pages, checkpoint
them to the storage, stop the region.
+ FilePageStoreManager filePageStoreManager =
createFilePageStoreManager(workDir);
+ PartitionMetaManager partitionMetaManager = new
PartitionMetaManager(ioRegistry, PAGE_SIZE, StoragePartitionMeta.FACTORY);
+ Collection<DataRegion<PersistentPageMemory>> dataRegions = new
ArrayList<>();
+
+ CheckpointManager checkpointManager = createCheckpointManager(
+ CheckpointConfiguration.builder().build(),
+ filePageStoreManager,
+ partitionMetaManager,
+ dataRegions
+ );
+
+ int systemPageSize = PAGE_SIZE + PAGE_OVERHEAD;
+
+ dataRegionSize = 1024 * systemPageSize;
+
+ PersistentPageMemory pageMemory = createPageMemory(
+ new long[]{dataRegionSize},
+ 128 * systemPageSize,
+ filePageStoreManager,
+ checkpointManager,
+ shouldNotHappenFlushDirtyPageForReplacement()
+ );
+
+ dataRegions.add(() -> pageMemory);
+
+ filePageStoreManager.start();
+ checkpointManager.start();
+ pageMemory.start();
+
+ try {
+ initGroupFilePageStores(filePageStoreManager,
partitionMetaManager, checkpointManager, pageMemory);
+
+ checkpointManager.checkpointTimeoutLock().checkpointReadLock();
+
+ try {
+ for (int i = 0; i < pages; i++) {
+ createDirtyPage(pageMemory);
+ }
+ } finally {
+
checkpointManager.checkpointTimeoutLock().checkpointReadUnlock();
+ }
+
+ CompletableFuture<Void> checkpointFuture = checkpointManager
+ .forceCheckpoint("before-stopping-in-test")
+ .futureFor(FINISHED);
+
+ assertThat(checkpointFuture, willCompleteSuccessfully());
+ } finally {
+ closeAll(
+ () -> pageMemory.stop(true),
+ checkpointManager::stop,
+ filePageStoreManager::stop
+ );
+ }
+
+ // Step 2. Start a new region over the same persistence. The goal here
is to test "acquirePage" that will actually read data from
+ // the storage instead of hitting the cached page.
+ PersistentPageMemory pageMemory2 = createPageMemory(
+ new long[]{dataRegionSize},
+ 128 * systemPageSize,
+ filePageStoreManager,
+ checkpointManager,
+ shouldNotHappenFlushDirtyPageForReplacement()
+ );
+
+ filePageStoreManager.start();
+ checkpointManager.start();
+ pageMemory2.start();
+
+ try {
+ initGroupFilePageStores(filePageStoreManager,
partitionMetaManager, checkpointManager, pageMemory2);
+
+ for (int i = 0; i < pages; i++) {
+ // We skip meta page, that's why we add 1 to i.
+ long fakePageId = PageIdUtils.pageId(PARTITION_ID,
PageIdAllocator.FLAG_AUX, i + 1);
+ long realPageId = PageIdUtils.pageId(PARTITION_ID,
PageIdAllocator.FLAG_DATA, i + 1);
+
+ // Step 3. Run the race for all pages in the partition.
+ IgniteTestUtils.runRace(
+ () -> pageMemory2.acquirePage(GRP_ID, fakePageId),
+ () -> {
+ long page = pageMemory2.acquirePage(GRP_ID,
realPageId);
+
+ assertNotEquals(0L, pageMemory2.readLock(GRP_ID,
realPageId, page));
Review Comment:
Resource leak: The read lock acquired at line 548 is never released. After
verifying the lock is non-zero, you should call `pageMemory2.readUnlock(GRP_ID,
realPageId, page)` to release the lock. Consider wrapping in a try-finally
block to ensure proper cleanup.
```suggestion
long lock = pageMemory2.readLock(GRP_ID,
realPageId, page);
try {
assertNotEquals(0L, lock);
} finally {
if (lock != 0L) {
pageMemory2.readUnlock(GRP_ID,
realPageId, page);
}
}
```
##########
modules/storage-page-memory/src/test/java/org/apache/ignite/internal/storage/pagememory/PersistentPageMemoryNoLoadTest.java:
##########
@@ -450,6 +453,111 @@ void testPageReplacement(@WorkDirectory Path workDir)
throws Exception {
}
}
+ /**
+ * Tests that {@link PersistentPageMemory#acquirePage(int, long)} works
correctly when multiple threads try to acquire the same page
+ * using different {@code padeId} values, assuming that one of the threads
simply has in invalid identifier from some obsolete source.
Review Comment:
Spelling errors: "padeId" should be "pageId" and "in invalid" should be "an
invalid".
```suggestion
* using different {@code pageId} values, assuming that one of the
threads simply has an invalid identifier from some obsolete source.
```
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/PersistentPageMemory.java:
##########
@@ -732,28 +725,21 @@ grpId, hexLong(pageId)
partGen
);
- long pageAddr = absPtr + PAGE_OVERHEAD;
-
- if (!restore) {
- delayedPageReplacementTracker.waitUnlock(fullId);
+ delayedPageReplacementTracker.waitUnlock(fullId);
- readPageFromStore = true;
- } else {
- zeroMemory(absPtr + PAGE_OVERHEAD, pageSize());
+ readPageFromStore = true;
- // Must init page ID in order to ensure RWLock tag
consistency.
- setPageId(pageAddr, pageId);
- }
+ // Mare page header as invalid. We have not yet read the real
value of "pageId" from the page, thus the state of "rwLock"
Review Comment:
Spelling error: "Mare" should be "Mark".
```suggestion
// Mark page header as invalid. We have not yet read the
real value of "pageId" from the page, thus the state of "rwLock"
```
##########
modules/storage-page-memory/src/test/java/org/apache/ignite/internal/storage/pagememory/PersistentPageMemoryNoLoadTest.java:
##########
@@ -450,6 +453,111 @@ void testPageReplacement(@WorkDirectory Path workDir)
throws Exception {
}
}
+ /**
+ * Tests that {@link PersistentPageMemory#acquirePage(int, long)} works
correctly when multiple threads try to acquire the same page
+ * using different {@code padeId} values, assuming that one of the threads
simply has in invalid identifier from some obsolete source.
+ */
+ @Test
+ void testAcquireRace(@WorkDirectory Path workDir) throws Exception {
+ int pages = 100;
+
+ // Step 1. Start the region, allocate a number of pages, checkpoint
them to the storage, stop the region.
+ FilePageStoreManager filePageStoreManager =
createFilePageStoreManager(workDir);
+ PartitionMetaManager partitionMetaManager = new
PartitionMetaManager(ioRegistry, PAGE_SIZE, StoragePartitionMeta.FACTORY);
+ Collection<DataRegion<PersistentPageMemory>> dataRegions = new
ArrayList<>();
+
+ CheckpointManager checkpointManager = createCheckpointManager(
+ CheckpointConfiguration.builder().build(),
+ filePageStoreManager,
+ partitionMetaManager,
+ dataRegions
+ );
+
+ int systemPageSize = PAGE_SIZE + PAGE_OVERHEAD;
+
+ dataRegionSize = 1024 * systemPageSize;
+
+ PersistentPageMemory pageMemory = createPageMemory(
+ new long[]{dataRegionSize},
+ 128 * systemPageSize,
+ filePageStoreManager,
+ checkpointManager,
+ shouldNotHappenFlushDirtyPageForReplacement()
+ );
+
+ dataRegions.add(() -> pageMemory);
+
+ filePageStoreManager.start();
+ checkpointManager.start();
+ pageMemory.start();
+
+ try {
+ initGroupFilePageStores(filePageStoreManager,
partitionMetaManager, checkpointManager, pageMemory);
+
+ checkpointManager.checkpointTimeoutLock().checkpointReadLock();
+
+ try {
+ for (int i = 0; i < pages; i++) {
+ createDirtyPage(pageMemory);
+ }
+ } finally {
+
checkpointManager.checkpointTimeoutLock().checkpointReadUnlock();
+ }
+
+ CompletableFuture<Void> checkpointFuture = checkpointManager
+ .forceCheckpoint("before-stopping-in-test")
+ .futureFor(FINISHED);
+
+ assertThat(checkpointFuture, willCompleteSuccessfully());
+ } finally {
+ closeAll(
+ () -> pageMemory.stop(true),
+ checkpointManager::stop,
+ filePageStoreManager::stop
+ );
+ }
+
+ // Step 2. Start a new region over the same persistence. The goal here
is to test "acquirePage" that will actually read data from
+ // the storage instead of hitting the cached page.
+ PersistentPageMemory pageMemory2 = createPageMemory(
+ new long[]{dataRegionSize},
+ 128 * systemPageSize,
+ filePageStoreManager,
+ checkpointManager,
+ shouldNotHappenFlushDirtyPageForReplacement()
+ );
+
+ filePageStoreManager.start();
+ checkpointManager.start();
+ pageMemory2.start();
+
+ try {
+ initGroupFilePageStores(filePageStoreManager,
partitionMetaManager, checkpointManager, pageMemory2);
+
+ for (int i = 0; i < pages; i++) {
+ // We skip meta page, that's why we add 1 to i.
+ long fakePageId = PageIdUtils.pageId(PARTITION_ID,
PageIdAllocator.FLAG_AUX, i + 1);
+ long realPageId = PageIdUtils.pageId(PARTITION_ID,
PageIdAllocator.FLAG_DATA, i + 1);
+
+ // Step 3. Run the race for all pages in the partition.
+ IgniteTestUtils.runRace(
+ () -> pageMemory2.acquirePage(GRP_ID, fakePageId),
+ () -> {
+ long page = pageMemory2.acquirePage(GRP_ID,
realPageId);
+
+ assertNotEquals(0L, pageMemory2.readLock(GRP_ID,
realPageId, page));
Review Comment:
Resource leak: The pages acquired via `acquirePage` at lines 544 and 546 are
never released. After using the acquired pages, you should call
`pageMemory2.releasePage(GRP_ID, pageId, page)` to properly release them.
Consider wrapping the acquisition in a try-finally block to ensure pages are
released even if assertions fail.
```suggestion
() -> {
long page = pageMemory2.acquirePage(GRP_ID,
fakePageId);
try {
// No operation, just acquire and release.
} finally {
pageMemory2.releasePage(GRP_ID, fakePageId,
page);
}
},
() -> {
long page = pageMemory2.acquirePage(GRP_ID,
realPageId);
try {
assertNotEquals(0L,
pageMemory2.readLock(GRP_ID, realPageId, page));
} finally {
pageMemory2.releasePage(GRP_ID, realPageId,
page);
}
```
--
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]
