timoninmaxim commented on code in PR #11346:
URL: https://github.com/apache/ignite/pull/11346#discussion_r1601875869
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotMetadataVerificationTask.java:
##########
@@ -88,25 +96,74 @@ public
MetadataVerificationJob(SnapshotMetadataVerificationTaskArg arg) {
}
/** {@inheritDoc} */
- @Override public List<SnapshotMetadata> execute() throws
IgniteException {
+ @Override public List<SnapshotMetadata> execute() {
IgniteSnapshotManager snpMgr =
ignite.context().cache().context().snapshotMgr();
- List<SnapshotMetadata> snpMeta =
snpMgr.readSnapshotMetadatas(arg.snapshotName(), arg.snapshotPath());
Review Comment:
Let's revert variable rename.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotMetadataVerificationTask.java:
##########
@@ -88,25 +96,74 @@ public
MetadataVerificationJob(SnapshotMetadataVerificationTaskArg arg) {
}
/** {@inheritDoc} */
- @Override public List<SnapshotMetadata> execute() throws
IgniteException {
+ @Override public List<SnapshotMetadata> execute() {
IgniteSnapshotManager snpMgr =
ignite.context().cache().context().snapshotMgr();
- List<SnapshotMetadata> snpMeta =
snpMgr.readSnapshotMetadatas(arg.snapshotName(), arg.snapshotPath());
+ List<SnapshotMetadata> metas =
snpMgr.readSnapshotMetadatas(arg.snapshotName(), arg.snapshotPath());
+
+ for (SnapshotMetadata meta : metas)
+ checkMeta(meta,
ignite.context().config().getEncryptionSpi().masterKeyDigest());
Review Comment:
Looks like the `masterKeyDigest()` can invoked within the `checkMeta` method.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotMetadataVerificationTask.java:
##########
@@ -88,25 +96,74 @@ public
MetadataVerificationJob(SnapshotMetadataVerificationTaskArg arg) {
}
/** {@inheritDoc} */
- @Override public List<SnapshotMetadata> execute() throws
IgniteException {
+ @Override public List<SnapshotMetadata> execute() {
IgniteSnapshotManager snpMgr =
ignite.context().cache().context().snapshotMgr();
- List<SnapshotMetadata> snpMeta =
snpMgr.readSnapshotMetadatas(arg.snapshotName(), arg.snapshotPath());
+ List<SnapshotMetadata> metas =
snpMgr.readSnapshotMetadatas(arg.snapshotName(), arg.snapshotPath());
+
+ for (SnapshotMetadata meta : metas)
+ checkMeta(meta,
ignite.context().config().getEncryptionSpi().masterKeyDigest());
if (arg.incrementIndex() > 0) {
- List<SnapshotMetadata> metas = snpMeta.stream()
+ List<SnapshotMetadata> locNodeMetas = metas.stream()
.filter(m ->
m.consistentId().equals(ignite.localNode().consistentId()))
.collect(Collectors.toList());
- if (metas.size() != 1) {
- throw new IgniteException("Failed to find snapshot
metafile [metas=" + metas +
- ", snpName=" + arg.snapshotName() + ", snpPath=" +
arg.snapshotPath() + ']');
+ if (locNodeMetas.size() != 1) {
+ throw new IgniteException("Failed to find single snapshot
metafile for local node [locNodeId="
Review Comment:
Let's write here that incremental snapshots requires single meta file per
node, because incremental snapshots don't support restoring on different
topology.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotMetadataVerificationTaskArg.java:
##########
@@ -76,18 +81,27 @@ public int incrementIndex() {
return incIdx;
}
+ /**
+ * @return Cache group ids.
+ */
+ public Collection<Integer> grpIds() {
+ return grpIds;
+ }
+
/** {@inheritDoc} */
@Override protected void writeExternalData(ObjectOutput out) throws
IOException {
U.writeString(out, snpName);
U.writeString(out, snpPath);
out.writeInt(incIdx);
+ U.writeCollection(out, grpIds);
Review Comment:
Should change protocol version to V2.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotMetadataVerificationTask.java:
##########
@@ -88,25 +96,74 @@ public
MetadataVerificationJob(SnapshotMetadataVerificationTaskArg arg) {
}
/** {@inheritDoc} */
- @Override public List<SnapshotMetadata> execute() throws
IgniteException {
+ @Override public List<SnapshotMetadata> execute() {
IgniteSnapshotManager snpMgr =
ignite.context().cache().context().snapshotMgr();
- List<SnapshotMetadata> snpMeta =
snpMgr.readSnapshotMetadatas(arg.snapshotName(), arg.snapshotPath());
+ List<SnapshotMetadata> metas =
snpMgr.readSnapshotMetadatas(arg.snapshotName(), arg.snapshotPath());
+
+ for (SnapshotMetadata meta : metas)
+ checkMeta(meta,
ignite.context().config().getEncryptionSpi().masterKeyDigest());
if (arg.incrementIndex() > 0) {
- List<SnapshotMetadata> metas = snpMeta.stream()
Review Comment:
Let's revert the name of the variable. You actually start using `metas` name
for another variable. It's a little bit misleading.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotRestoreProcess.java:
##########
@@ -402,12 +402,8 @@ public IgniteFutureImpl<Void> start(
return;
}
- if (!reqGrpIds.isEmpty()) {
- finishProcess(fut0.rqId, new
IllegalArgumentException(OP_REJECT_MSG + "Cache group(s) was not " +
- "found in the snapshot [groups=" + reqGrpIds.values() + ",
snapshot=" + snpName + ']'));
-
- return;
- }
+ assert reqGrpIds.isEmpty() : "Cache group(s) was not found in the
snapshot [groups=" + reqGrpIds.values()
Review Comment:
Why this change from exception to assert?
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotMetadataVerificationTask.java:
##########
@@ -88,25 +96,74 @@ public
MetadataVerificationJob(SnapshotMetadataVerificationTaskArg arg) {
}
/** {@inheritDoc} */
- @Override public List<SnapshotMetadata> execute() throws
IgniteException {
+ @Override public List<SnapshotMetadata> execute() {
IgniteSnapshotManager snpMgr =
ignite.context().cache().context().snapshotMgr();
- List<SnapshotMetadata> snpMeta =
snpMgr.readSnapshotMetadatas(arg.snapshotName(), arg.snapshotPath());
+ List<SnapshotMetadata> metas =
snpMgr.readSnapshotMetadatas(arg.snapshotName(), arg.snapshotPath());
+
+ for (SnapshotMetadata meta : metas)
+ checkMeta(meta,
ignite.context().config().getEncryptionSpi().masterKeyDigest());
if (arg.incrementIndex() > 0) {
- List<SnapshotMetadata> metas = snpMeta.stream()
+ List<SnapshotMetadata> locNodeMetas = metas.stream()
.filter(m ->
m.consistentId().equals(ignite.localNode().consistentId()))
.collect(Collectors.toList());
- if (metas.size() != 1) {
- throw new IgniteException("Failed to find snapshot
metafile [metas=" + metas +
- ", snpName=" + arg.snapshotName() + ", snpPath=" +
arg.snapshotPath() + ']');
+ if (locNodeMetas.size() != 1) {
+ throw new IgniteException("Failed to find single snapshot
metafile for local node [locNodeId="
+ + ignite.localNode().consistentId() + ", metas=" +
metas + ", snpName=" + arg.snapshotName()
+ + ", snpPath=" + arg.snapshotPath() + ']');
+ }
+
+ checkIncrementalSnapshots(locNodeMetas.get(0), arg);
+ }
+
+ return metas;
+ }
+
+ /** */
+ private void checkMeta(SnapshotMetadata meta, byte[] masterKeyDigest) {
+ byte[] snpMasterKeyDigest = meta.masterKeyDigest();
+
+ if (masterKeyDigest == null && snpMasterKeyDigest != null) {
+ throw new IllegalStateException("Snapshot '" +
meta.snapshotName() + "' has encrypted caches " +
+ "while encryption is disabled. To restore this snapshot,
start Ignite with configured " +
+ "encryption and the same master key.");
+ }
+
+ if (snpMasterKeyDigest != null &&
!Arrays.equals(snpMasterKeyDigest, masterKeyDigest)) {
+ throw new IllegalStateException("Snapshot '" +
meta.snapshotName() + "' has different master " +
+ "key digest. To restore this snapshot, start Ignite with
the same master key.");
+ }
+
+ Collection<Integer> grpIds = new HashSet<>(F.isEmpty(arg.grpIds())
? meta.cacheGroupIds() : arg.grpIds());
+
+ if (meta.hasCompressedGroups() &&
grpIds.stream().anyMatch(meta::isGroupWithCompression)) {
+ try {
+ if (ignite.context().compress() == null)
Review Comment:
Do actually we need this check? I suppose that user should add
ignite-compress module to classpath and this message is in the catch exception
block below. Let's catch the NPE with the IgniteCheckedException together.
--
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]
