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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
