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/12/22 15:41:18 UTC
[GitHub] [ignite] alex-plekhanov commented on a diff in pull request #10430: IGNITE-18198 Implement snapshots of caches with disk page compression.
alex-plekhanov commented on code in PR #10430:
URL: https://github.com/apache/ignite/pull/10430#discussion_r1055467359
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/wal/reader/StandaloneGridKernalContext.java:
##########
@@ -144,8 +144,29 @@ public class StandaloneGridKernalContext implements GridKernalContext {
/** Marshaller context implementation. */
private MarshallerContextImpl marshallerCtx;
+ /** */
+ @Nullable private CompressionProcessor cmpProc;
Review Comment:
Abbriviation `cmp` is usually used for `compare`, let's use `compressProc` here (in other files too)
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotPartitionsVerifyHandler.java:
##########
@@ -174,13 +181,32 @@ public SnapshotPartitionsVerifyHandler(GridCacheSharedContext<?, ?> cctx) {
snpEncrKeyProvider : null).createPageStore(getTypeByPartId(partId), part::toPath, val -> {})
) {
if (partId == INDEX_PARTITION) {
- checkPartitionsPageCrcSum(() -> pageStore, INDEX_PARTITION, FLAG_IDX);
+ if (punchHoleEnabled && meta.isGroupWithCompresion(grpId) && type() == SnapshotHandlerType.CREATE) {
+ checkPartitionsPageCrcSum(() -> pageStore, INDEX_PARTITION, FLAG_IDX, (id, buffer) -> {
+ if (PageIO.getCompressionType(buffer) == CompressionProcessor.UNCOMPRESSED_PAGE)
+ return;
+
+ int comprPageSz = PageIO.getCompressedSize(buffer);
+
+ if (comprPageSz < pageStore.getPageSize()) {
+ try {
+ pageStore.punchHole(id, comprPageSz);
+ }
+ catch (Exception ignored) {
+ // No-op
+ }
+ }
+ });
+ }
+ else if (!skipHash())
+ checkPartitionsPageCrcSum(() -> pageStore, INDEX_PARTITION, FLAG_IDX);
return null;
}
if (grpId == MetaStorage.METASTORAGE_CACHE_ID) {
- checkPartitionsPageCrcSum(() -> pageStore, partId, FLAG_DATA);
+ if (!skipHash())
Review Comment:
Why this check was removed for "skipHash"? As far as I understand "skipHash" mean values check should be skipped, but not physical page checks.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/verify/IdleVerifyUtility.java:
##########
@@ -89,6 +109,9 @@ public static void checkPartitionsPageCrcSum(
buf.clear();
pageStore.read(pageId, buf, true, true);
+
+ if (pagePostProcessor != null)
+ pagePostProcessor.accept(partId, buf);
Review Comment:
`partId` -> `pageId`
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotPartitionsVerifyHandler.java:
##########
@@ -191,6 +217,22 @@ public SnapshotPartitionsVerifyHandler(GridCacheSharedContext<?, ?> cctx) {
long pageAddr = GridUnsafe.bufferAddress(pageBuff);
+ if (PageIO.getCompressionType(pageBuff) != CompressionProcessor.UNCOMPRESSED_PAGE) {
+ int comprPageSz = PageIO.getCompressedSize(pageBuff);
+ long pageId = PageIO.getPageId(pageAddr);
+
+ snpCtx.compress().decompressPage(pageBuff, pageStore.getPageSize());
+
+ if (punchHoleEnabled && comprPageSz < pageStore.getPageSize()) {
+ try {
+ pageStore.punchHole(pageId, comprPageSz);
Review Comment:
Shouldn't we punch the hole only on snapshot create operation?
##########
modules/compress/src/test/java/org/apache/ignite/testsuites/IgnitePdsCompressionTestSuite.java:
##########
@@ -60,7 +64,15 @@ public static List<Class<?>> suite() {
suite.add(IgnitePdsCheckpointSimulationWithRealCpDisabledAndWalCompressionTest.class);
suite.add(WalCompactionAndPageCompressionTest.class);
+ // Snapshots
Review Comment:
Point at the end of line
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotPartitionsVerifyHandler.java:
##########
@@ -213,14 +255,21 @@ public SnapshotPartitionsVerifyHandler(GridCacheSharedContext<?, ?> cctx) {
// There is no `primary` partitions for snapshot.
PartitionKeyV2 key = new PartitionKeyV2(grpId, partId, grpName);
+ GridIterator<CacheDataRow> partRowIter;
+ if (punchHoleEnabled && meta.isGroupWithCompresion(grpId) && type() == SnapshotHandlerType.CREATE)
+ partRowIter = snpMgr.partitionRowIterator(snpCtx, grpName, partId, pageStore, true);
Review Comment:
This makes values hash calculation unskippable. If we don't want hash calculation we can use lightweight check `checkPartitionsPageCrcSum`. I propose to change logic in this method a little bit:
- undo changes in partitionRowIterator, this iterator should only be called on restore operation (skipHash == false)
- add `checkPartitionsPageCrcSum` with hole punching for create operation (at least when group is compessed and punch holes is enabled, but perhaps always, I don't know why we do CRC check for index partition and skip this check for regular partition)
- undo dedicated hole punching for metadata page
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotFutureTask.java:
##########
@@ -943,12 +945,28 @@ protected void init() throws IOException {
if (!store.read(pageId, locBuf, true))
return;
- locBuf.flip();
+ locBuf.limit(locBuf.capacity());
+ locBuf.position(0);
writePage0(pageId, locBuf);
}
else {
// Direct buffer is needs to be written, associated checkpoint not finished yet.
+ if (PageIO.getCompressionType(GridUnsafe.bufferAddress(buf)) != CompressionProcessor.UNCOMPRESSED_PAGE) {
+ final ByteBuffer locBuf = locBuff.get();
+
+ assert locBuf.capacity() == store.getPageSize();
+
+ locBuf.clear();
+
+ GridUnsafe.copyOffheapOffheap(GridUnsafe.bufferAddress(buf), GridUnsafe.bufferAddress(locBuf), buf.limit());
+
+ locBuf.limit(locBuf.capacity());
+ locBuf.position(0);
+
+ buf = locBuf;
+ }
Review Comment:
Why can't we just set and restore `limit` for `buf`?
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotFutureTask.java:
##########
@@ -943,12 +945,28 @@ protected void init() throws IOException {
if (!store.read(pageId, locBuf, true))
return;
- locBuf.flip();
+ locBuf.limit(locBuf.capacity());
+ locBuf.position(0);
Review Comment:
`locBuf.clear()`?
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteSnapshotManager.java:
##########
@@ -1458,6 +1465,38 @@ public IgniteInternalFuture<SnapshotPartitionsVerifyTaskResult> checkSnapshot(
return;
}
+ if (meta.hasCompressedGroups() && grpIds.keySet().stream().anyMatch(meta::isGroupWithCompresion)) {
+ try {
+ File dbPath = kctx0.pdsFolderResolver().resolveFolders().persistentStoreRootPath();
+ int pageSize = kctx0.config().getDataStorageConfiguration().getPageSize();
+
+ kctx0.compress().checkPageCompressionSupported(dbPath.toPath(), pageSize);
Review Comment:
This method also checks that FS is hole-punchable, but to check snapshot we can proceed even on FS without this ability, so we can just check compression processor is enabled (method `checkPageCompressionSupported()` without parameters)
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteSnapshotManager.java:
##########
@@ -1458,6 +1465,38 @@ public IgniteInternalFuture<SnapshotPartitionsVerifyTaskResult> checkSnapshot(
return;
}
+ if (meta.hasCompressedGroups() && grpIds.keySet().stream().anyMatch(meta::isGroupWithCompresion)) {
+ try {
+ File dbPath = kctx0.pdsFolderResolver().resolveFolders().persistentStoreRootPath();
+ int pageSize = kctx0.config().getDataStorageConfiguration().getPageSize();
+
+ kctx0.compress().checkPageCompressionSupported(dbPath.toPath(), pageSize);
+ }
+ catch (IgniteCheckedException e) {
+ String msg;
+ if (grpIds.isEmpty()) {
+ msg = "Snapshot '" + meta.snapshotName() + "' contains compressed cache groups while " +
Review Comment:
Unreachable code
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotRestoreProcess.java:
##########
@@ -688,6 +689,28 @@ private SnapshotRestoreContext prepareContext(
if (!F.isEmpty(req.groups()) && !req.groups().contains(grpName))
continue;
+ if (!skipCompressCheck && meta.isGroupWithCompresion(CU.cacheId(grpName))) {
+ try {
+ File path = ctx.pdsFolderResolver().resolveFolders().persistentStoreRootPath();
+
+ ctx.compress().checkPageCompressionSupported(path.toPath(), meta.pageSize());
Review Comment:
This method also checks that FS is hole-punchable, but to restore we can proceed even on FS without this ability since in any way we don't punch the holes during restore. So we can just check compression processor is enabled (method `checkPageCompressionSupported()` without parameters).
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotPartitionsVerifyHandler.java:
##########
@@ -174,13 +181,32 @@ public SnapshotPartitionsVerifyHandler(GridCacheSharedContext<?, ?> cctx) {
snpEncrKeyProvider : null).createPageStore(getTypeByPartId(partId), part::toPath, val -> {})
) {
if (partId == INDEX_PARTITION) {
- checkPartitionsPageCrcSum(() -> pageStore, INDEX_PARTITION, FLAG_IDX);
+ if (punchHoleEnabled && meta.isGroupWithCompresion(grpId) && type() == SnapshotHandlerType.CREATE) {
+ checkPartitionsPageCrcSum(() -> pageStore, INDEX_PARTITION, FLAG_IDX, (id, buffer) -> {
+ if (PageIO.getCompressionType(buffer) == CompressionProcessor.UNCOMPRESSED_PAGE)
+ return;
+
+ int comprPageSz = PageIO.getCompressedSize(buffer);
+
+ if (comprPageSz < pageStore.getPageSize()) {
+ try {
+ pageStore.punchHole(id, comprPageSz);
+ }
+ catch (Exception ignored) {
+ // No-op
+ }
+ }
+ });
+ }
+ else if (!skipHash())
Review Comment:
Why this check was removed for "skipHash"? As far as I understand "skipHash" mean values check should be skipped, but not physical page checks.
--
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