You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2022/06/28 06:42:44 UTC
[GitHub] [ignite-3] tkalkirill opened a new pull request, #901: IGNITE-17230 Support splt-file page store
tkalkirill opened a new pull request, #901:
URL: https://github.com/apache/ignite-3/pull/901
https://issues.apache.org/jira/browse/IGNITE-17230
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #901: IGNITE-17230 Support splt-file page store
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #901:
URL: https://github.com/apache/ignite-3/pull/901#discussion_r932925880
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/PersistentPageMemory.java:
##########
@@ -1295,6 +1295,9 @@ public class Segment extends ReentrantReadWriteLock {
/** Padding to read from word beginning. */
private static final int ACQUIRED_PAGES_PADDING = 4;
+ /** Page memory. */
+ private final PersistentPageMemory pageMemory;
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #901: IGNITE-17230 Support splt-file page store
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #901:
URL: https://github.com/apache/ignite-3/pull/901#discussion_r932930261
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/checkpoint/CheckpointManager.java:
##########
@@ -221,4 +233,56 @@ static boolean safeToUpdateAllPageMemories(Collection<? extends DataRegion<Persi
return true;
}
+
+ /**
+ * Writes a page to delta file page store.
+ *
+ * <p>Must be used at breakpoint and page replacement.
+ *
+ * @param pageMemory Page memory.
+ * @param pageId Page ID.
+ * @param pageBuf Page buffer to write from.
+ * @param calculateCrc If {@code false} crc calculation will be forcibly skipped.
+ * @throws IgniteInternalCheckedException If page writing failed (IO error occurred).
+ */
+ public void writePageToDeltaFilePageStore(
+ PersistentPageMemory pageMemory,
+ FullPageId pageId,
+ ByteBuffer pageBuf,
+ boolean calculateCrc
+ ) throws IgniteInternalCheckedException {
+ FilePageStore filePageStore = filePageStoreManager.getStore(pageId.groupId(), pageId.partitionId());
+
+ CheckpointProgress lastCheckpointProgress = lastCheckpointProgress();
+
+ assert lastCheckpointProgress != null : "Checkpoint has not happened yet";
+ assert lastCheckpointProgress.inProgress() : "Checkpoint must be in progress";
+
+ CheckpointDirtyPages pagesToWrite = lastCheckpointProgress.pagesToWrite();
+
+ assert pagesToWrite != null : "Dirty pages must be sorted out";
+
+ CompletableFuture<DeltaFilePageStoreIo> deltaFilePageStoreFuture = filePageStore.getOrCreateNewDeltaFile(
Review Comment:
Are you suggesting just return **DeltaFilePageStoreIo**, **join** execute inside and not outside??
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] SammyVimes commented on a diff in pull request #901: IGNITE-17230 Support splt-file page store
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on code in PR #901:
URL: https://github.com/apache/ignite-3/pull/901#discussion_r932265975
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/checkpoint/CheckpointWorkflow.java:
##########
@@ -219,15 +226,17 @@ public Checkpoint markCheckpointBegin(
*/
public void markCheckpointEnd(Checkpoint chp) throws IgniteInternalCheckedException {
synchronized (this) {
- chp.progress.clearCounters();
-
for (DataRegion<PersistentPageMemory> dataRegion : dataRegions) {
dataRegion.pageMemory().finishCheckpoint();
}
}
if (chp.hasDelta()) {
checkpointMarkersStorage.onCheckpointEnd(chp.progress.id());
+
+ chp.progress.pagesToWrite(null);
Review Comment:
Do I understand correctly that if `hasDelta == false`, then dirty pages will be null, right? So we don't need to set null if hasDelta == false
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/checkpoint/CheckpointManager.java:
##########
@@ -221,4 +233,56 @@ static boolean safeToUpdateAllPageMemories(Collection<? extends DataRegion<Persi
return true;
}
+
+ /**
+ * Writes a page to delta file page store.
+ *
+ * <p>Must be used at breakpoint and page replacement.
+ *
+ * @param pageMemory Page memory.
+ * @param pageId Page ID.
+ * @param pageBuf Page buffer to write from.
+ * @param calculateCrc If {@code false} crc calculation will be forcibly skipped.
+ * @throws IgniteInternalCheckedException If page writing failed (IO error occurred).
+ */
+ public void writePageToDeltaFilePageStore(
+ PersistentPageMemory pageMemory,
+ FullPageId pageId,
+ ByteBuffer pageBuf,
+ boolean calculateCrc
+ ) throws IgniteInternalCheckedException {
+ FilePageStore filePageStore = filePageStoreManager.getStore(pageId.groupId(), pageId.partitionId());
+
+ CheckpointProgress lastCheckpointProgress = lastCheckpointProgress();
+
+ assert lastCheckpointProgress != null : "Checkpoint has not happened yet";
+ assert lastCheckpointProgress.inProgress() : "Checkpoint must be in progress";
+
+ CheckpointDirtyPages pagesToWrite = lastCheckpointProgress.pagesToWrite();
+
+ assert pagesToWrite != null : "Dirty pages must be sorted out";
+
+ CompletableFuture<DeltaFilePageStoreIo> deltaFilePageStoreFuture = filePageStore.getOrCreateNewDeltaFile(
Review Comment:
Do we need a future here? I see that we mostly use join on these futures
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/PersistentPageMemory.java:
##########
@@ -1295,6 +1295,9 @@ public class Segment extends ReentrantReadWriteLock {
/** Padding to read from word beginning. */
private static final int ACQUIRED_PAGES_PADDING = 4;
+ /** Page memory. */
+ private final PersistentPageMemory pageMemory;
Review Comment:
Segment class is inner, so you can just use `PersistentPageMemory.this`
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] SammyVimes commented on a diff in pull request #901: IGNITE-17230 Support splt-file page store
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on code in PR #901:
URL: https://github.com/apache/ignite-3/pull/901#discussion_r932933053
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/checkpoint/CheckpointWorkflow.java:
##########
@@ -219,15 +226,17 @@ public Checkpoint markCheckpointBegin(
*/
public void markCheckpointEnd(Checkpoint chp) throws IgniteInternalCheckedException {
synchronized (this) {
- chp.progress.clearCounters();
-
for (DataRegion<PersistentPageMemory> dataRegion : dataRegions) {
dataRegion.pageMemory().finishCheckpoint();
}
}
if (chp.hasDelta()) {
checkpointMarkersStorage.onCheckpointEnd(chp.progress.id());
+
+ chp.progress.pagesToWrite(null);
Review Comment:
No, I was just curious, current approach is good
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] SammyVimes merged pull request #901: IGNITE-17230 Support splt-file page store
Posted by GitBox <gi...@apache.org>.
SammyVimes merged PR #901:
URL: https://github.com/apache/ignite-3/pull/901
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #901: IGNITE-17230 Support splt-file page store
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #901:
URL: https://github.com/apache/ignite-3/pull/901#discussion_r932928021
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/checkpoint/CheckpointWorkflow.java:
##########
@@ -219,15 +226,17 @@ public Checkpoint markCheckpointBegin(
*/
public void markCheckpointEnd(Checkpoint chp) throws IgniteInternalCheckedException {
synchronized (this) {
- chp.progress.clearCounters();
-
for (DataRegion<PersistentPageMemory> dataRegion : dataRegions) {
dataRegion.pageMemory().finishCheckpoint();
}
}
if (chp.hasDelta()) {
checkpointMarkersStorage.onCheckpointEnd(chp.progress.id());
+
+ chp.progress.pagesToWrite(null);
Review Comment:
If **hasDelta == false** then we set **CheckpointDirtyPages#EMPTY**, i can change to **null**, WDYT?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #901: IGNITE-17230 Support splt-file page store
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #901:
URL: https://github.com/apache/ignite-3/pull/901#discussion_r933003476
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/checkpoint/CheckpointManager.java:
##########
@@ -221,4 +233,56 @@ static boolean safeToUpdateAllPageMemories(Collection<? extends DataRegion<Persi
return true;
}
+
+ /**
+ * Writes a page to delta file page store.
+ *
+ * <p>Must be used at breakpoint and page replacement.
+ *
+ * @param pageMemory Page memory.
+ * @param pageId Page ID.
+ * @param pageBuf Page buffer to write from.
+ * @param calculateCrc If {@code false} crc calculation will be forcibly skipped.
+ * @throws IgniteInternalCheckedException If page writing failed (IO error occurred).
+ */
+ public void writePageToDeltaFilePageStore(
+ PersistentPageMemory pageMemory,
+ FullPageId pageId,
+ ByteBuffer pageBuf,
+ boolean calculateCrc
+ ) throws IgniteInternalCheckedException {
+ FilePageStore filePageStore = filePageStoreManager.getStore(pageId.groupId(), pageId.partitionId());
+
+ CheckpointProgress lastCheckpointProgress = lastCheckpointProgress();
+
+ assert lastCheckpointProgress != null : "Checkpoint has not happened yet";
+ assert lastCheckpointProgress.inProgress() : "Checkpoint must be in progress";
+
+ CheckpointDirtyPages pagesToWrite = lastCheckpointProgress.pagesToWrite();
+
+ assert pagesToWrite != null : "Dirty pages must be sorted out";
+
+ CompletableFuture<DeltaFilePageStoreIo> deltaFilePageStoreFuture = filePageStore.getOrCreateNewDeltaFile(
Review Comment:
Discussed personally, leave as is.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org