Conversation
# Conflicts: # modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/schema/IgniteSchema.java
zstan
reviewed
Sep 17, 2026
|
|
||
| The delete operation is subject to the following limitations: | ||
|
|
||
| * The deletion is rejected if any concurrent snapshot operation (create, restore, check, or a delete with the same name) is |
Contributor
There was a problem hiding this comment.
if any concurrent - "concurrent_" is sounds like our working term it clear for you and me but for common people you need to re-phrase it like : "The deletion is rejected if any snapshot operation with the same snapshot path\name" smth like this. Concurrent may be treated like JUST one more snapshot operation no matter with this name\path or another.
|
|
||
| * The deletion is rejected if any concurrent snapshot operation (create, restore, check, or a delete with the same name) is | ||
| active for the snapshot. | ||
| * The operation is irreversible. It cannot be undone, and the deleted snapshot cannot be restored. |
Contributor
There was a problem hiding this comment.
Suggested change
| * The operation is irreversible. It cannot be undone, and the deleted snapshot cannot be restored. | |
| * The operation cannot be cancelled. It cannot be undone, and the deleted snapshot cannot be restored. |
| * The deletion is rejected if any concurrent snapshot operation (create, restore, check, or a delete with the same name) is | ||
| active for the snapshot. | ||
| * The operation is irreversible. It cannot be undone, and the deleted snapshot cannot be restored. | ||
| * The command prompts for a confirmation before the deletion because the operation is irreversible and cannot be undone. |
Contributor
There was a problem hiding this comment.
Suggested change
| * The command prompts for a confirmation before the deletion because the operation is irreversible and cannot be undone. | |
| * The command prompts for a confirmation before the deletion. |
| public class SnapshotDeleteCommand extends AbstractSnapshotCommand<SnapshotDeleteCommandArg, SnapshotDeleteProcessResult> { | ||
| /** {@inheritDoc} */ | ||
| @Override public String description() { | ||
| return "Deletes snapshot and all its increments from all the online server nodes"; |
| if (!F.isEmpty(res.uncompletedNodes())) { | ||
| found = true; | ||
|
|
||
| printer.accept("WARNING, the following nodes found snapshot data but might not remove it completely " |
Contributor
There was a problem hiding this comment.
Suggested change
| printer.accept("WARNING, the following nodes found snapshot data but might not remove it completely " | |
| printer.accept("WARNING: the following nodes found snapshot data but might not remove it completely" | |
| ``` also i can\t fount the relative test, plz append it |
Contributor
There was a problem hiding this comment.
also : do we need here and else where "." at the end ?
|
|
||
| /** */ | ||
| @Test | ||
| public void testConcurrentUnfinishedRU() throws Exception { |
Contributor
There was a problem hiding this comment.
test name - confusing, rename it please, or append java doc
|
|
||
| /** Nodes which found snapshot data and completely removed it. */ | ||
| @Order(0) | ||
| @Nullable Collection<UUID> completedNodes; |
Contributor
There was a problem hiding this comment.
probably it`s better to initialize with empty collections all of them ? And aslo initialization constructor code references ?
|
|
||
| /** */ | ||
| @Test | ||
| public void testNodeNotSupportingSnapshotDeleteFeature() throws Exception { |
Contributor
There was a problem hiding this comment.
test name need to be changed, as I can see - this test is about unsupported ver of thick client?
Comment on lines
+138
to
+141
| for (int i = 0; i < ALL_GRIDS; i++) { | ||
| assertFalse(ru(grid(i)).isVersionUpgradeEnabled()); | ||
|
|
||
| assertFalse(F.isEmpty(snp(i).deleteSnapshot(SNP_NAME, null).get().completedNodes)); |
Contributor
There was a problem hiding this comment.
Suggested change
| for (int i = 0; i < ALL_GRIDS; i++) { | |
| assertFalse(ru(grid(i)).isVersionUpgradeEnabled()); | |
| assertFalse(F.isEmpty(snp(i).deleteSnapshot(SNP_NAME, null).get().completedNodes)); | |
| @Nullable Collection<UUID> compNodes = null; | |
| for (int i = 0; i < ALL_GRIDS; i++) { | |
| assertFalse(ru(grid(i)).isVersionUpgradeEnabled()); | |
| if (compNodes == null) | |
| compNodes = snp(i).deleteSnapshot(SNP_NAME, null).get().completedNodes(); | |
| else | |
| assertEqualsCollectionsIgnoringOrder(compNodes, snp(i).deleteSnapshot(SNP_NAME, null).get().completedNodes()); |
Comment on lines
+150
to
+158
| int partsCnt = 32; | ||
| int keysCnt = partsCnt * 10; | ||
|
|
||
| grid(gridIdx).createCache(new CacheConfiguration<>(DEFAULT_CACHE_NAME) | ||
| .setCacheMode(CacheMode.REPLICATED) | ||
| .setBackups(1) | ||
| .setAffinity(new RendezvousAffinityFunction().setPartitions(32)) | ||
| .setWriteSynchronizationMode(CacheWriteSynchronizationMode.FULL_SYNC) | ||
| .setAtomicityMode(CacheAtomicityMode.ATOMIC)); |
Contributor
There was a problem hiding this comment.
Suggested change
| int partsCnt = 32; | |
| int keysCnt = partsCnt * 10; | |
| grid(gridIdx).createCache(new CacheConfiguration<>(DEFAULT_CACHE_NAME) | |
| .setCacheMode(CacheMode.REPLICATED) | |
| .setBackups(1) | |
| .setAffinity(new RendezvousAffinityFunction().setPartitions(32)) | |
| .setWriteSynchronizationMode(CacheWriteSynchronizationMode.FULL_SYNC) | |
| .setAtomicityMode(CacheAtomicityMode.ATOMIC)); | |
| int partsCnt = 5; | |
| int keysCnt = partsCnt * 10; | |
| grid(gridIdx).createCache(new CacheConfiguration<>(DEFAULT_CACHE_NAME) | |
| .setCacheMode(CacheMode.REPLICATED) | |
| .setAffinity(new RendezvousAffinityFunction().setPartitions(partsCnt)) | |
| .setAtomicityMode(CacheAtomicityMode.ATOMIC)); |
a bit speed up
There was a problem hiding this comment.
🟡 Changes recommended
Critical path-safety, feature-gating, path-identity, and rolling-upgrade issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
IGNITE-29050 adds cluster-wide snapshot deletion through the persistence API and control utility.
Changes:
- Adds distributed deletion, conflict handling, path validation, and rolling-upgrade gating.
- Adds the
--snapshot deletecommand, help text, and documentation. - Adds deletion, concurrency, rolling-upgrade, and command tests.
File summaries
| File | Summary and review findings |
|---|---|
modules/core/src/test/resources/org.apache.ignite.util/GridCommandHandlerClusterByClassWithSSLTest_help.output |
Updates SSL command help output. |
modules/core/src/test/resources/org.apache.ignite.util/GridCommandHandlerClusterByClassTest_help.output |
Updates command help output. |
modules/core/src/test/java/org/apache/ignite/testsuites/IgniteSnapshotTestSuite8.java |
Registers snapshot tests. |
modules/core/src/test/java/org/apache/ignite/internal/processors/rollingupgrade/feature/TestIgniteReleaseFeatures_2_19_1.java |
Adds feature coverage. Critical (1 vote): feature ID reuse aliases an unrelated 2.19.2 feature; subsequent IDs, aliases, and expected ranges must be adjusted. |
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteClusterSnapshotSelfTest.java |
Tests creation/deletion concurrency. |
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteClusterSnapshotRestoreSelfTest.java |
Tests restore/deletion concurrency. |
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteClusterSnapshotDeleteTest.java |
Tests snapshot deletion behavior. |
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteClusterSnapshotDeleteRollingUpgradeTest.java |
Tests rolling-upgrade behavior. |
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteClusterSnapshotCheckTest.java |
Tests check/deletion concurrency. |
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/AbstractSnapshotSelfTest.java |
Adds shared test helpers. Nit (2 votes): cleanup comment misspells “paths”. |
modules/core/src/main/java/org/apache/ignite/internal/util/distributed/DistributedProcess.java |
Adds the deletion process type. |
modules/core/src/main/java/org/apache/ignite/internal/processors/rollingupgrade/feature/SupportedFeatureRegistry.java |
Registers the deletion feature. |
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotRestoreProcess.java |
Rejects restore during deletion. |
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotDeleteResponse.java |
Defines node deletion responses. |
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotDeleteRequest.java |
Defines deletion requests. Critical (1 vote): raw path identity allows equivalent operations to run concurrently; normalize the effective root. |
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotDeleteProcessResult.java |
Defines aggregated deletion results. |
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotDeleteProcess.java |
Implements distributed deletion and validation. Critical (2 votes): check the feature before broadcasting messages during rolling upgrades. Critical (1 vote): normalize empty/default paths for deletion-state checks. Critical (3 votes): canonicalize paths and enforce segment-aware containment. Moderate (1 vote): use normalized path identity for conflict checks. Moderate (1 vote): unify relative-path resolution across snapshot operations. |
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotCheckProcess.java |
Rejects checks during deletion. |
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteSnapshotManager.java |
Integrates deletion and local cleanup. Moderate (1 vote): failed path visits can report incomplete deletion as successful. Nits (2, 2, and 1 votes): clarify that completion covers all online server nodes and correct Javadoc spelling. |
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/dht/preloader/GridDhtPartitionsSingleMessage.java |
Corrects Javadoc spelling. |
modules/core/src/main/java/org/apache/ignite/internal/management/snapshot/SnapshotDeleteTask.java |
Executes deletion from management tasks. |
modules/core/src/main/java/org/apache/ignite/internal/management/snapshot/SnapshotDeleteCommandArg.java |
Defines command arguments. |
modules/core/src/main/java/org/apache/ignite/internal/management/snapshot/SnapshotDeleteCommand.java |
Implements command output and confirmation. |
modules/core/src/main/java/org/apache/ignite/internal/management/snapshot/SnapshotCommand.java |
Registers the delete subcommand. |
modules/core/src/main/java/org/apache/ignite/internal/CoreMessagesProvider.java |
Registers deletion messages. |
modules/control-utility/src/test/java/org/apache/ignite/util/GridCommandHandlerDeleteSnapshotTest.java |
Tests the control command. Moderate (1 vote): incremental variants preload repeated rather than distinct keys. Nit (2 votes): cleanup comment misspells “patches”. |
modules/control-utility/src/test/java/org/apache/ignite/testsuites/IgniteControlUtilityTestSuite.java |
Registers command tests. |
modules/control-utility/src/test/java/org/apache/ignite/internal/commandline/CommandHandlerParsingTest.java |
Covers command parsing. |
docs/_docs/snapshots/snapshots.adoc |
Documents snapshot deletion. |
Review details
Suppressed comments (7)
modules/control-utility/src/test/java/org/apache/ignite/util/GridCommandHandlerDeleteSnapshotTest.java:152
- The loop computes distinct keys in
d, but this line repeatedly writes only the outer increment indexi(and valuei). The incremental-snapshot variants therefore do not preload the intendedentriesCnt / 4new entries and do not exercise deletion of distinct incremental data.
streamer.addData(i, i);
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteSnapshotManager.java:739
- The new Javadoc misspells “snapshot” as “shapshot”.
* Deletes local shapshot data.
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteSnapshotManager.java:817
- Because
visitFileFailedignores its exception, this return can report success even when the snapshot root or a child could not be visited and still exists. That causes the distributed result to classify incomplete deletion asDELETED; mark the result false for failed paths that remain, while keeping the concurrent-removal case successful.
return res.get();
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotDeleteProcess.java:248
- When
--srcpoints to a directory that is absent on a node, this returnsIllegalArgumentException;DistributedProcesspropagates that error and the command exits with invalid arguments. The added command test explicitly expects a missing source path to be a successful no-op, so an absent directory should produce aNOT_FOUND/empty-node result rather than aborting the whole deletion.
if (!path.exists())
return SNP_PATH_ERR_PREF + "doesn't exist";
if (!path.isDirectory())
return SNP_PATH_ERR_PREF + "is not a directory";
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotDeleteProcess.java:188
SnapshotFileTreevalidatesreq.snpNameand can throw here, but the executor callback has no catch; only the success path completesreqLocFut, whilefinallymerely removes the request. An invalid public API or CLI name such asbad-nametherefore leaves the distributed process waiting forever and makes the control utility's.get()hang. CompletereqLocFutexceptionally when the callback fails.
var sft = new SnapshotFileTree(kctx, req.snpName, path0 == null ? null : path0.getAbsolutePath());
boolean deleted = snpMgr.deleteLocalSnapshot(sft, foundFlag);
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotDeleteProcess.java:138
- This guard compares only the snapshot name, while the newly added
isSnapshotDeleting(name, path)checks are path-sensitive and the different-path tests allow the operations when deletion starts first. If create (and similarly restore/check) starts first for another path, deletion is rejected here, so behavior depends on start order even though the snapshot roots are independent. Make the conflict check use the same normalized path identity.
if (curCreateRq != null && curCreateRq.snpName.equals(req.snpName)) {
return new GridFinishedFuture<>(new IgniteIllegalStateException(OP_REJECT_MSG +
"Snapshot with this name is being created [req=" + req + ']'));
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotDeleteProcess.java:162
- Relative
--srcpaths are resolved here against the configured snapshots root, but create/restore/check pass the same relative path directly toSnapshotFileTree, wherenew File(path, name)resolves it against the JVM working directory. Thus a snapshot created withsrc=foois not deleted bysrc=foo(and this validation may report the snapshots-root/foodirectory as missing). Use one path-resolution rule for all snapshot operations or reject relative paths consistently.
if (!path.isAbsolute())
path = new File(kctx.pdsFolderResolver().fileTree().snapshotsRoot(), req.snpPath);
- Files reviewed: 29/29 changed files
- Comments generated: 9
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| SnapshotDeleteRequest req = new SnapshotDeleteRequest(reqId, snpName, snpPath); | ||
|
|
||
| distrProc.start(reqId, req); |
| var ignWorkRootStr = kctx.pdsFolderResolver().fileTree().root().getAbsolutePath(); | ||
| var pathStr = path.getAbsolutePath(); | ||
|
|
||
| if (pathStr.startsWith(ignWorkRootStr) && !pathStr.startsWith(ignWorkRoot.snapshotsRoot().getAbsolutePath())) |
Comment on lines
+309
to
+310
| public boolean isSnapshotDeleting(String snpName, @Nullable String snpPath) { | ||
| return requests.contains(new SnapshotDeleteRequest(null, snpName, snpPath)); |
|
|
||
| SnapshotDeleteRequest other = (SnapshotDeleteRequest)o; | ||
|
|
||
| return snpName.equals(other.snpName) && Objects.equals(snpPath, other.snpPath); |
| public static final IgniteFeature ROLLING_UPGRADE_FEATURE = TestIgniteReleaseFeatures_2_19_0.ROLLING_UPGRADE_FEATURE; | ||
|
|
||
| /** */ | ||
| public static final IgniteFeature SNAPSHOT_DELETE_FEATURE = SupportedFeatureRegistry.SNAPSHOT_DELETE_FEATURE; |
| @Override protected void cleanPersistenceDir() throws Exception { | ||
| super.cleanPersistenceDir(); | ||
|
|
||
| // Also cleans separated snapshot working directories and custom snapshot pacthes. |
| sft.allStorages().forEach(U::delete); | ||
| U.delete(sft.meta()); | ||
| /** | ||
| * Deletes local shapshot data. |
| * | ||
| * @param name Snapshot name. | ||
| * @param snpPath Snapshot directory path. If {@code null}, the default configured snapshot directory will be used. | ||
| * @return Future which will be completed when the snapshot is deleted on all the baseline nodes. |
| @Override protected void cleanPersistenceDir() throws Exception { | ||
| super.cleanPersistenceDir(); | ||
|
|
||
| // Clean all: also separated snapshot working directories and custom snapshot pathes. |
zstan
reviewed
Sep 18, 2026
| import static org.junit.Assume.assumeTrue; | ||
|
|
||
| /** Test for the command '--snapshot delete'. */ | ||
| @RunWith(Parameterized.class) |
|
|
||
| /** */ | ||
| @Parameter(2) | ||
| public boolean addIncrements; |
Contributor
There was a problem hiding this comment.
please remove all such notation
| F.asList(false, true), // Add increments to the test snapshot; | ||
| F.asList(false, true), // Change baseline; | ||
| F.asList(false, true), // Use custom snapshot path; | ||
| F.asList(true, false) // Separated (own) work directory. |
Contributor
There was a problem hiding this comment.
just 4 fun )) all previous : false, true, but only this in different order )
| @Override protected void beforeTest() throws Exception { | ||
| super.beforeTest(); | ||
|
|
||
| /** Handy if test running is interrupted and {@link #afterTest()} isn't invoked. */ |
Contributor
There was a problem hiding this comment.
afterTest not cleans the directory - why we need such a line here ?
| // work directories are set. | ||
| assumeTrue(!customPath || !separatedWorkDir); | ||
|
|
||
| int entriesCnt = 4000; |
Contributor
There was a problem hiding this comment.
is it really need ? I mean why 10, 100 is not enough ?
|
|
||
| /** */ | ||
| @Test | ||
| public void testSnapshotDelete() throws Exception { |
Possible compatibility issues. Please, check rolling upgrade casesThis PR modifies protected classes (with Order annotation). Affected files:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
The description explains WHAT and WHY was made instead of HOW.
The following pattern must be used:
IGNITE-XXXX Change summarywhereXXXX- number of JIRA issue.(see the Maintainers list)
the
green visaattached to the JIRA ticket (see tabPR Checkat 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.