Skip to content

IGNITE-29050 Introduce snapshot deletion command - #13577

Open
Vladsz83 wants to merge 37 commits into
apache:masterfrom
Vladsz83:Introduce-snapshot-deletion-command
Open

Vladsz83 wants to merge 37 commits into
apache:masterfrom
Vladsz83:Introduce-snapshot-deletion-command

Conversation

@Vladsz83

Copy link
Copy Markdown
Contributor

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.

@github-actions

Copy link
Copy Markdown

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/distributed/dht/preloader/GridDhtPartitionsSingleMessage.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


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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix for : "increments"

if (!F.isEmpty(res.uncompletedNodes())) {
found = true;

printer.accept("WARNING, the following nodes found snapshot data but might not remove it completely "

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also : do we need here and else where "." at the end ?


/** */
@Test
public void testConcurrentUnfinishedRU() throws Exception {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably it`s better to initialize with empty collections all of them ? And aslo initialization constructor code references ?


/** */
@Test
public void testNodeNotSupportingSnapshotDeleteFeature() throws Exception {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants