IGNITE-29050 Introduce snapshot deletion command by Vladsz83 · Pull Request #13577 · apache/ignite

Vladsz83 · 2026-09-10T22:13:50Z

Thank you for submitting the pull request to the Apache Ignite.

In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:

The Contribution Checklist

There is a single JIRA ticket related to the pull request.
The web-link to the pull request is attached to the JIRA ticket.
The JIRA ticket has the Patch Available state.
The pull request body describes changes that have been made.
The description explains WHAT and WHY was made instead of HOW.
The pull request title is treated as the final commit message.
The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
A reviewer has been mentioned through the JIRA comments
(see the Maintainers list)
The pull request has been checked by the Teamcity Bot and
the green visa attached to the JIRA ticket (see tab PR Check at TC.Bot - Instance 1 or TC.Bot - Instance 2)

Notes

If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com #ignite channel.

# Conflicts: # modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/schema/IgniteSchema.java

…ommand

zstan · 2026-09-30T14:00:14Z

+                if (status == null || status == res.status)
+                    status = res.status;


why do u need to set status = res.status if it already can be the same ? status == res.status branch

Done. Will push a bit later

now it hard to understand logic : if status ==null - you set it to res.status, otherwize you set it to PARTLY despite of incoming status (

…ommand

… into IGNITE-29050-Introduce-snapshot-deletion-command # Conflicts: # modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteClusterSnapshotDeleteTest.java

…ommand

Copilot

Copilot review overview

🟡 Changes recommended

Critical issues remain in deletion cleanup, metadata validation, and rolling-upgrade feature identification.

Review effort: Lite
Findings: 3 · 6

Open (9)

Preserve metadata when snapshot cleanup is incomplete · New
Prevent deletion when any snapshot metadata is unreadable · New
Reconcile duplicate rolling-upgrade feature ID · New
Resolve relative snapshot source paths against configured snapshot directory · New
Use filesystem-aware snapshot name comparison during restore · New
Use filesystem-aware snapshot matching in incremental scans · New
Use filesystem-aware keys for snapshot validation contexts · New
Use filesystem-aware identity for creation and deletion conflicts · New
Check captured permissions before restoring them · New

Resolved since last review (7)

The feature check is only performed inside deletePhase, after this call has broadcast an…
Snapshot deletion key omits snapshot name
Conflict checks ignore snapshot path
Correct wording after “has completely”
Fix typo in “snapshot”
Refer to filesystem paths, not patches
Use a Windows path in the Windows example

-            U.delete(sft.binaryMeta());
-            sft.allStorages().forEach(U::delete);
-            U.delete(sft.meta());
+            if (!sft.meta().delete() && sft.meta().exists())


Vladsz83 · 2026-10-01T12:49:26Z

+
+            // We need to find and read snapshot metas to ensure the content is a snapshot. Also, the metas contain
+            // initial cluster topology and actual snapshot folder names.
+            List<SnapshotMetadata> locMetas = kctx.cache().context().snapshotMgr().readSnapshotMetadatas(snpFiles, false);


This seems not so critical. On separated nodes, with own working directory, there is one snapshot. In the shared snapshot folder, we'll try to remove entire if just one meta is read. It is ok to me.

Vladsz83 · 2026-10-01T12:51:04Z

    public static final IgniteFeature ROLLING_UPGRADE_FEATURE = new IgniteCoreFeature(0);
+
+    /** */
+    public static final IgniteFeature SNAPSHOT_DELETE_FEATURE = new IgniteCoreFeature(1);


This is relative test sets. I think isn't critical it all the related tests passes.

Vladsz83 · 2026-10-01T12:55:03Z

+    @Argument(example = "path/to/snapshots", optional = true, description = "Path to snapshot location directory. If not specified " +
+        "or specified a relative path, the default snapshot configuration directory will be used")


Vladsz83 · 2026-10-01T12:57:01Z

+        if (snpName == null)
+            return false;
+
+        return snpName.equalsIgnoreCase(restoreCacheGrpProc.restoringSnapshotName());


We know. Many snapshot operations doesn't support case insensitivity. This is current limitation. We have tests for such cases. Bringing filesystem-aware/path-aware comparison everywhere is out of scope of this ticket.

Vladsz83 · 2026-10-01T12:57:23Z


-                if (rec.type() == CLUSTER_SNAPSHOT && ((ClusterSnapshotRecord)rec).clusterSnapshotName().equals(sft.name())) {
+                // A filesystem might not support the character case of directory or file name.
+                if (rec.type() == CLUSTER_SNAPSHOT && ((ClusterSnapshotRecord)rec).clusterSnapshotName().equalsIgnoreCase(sft.name())) {


We know. Many snapshot operations doesn't support case insensitivity. This is current limitation. We have tests for such cases. Bringing filesystem-aware/path-aware comparison everywhere is out of scope of this ticket.

Vladsz83 · 2026-10-01T12:57:36Z

                return new GridFinishedFuture<>(new NodeStoppingException("The node is stopping: " + kctx.localNodeId()));

-            ctx = contexts.computeIfAbsent(req.snapshotName(), snpName -> new SnapshotCheckContext(req));
+            ctx = contexts.computeIfAbsent(req.snapshotName().toLowerCase(Locale.ROOT), snpName -> new SnapshotCheckContext(req));


We know. Many snapshot operations doesn't support case insensitivity. This is current limitation. We have tests for such cases. Bringing filesystem-aware/path-aware comparison everywhere is out of scope of this ticket.

Vladsz83 · 2026-10-01T12:57:51Z

+
+        SnapshotOperationRequest curCreateRq = snpMgr.currentCreateRequest();
+
+        if (curCreateRq != null && curCreateRq.snpName.equalsIgnoreCase(req.snpName)) {


We know. Many snapshot operations doesn't support case insensitivity. This is current limitation. We have tests for such cases. Bringing filesystem-aware/path-aware comparison everywhere is out of scope of this ticket.

Vladsz83 · 2026-10-01T12:59:20Z

+            if (pathRef.get() != null && pathRef.get() != null)
+                Files.setPosixFilePermissions(pathRef.get(), prevPerms.get());


… into IGNITE-29050-Introduce-snapshot-deletion-command # Conflicts: # modules/core/src/test/resources/org.apache.ignite.util/GridCommandHandlerClusterByClassWithSSLTest_help.output

…ommand # Conflicts: # modules/core/src/test/java/org/apache/ignite/internal/processors/rollingupgrade/feature/TestIgniteReleaseFeatures_2_19_1.java

… into IGNITE-29050-Introduce-snapshot-deletion-command # Conflicts: # modules/core/src/test/java/org/apache/ignite/internal/processors/rollingupgrade/feature/TestIgniteReleaseFeatures_2_19_1.java

…ommand

github-actions · 2026-10-02T09:29:27Z

Possible compatibility issues. Please, check rolling upgrade cases

This PR modifies protected classes (with Order annotation).
Changes to these classes can break rolling upgrade compatibility.

Affected files:

modules/core/src/main/java/org/apache/ignite/internal/management/snapshot/SnapshotDeleteCommandArg.java
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotDeleteProcessResult.java
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotDeleteRequest.java
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotDeleteResponse.java

Vladsz83 and others added 20 commits September 7, 2026 19:39

neuroslop

eb6273c

IGNITE-29037 Refactored Message Factory usage in TCP Discovery implem…

d9aa470

…entations (apache#13551)

raw

7042aec

raw

682a21c

Merge branch 'master' into support-snapshot-lists

bcc72c4

raw

3ab443a

raw

9171c7e

+ snpPath

de68328

better tests

f0996e9

own test

80c7644

Merge branch 'master' into snp-delete

c8d1c25

# Conflicts: # modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/schema/IgniteSchema.java

+ test

fc11a5f

Simplification

8a007e3

+ restore test

97ab26c

+ create test

fbb2cf8

reimpl tests

2ed26ad

better IgniteClusterSnapshotDeleteTest

9b0e5ff

+incr test

b09e0b3

raw

71717c2

+ feature support test

7148da6

github-actions Bot added the compatibility label Sep 10, 2026

Vladsz83 added 9 commits September 11, 2026 01:50

+ IgniteClusterSnapshotDeleteTest

d36c642

fix

9a658df

Merge branch 'master' into Introduce-snapshot-deletion-command

35a4c9d

test fix + doc

d715c66

+ RU test. + many small fixes.

e6aa330

review fixes

4233322

test fix

b11f389

Merge branch 'master' into IGNITE-29050-Introduce-snapshot-deletion-c…

37cb071

…ommand

+ tests

f00d266

test fixes

40eb537

zstan reviewed Sep 30, 2026

View reviewed changes

Comment thread ...e/ignite/internal/processors/cache/persistence/snapshot/IgniteClusterSnapshotDeleteTest.java Outdated

test simplification

142b32c

zstan reviewed Sep 30, 2026

View reviewed changes

Comment thread ...e/ignite/internal/processors/cache/persistence/snapshot/IgniteClusterSnapshotDeleteTest.java

Vladsz83 added 12 commits September 30, 2026 18:07

review fixes

fb42198

Merge branch 'master' into IGNITE-29050-Introduce-snapshot-deletion-c…

e162fda

…ommand

Merge branch 'master' into IGNITE-29050-Introduce-snapshot-deletion-c…

b3582fe

…ommand

Merge remote-tracking branch 'my/Introduce-snapshot-deletion-command'…

4c85930

… into IGNITE-29050-Introduce-snapshot-deletion-command # Conflicts: # modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteClusterSnapshotDeleteTest.java

+ master, review fixes

7e5bf2d

improve

4b4a156

fixes

63426cb

review fixes

7147213

review fixes

5e9682e

Merge branch 'master' into IGNITE-29050-Introduce-snapshot-deletion-c…

3016b67

…ommand

test fixes

54e7574

Merge branch 'master' into IGNITE-29050-Introduce-snapshot-deletion-c…

d7f2d75

…ommand

zstan requested a lite review from Copilot October 1, 2026 12:24

Copilot started reviewing on behalf of zstan October 1, 2026 12:25 View session

Copilot AI reviewed Oct 1, 2026

View reviewed changes

Vladsz83 added 9 commits October 1, 2026 16:16

copilot review fixes

233e0ab

Merge remote-tracking branch 'my/Introduce-snapshot-deletion-command'…

58c28a7

… into IGNITE-29050-Introduce-snapshot-deletion-command # Conflicts: # modules/core/src/test/resources/org.apache.ignite.util/GridCommandHandlerClusterByClassWithSSLTest_help.output

fix

722ba31

fix

877496b

revert mistaken deletion cycle processing

20998d4

Merge branch 'master' into IGNITE-29050-Introduce-snapshot-deletion-c…

6303876

…ommand # Conflicts: # modules/core/src/test/java/org/apache/ignite/internal/processors/rollingupgrade/feature/TestIgniteReleaseFeatures_2_19_1.java

Merge remote-tracking branch 'my/Introduce-snapshot-deletion-command'…

c2f1e13

… into IGNITE-29050-Introduce-snapshot-deletion-command # Conflicts: # modules/core/src/test/java/org/apache/ignite/internal/processors/rollingupgrade/feature/TestIgniteReleaseFeatures_2_19_1.java

Merge branch 'master' into IGNITE-29050-Introduce-snapshot-deletion-c…

0fd27b9

…ommand

RU tests fixes

268a42a

		if (status == null \|\| status == res.status)
		status = res.status;

		@Argument(example = "path/to/snapshots", optional = true, description = "Path to snapshot location directory. If not specified " +
		"or specified a relative path, the default snapshot configuration directory will be used")


		SnapshotOperationRequest curCreateRq = snpMgr.currentCreateRequest();

		if (curCreateRq != null && curCreateRq.snpName.equalsIgnoreCase(req.snpName)) {

		if (pathRef.get() != null && pathRef.get() != null)
		Files.setPosixFilePermissions(pathRef.get(), prevPerms.get());

Conversation

Vladsz83 commented Sep 10, 2026

The Contribution Checklist

Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Oct 2, 2026

Possible compatibility issues. Please, check rolling upgrade cases

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants