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

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

zstan · 2026-09-23T10:48:20Z

+  active for the snapshot.
+* The operation cannot be undone and the deleted snapshot cannot be restored. The command prompts for a confirmation.
+* Before deletion, no snapshot validation is done except finding and reading its metadata. If the metadata isn't found or cannot be read, snapshot isn't deleted.
+* If the provided snapshot path belongs to Ignite's home or to Ignite's a not snapshot work directory, the operation fails.


why do we need this additional check ? You think someone can put snap meta file into Ignite home dir ?

Isn't it a good practice in general as we discussed? Why we allow to create snapshots everywhere inside Ignite? Looks like a bug. I believe, we shouldn't allow any attempts to corrupt Ignite's data with Ignite's features. A project/framework which takes into account such cases looks better to me.

We discuss: if folder contains snap meta - it can be removed, othervize - no, why we need code complication?

Also, we discussed that we should not touch Ignite's directories like snapshot creating anywhere inside. Reverted.

yes, it`s true, no one ignite working directory (besides snap of course) can`t store snap meta.

yes, its true, no one ignite working directory (besides snap of course) cant store snap meta.

Currently, can. You can use relative or absolute path while creating a snp.

zstan · 2026-09-23T12:27:06Z

+
+    /** {@inheritDoc} */
+    @Override public String confirmationPrompt(SnapshotDeleteCommandArg arg) {
+        return "This operation will completely remove snapshot: '" + arg.snapshotName() + "' and all its incrementals " +


Suggested change

return "This operation will completely remove snapshot: '" + arg.snapshotName() + "' and all its incrementals " +

return "This operation will completely remove snapshot: '" + arg.snapshotName() + "' and all its incremental snapshots." +

zstan · 2026-09-23T12:44:29Z

+            U.nl() + U.nl() +
+            "If the security is enabled, the operation requires the snapshot administration permissions." +
+            U.nl() + U.nl() +
+            "Deletion in not snapshots Ignite's directories and deletion of any data without or corrupted snapshot " +


Suggested change

"Deletion in not snapshots Ignite's directories and deletion of any data without or corrupted snapshot " +

"Deletion of any data without (valid) snapshot metadata is prohibited." +

Too complex for understanding, also incorrect:
snapshots Ignite's directories -> Ignite snapshot directories
seems my variant still more readable

zstan · 2026-09-23T12:51:20Z

+     * @return {@code True}, if data is found and completely deleted;
+     *         {@code False}, if nothing found or if data is found but might not be deleted completely.
+     */
+    public boolean deleteLocalSnapshot(SnapshotFileTree sft, @Nullable AtomicBoolean existsFlag) {


I wonder - why Atomic here ?

Because:
Variable used in lambda expression should be final or effectively final

I question about atomic - you reply about lambda and final )
Seems this complication cause you want return
IgniteSnapshotManager#deleteLocalSnapshot 2 states here, from return mechanism and from AtomicBoolean state, overcomplicated, change it plz

zstan · 2026-09-23T13:32:52Z

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


Suggested change

if (sft.binaryMeta().exists() && !U.delete(sft.binaryMeta()) && sft.binaryMeta().exists())

if (sft.binaryMeta().exists() && !U.delete(sft.binaryMeta()))

Simplified but in another manner. Added a comment.

zstan · 2026-09-23T13:33:09Z

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


why not simple return ?

Because we might not delete snapshot al all in this case when nodes share their working and snapshot directories. This would be unexpected behaviour. We have to delete if we can. If someone concurrently deletes, we should not stop. This is not an error in this case. Node can steal deletion jobs

overcomplicated, you need to use directory lock, check public static class NodeFileLockHolder extends FileLockHolder

zstan · 2026-09-23T13:37:22Z


-            deleteDirectory(sft.binaryMetaRoot());
-            deleteDirectory(sft.marshaller());
+            if (sft.meta().exists() && !U.delete(sft.meta()) && sft.meta().exists())


from spec i can found that : (translated ver)

All data in the snapshot directories is deleted. There are no checks on their type or affiliation to the snapshot.

thus - why do you need all near checks ?

Here no any data types and content checks. All this made for concurrent deletion when nodes share their working and snapshot directories.

I've added the comment:

// The 'exists' checks are for a concurrent deletion when nodes share their working and snapshot directories. // Nodes may steal removal jobs and the files aren't synchronized. There are gaps between and `exists()` and `delete()`. // We try to delete first. If snapshot data wasn't deleted because it doesn't already exist is not a delete error here.

zstan · 2026-09-23T14:03:05Z


        try {
-            contexts.remove(ctx.req.snapshotName());
+            contexts.remove(ctx.req.snapshotName().toLowerCase());


If snap name is case insensitive - it need to be documented somehow and transformed into lower case in command parsing level, isn`t it ?

This is to prevent concurrent snapshot operations for the same snapshot. User can type the name using any variants of characters cases

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

Review effort: Lite
Findings: 3 · 8 · 5

Open (16)

deleteDirectory currently returns false even on success because res is initialized to false… · New
deleteDirectory currently returns false even on success because res is initialized to false… · New
deleteDirectory currently returns false even on success because res is initialized to false… · New
Typos/grammar in documentation: “pesmissins” → “permissions”, “a not snapshot” → “a non-snapshot”,… · New
Typos/grammar in documentation: “pesmissins” → “permissions”, “a not snapshot” → “a non-snapshot”,… · New
The loop variable d is not used when streaming data; streamer.addData(i, i) repeatedly writes… · New
There are multiple grammar issues in this user-visible confirmation prompt (e.g., “Deletion in not… · New
Two issues here: (1) toLowerCase() without an explicit locale can produce surprising results in… · New
Two issues here: (1) toLowerCase() without an explicit locale can produce surprising results in… · New
Two issues here: (1) toLowerCase() without an explicit locale can produce surprising results in… · New
This method rejects other snapshot states via IgniteCheckedException, but the new “delete in… · New
“propogated” is misspelled; should be “propagated”. · New
The identifier and the output text contain a typo/awkward phrasing: UNSURED_DELETION_PREF should… · New
Javadoc typos: “exeption” → “exception”, “cant” → “can’t/cannot”. · New
The returned validation text has a grammar issue (“a an”) and reads awkwardly for a user-facing… · New
G.allGrids(); is a no-op statement here (return value is ignored, and it has no side effects). It… · New

Resolved since last review (9)

This assigns core feature ID 1 to snapshot deletion, but the existing simulated 2.19.2 release…
Request identity uses the raw path string, so null, the default snapshots root, and an equivalent…
The set is keyed by the raw path string, but deletePhase treats an empty path as the default…
This lexical prefix check does not ensure that the path is actually below snapshotsRoot: values…
The feature check is only performed inside deletePhase, after this call has broadcast an…
The new test cleanup comment misspells “paths”.
The distributed process waits for serverNodes(topVer), not baseline nodes, and deletePhase…
The new Javadoc misspells “snapshot”.
The new test cleanup comment misspells “patches”.

github-actions · 2026-09-24T12:14:04Z

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

zstan · 2026-09-24T12:58:46Z

+* The operation cannot be undone and the deleted snapshot cannot be restored. The command prompts for a confirmation.
+* Before deletion, no snapshot validation is done except finding and reading its metadata. If the metadata isn't found
+or cannot be read, snapshot isn't deleted.
+* If the provided snapshot path belongs to Ignite's home or to Ignite's a non-snapshot work directory, the operation fails.


now my comment move to this line, no need additional mention here and code checks.

zstan · 2026-09-24T13:07:58Z

+            U.nl() + U.nl() +
+            "If the security is enabled, the operation requires the snapshot administration permissions." +
+            U.nl() + U.nl() +
+            "Deletion in not snapshots Ignite's directories and deletion of any data without or corrupted snapshot " +


Too complex for understanding, also incorrect:
snapshots Ignite's directories -> Ignite snapshot directories
seems my variant still more readable

zstan · 2026-09-24T13:08:30Z

+    @Override public String confirmationPrompt(SnapshotDeleteCommandArg arg) {
+        return "This operation will completely remove snapshot: '" + arg.snapshotName() + "' and all its incrementals." +
+            U.nl() + U.nl() +
+            "If the security is enabled, the operation requires the snapshot administration permissions." +
+            U.nl() + U.nl() +
+            "Deletion in not snapshots Ignite's directories and deletion of any data without or corrupted snapshot " +
+                "metadata are prohibited." +
+            U.nl() + U.nl() +
+            "The operation cannot be reverted." +
+            U.nl() + U.nl() +
+            "NOTE: Snapshot data on offline server nodes will remain untouched.";
+    }


Copilot found the same- fix it !

zstan · 2026-09-24T13:09:44Z

+
+    /** */
+    public String snapshotName() {
+        return snapshotName;


It`s not about arg processing, it about @Nullable annotation

zstan · 2026-09-24T13:11:29Z

+
+    /** */
+    public String src() {
+        return src;


I not aware about other commands and I not reviewed them, if it possible to return null - plz annotate it correctly

zstan · 2026-09-24T13:21:32Z

+     * @return {@code True}, if data is found and completely deleted;
+     *         {@code False}, if nothing found or if data is found but might not be deleted completely.
+     */
+    public boolean deleteLocalSnapshot(SnapshotFileTree sft, @Nullable AtomicBoolean existsFlag) {


I question about atomic - you reply about lambda and final )
Seems this complication cause you want return
IgniteSnapshotManager#deleteLocalSnapshot 2 states here, from return mechanism and from AtomicBoolean state, overcomplicated, change it plz

zstan · 2026-09-24T13:25:48Z

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


overcomplicated, you need to use directory lock, check public static class NodeFileLockHolder extends FileLockHolder

zstan · 2026-09-24T13:31:55Z

+     * proceeds if snapshot already deleted.
+     */
+    @Test
+    public void testConcurrentSnapshotDeleteOperation() throws Exception {


From java doc: "Tests that snapshot create detects concurrent deletion", but i see that it tests only already existing snap ? Seems this test is not cover conc deletion opertion, other conc tests need to be re checked too

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

Vladsz83 added 19 commits September 21, 2026 20:08

test fixes, fixes

6596432

test fixes, fixes

4977c26

review fixes, checkstyle.

3da6b6a

fixes

6c076ae

+ baseline check and warning

0f09da9

+ baseline check and warning

219c572

review fixes

d8ce7d8

fixes

95bc7a1

ru fixes

501a6b9

ru fixes

23fe41c

doc fix

0e96740

+ corrupted metafile test

204acaa

manual review, review fixes

e6086d6

test fixes

d4c7e88

flaky fixes

2b2cc21

+ doc, + test fix

b1e8d98

doc fix

9d493a0

fix of the confirmation promt.

1dd3d53

zstan reviewed Sep 23, 2026

View reviewed changes

zstan requested a lite review from Copilot September 23, 2026 13:56

zstan reviewed Sep 23, 2026

View reviewed changes

Copilot AI reviewed Sep 23, 2026

View reviewed changes

Copilot started reviewing on behalf of zstan September 23, 2026 14:39 View session

Vladsz83 added 4 commits September 24, 2026 14:30

review fixes

2092624

review fix

0dd70d4

review fix

3b0da46

revert test

be040de

zstan reviewed Sep 24, 2026

View reviewed changes

	return "This operation will completely remove snapshot: '" + arg.snapshotName() + "' and all its incrementals " +
	return "This operation will completely remove snapshot: '" + arg.snapshotName() + "' and all its incremental snapshots." +

	"Deletion in not snapshots Ignite's directories and deletion of any data without or corrupted snapshot " +
	"Deletion of any data without (valid) snapshot metadata is prohibited." +

	if (sft.binaryMeta().exists() && !U.delete(sft.binaryMeta()) && sft.binaryMeta().exists())
	if (sft.binaryMeta().exists() && !U.delete(sft.binaryMeta()))

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!

Choose a reason for hiding this comment

Uh oh!

zstan Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

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!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Sep 24, 2026

Possible compatibility issues. Please, check rolling upgrade cases

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

zstan Sep 24, 2026 •

edited

Loading