zstan commented on code in PR #13577:
URL: https://github.com/apache/ignite/pull/13577#discussion_r4294110418
##########
modules/control-utility/src/test/java/org/apache/ignite/testsuites/IgniteControlUtilityTestSuite.java:
##########
@@ -74,6 +75,7 @@
GridCommandHandlerCheckIndexesInlineSizeTest.class,
GridCommandHandlerMetadataTest.class,
GridCommandHandlerCheckIncrementalSnapshotTest.class,
+ GridCommandHandlerDeleteSnapshotTest.class,
Review Comment:
Seems you need to remove it from here ?
##########
modules/core/src/main/java/org/apache/ignite/internal/management/snapshot/SnapshotDeleteCommand.java:
##########
@@ -0,0 +1,123 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.management.snapshot;
+
+import java.util.Map;
+import java.util.UUID;
+import java.util.function.Consumer;
+import java.util.stream.Collectors;
+import
org.apache.ignite.internal.processors.cache.persistence.snapshot.SnapshotDeleteProcess;
+import
org.apache.ignite.internal.processors.cache.persistence.snapshot.SnapshotDeleteProcessResult;
+import
org.apache.ignite.internal.processors.rollingupgrade.feature.CoreFeatureRegistry;
+import org.apache.ignite.internal.util.typedef.internal.U;
+
+/**
+ * Snapshot deletion command.
+ *
+ * @see CoreFeatureRegistry#SNAPSHOT_DELETE_FEATURE
+ * @see SnapshotDeleteProcess
+ */
+public class SnapshotDeleteCommand extends
AbstractSnapshotCommand<SnapshotDeleteCommandArg, SnapshotDeleteProcessResult> {
+ /** */
+ public static final String DESC = "Deletes the snapshot and all its
incremental snapshots from all online server nodes";
+
+ /** */
+ public static final String UNSURE_DELETION_PREF = "WARNING: the following
nodes found snapshot data but might not " +
+ "remove it completely: ";
+
+ /** */
+ public static final String REMOVED_PREF = "Snapshot removal is completed
on: ";
+
+ /** */
+ public static final String NODE_NOT_FOUND_PREF = "NOTE: the following
nodes can't find any snapshot data, " +
+ "operation skipped: ";
+
+ /** */
+ public static final String NOT_FOUND = "Snapshot not found on available
server nodes.";
+
+ /** */
+ public static final String MISSING_BASELINES = "WARNING: the snapshot's
baseline nodes with the following consistent " +
+ "ids are missing in current cluster: ";
Review Comment:
```suggestion
"IDs are missing in current cluster: ";
```
##########
modules/control-utility/src/test/java/org/apache/ignite/util/GridCommandHandlerDeleteSnapshotTest.java:
##########
@@ -0,0 +1,240 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.util;
+
+import java.io.File;
+import java.nio.file.DirectoryStream;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Collection;
+import org.apache.ignite.IgniteDataStreamer;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.management.snapshot.SnapshotDeleteCommand;
+import org.apache.ignite.internal.util.typedef.F;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.junit.Test;
+import org.junit.runners.Parameterized.Parameter;
+import org.junit.runners.Parameterized.Parameters;
+
+import static java.nio.file.Files.newDirectoryStream;
+import static org.apache.ignite.cluster.ClusterState.ACTIVE;
+import static
org.apache.ignite.internal.commandline.CommandHandler.EXIT_CODE_OK;
+import static
org.apache.ignite.internal.processors.cache.persistence.snapshot.AbstractSnapshotSelfTest.snp;
+import static org.apache.ignite.testframework.GridTestUtils.waitForCondition;
+import static org.junit.Assume.assumeTrue;
+
+/** Test for the command '--snapshot delete'. */
+public class GridCommandHandlerDeleteSnapshotTest extends
GridCommandHandlerAbstractTest {
+ /** Flag to add an extra server node after the cluster start and the
snapshot creation. */
+ @Parameter(1)
+ public boolean extraServerNode;
+
+ /** */
+ @Parameter(2)
+ public boolean incremental;
+
+ /** */
+ @Parameter(3)
+ public boolean changeBaseline;
+
+ /** */
+ @Parameter(4)
+ public boolean customPath;
+
+ /** */
+ @Parameter(5)
+ public boolean separatedWorkDir;
+
+ /** */
+ @Parameters(name =
"client={0},useExtraNode={1},inc={2},chBaseln={3},cstSnpPath={4},ownWorkDir={5}")
Review Comment:
Appropriate param called :
```
@Parameter(5)
public boolean separatedWorkDir;
```
but you name it as : `ownWorkDir` it hard to understand
##########
modules/control-utility/src/test/java/org/apache/ignite/util/GridCommandHandlerDeleteSnapshotTest.java:
##########
@@ -0,0 +1,240 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.util;
+
+import java.io.File;
+import java.nio.file.DirectoryStream;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Collection;
+import org.apache.ignite.IgniteDataStreamer;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.management.snapshot.SnapshotDeleteCommand;
+import org.apache.ignite.internal.util.typedef.F;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.junit.Test;
+import org.junit.runners.Parameterized.Parameter;
+import org.junit.runners.Parameterized.Parameters;
+
+import static java.nio.file.Files.newDirectoryStream;
+import static org.apache.ignite.cluster.ClusterState.ACTIVE;
+import static
org.apache.ignite.internal.commandline.CommandHandler.EXIT_CODE_OK;
+import static
org.apache.ignite.internal.processors.cache.persistence.snapshot.AbstractSnapshotSelfTest.snp;
+import static org.apache.ignite.testframework.GridTestUtils.waitForCondition;
+import static org.junit.Assume.assumeTrue;
+
+/** Test for the command '--snapshot delete'. */
+public class GridCommandHandlerDeleteSnapshotTest extends
GridCommandHandlerAbstractTest {
+ /** Flag to add an extra server node after the cluster start and the
snapshot creation. */
+ @Parameter(1)
+ public boolean extraServerNode;
+
+ /** */
+ @Parameter(2)
+ public boolean incremental;
+
+ /** */
+ @Parameter(3)
+ public boolean changeBaseline;
+
+ /** */
+ @Parameter(4)
+ public boolean customPath;
+
+ /** */
+ @Parameter(5)
+ public boolean separatedWorkDir;
+
+ /** */
+ @Parameters(name =
"client={0},useExtraNode={1},inc={2},chBaseln={3},cstSnpPath={4},ownWorkDir={5}")
+ public static Collection<?> parameters() {
+ return GridTestUtils.cartesianProduct(
+ commandHandlers(),
+ F.asList(false, true), // Use or not extra server node after the
cluster start and the snapshot creation;
Review Comment:
Common practise is to set a "." not ";" at the comment end, for example :
org.apache.ignite.spi.communication.tcp.CommunicationConnectionPoolMetricsTest#params
##########
modules/core/src/test/resources/org.apache.ignite.util/GridCommandHandlerClusterByClassWithSSLTest_help.output:
##########
@@ -269,6 +269,13 @@ This utility can do the following commands:
Get the status of the current snapshot operation:
control.(sh|bat) --snapshot status
+ Deletes the snapshot and all its incremental snapshots from all online
server nodes:
+ control.(sh|bat) --snapshot delete snapshot_name [--src path/to/snapshots]
Review Comment:
change to common style : `[--src path]`
##########
modules/core/src/test/resources/org.apache.ignite.util/GridCommandHandlerClusterByClassTest_help.output:
##########
@@ -269,6 +269,13 @@ This utility can do the following commands:
Get the status of the current snapshot operation:
control.(sh|bat) --snapshot status
+ Deletes the snapshot and all its incremental snapshots from all online
server nodes:
+ control.(sh|bat) --snapshot delete snapshot_name [--src path/to/snapshots]
Review Comment:
Change it consistently to common style : `[--src path]`
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotCheckProcess.java:
##########
@@ -487,12 +489,17 @@ private IgniteInternalFuture<SnapshotCheckResponse>
prepareAndCheckMetas(UUID ig
if (nodeStopping)
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));
Review Comment:
If we have no appropriate tests for such a case - no need to change it and
just reject Copilot comment. If i`m right I suggest to revert this change and
fill appropriate issue.
##########
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/AbstractSnapshotSelfTest.java:
##########
@@ -814,6 +862,86 @@ public static void doSnapshotCancellationTest(
assertEquals("Snapshot directory must be empty due to snapshot
cancelled", 0, snpDir.list().length);
}
+ /** Tests concurrent snapshot operation when snapshot deletion with the
same name is active. */
+ protected void doConcurrentSnapshotOperationWhenDeletionIsActive(
+ ExRunnable prepareCluster,
+ ExRunnable concurrentOp,
+ @Nullable Function<Exception, Boolean> errValidator,
+ boolean rerunAtTheEnd
+ ) throws Exception {
+ CountDownLatch delProcInitLatch = new CountDownLatch(1);
+ CountDownLatch delProcProceedLatch = new CountDownLatch(1);
+
+ pluginProvider = new AbstractTestPluginProvider() {
+ @Override public String name() {
+ return "TestSnpMgrProvider";
+ }
+
+ @Override public <T> T createComponent(PluginContext ctx, Class<T>
cls) {
+ if (IgniteSnapshotManager.class.isAssignableFrom(cls)) {
+ return (T)new
IgniteSnapshotManager(((IgniteEx)ctx.grid()).context()) {
+ @Override public T2<Boolean, Boolean>
deleteLocalSnapshot(
+ SnapshotFileTree sft,
+ boolean scoped,
+ boolean ignoreErrs
+ ) {
+ delProcInitLatch.countDown();
+
+ try {
+
assertTrue(delProcProceedLatch.await(getTestTimeout(), TimeUnit.MILLISECONDS));
+ }
+ catch (InterruptedException e) {
+ throw new RuntimeException("Interrupted.", e);
+ }
+
+ return super.deleteLocalSnapshot(sft, scoped,
ignoreErrs);
+ }
+ };
+ }
+
+ return super.createComponent(ctx, cls);
+ }
+ };
+
+ prepareCluster.run();
+
+ IgniteInternalFuture<SnapshotDeleteProcessResult> delFut =
snp(grid(0)).deleteSnapshot(SNAPSHOT_NAME, null);
Review Comment:
no take into account FS sensitivity, thus, as i say before - global flag
looks very strange
##########
modules/control-utility/src/test/java/org/apache/ignite/util/GridCommandHandlerDeleteSnapshotTest.java:
##########
@@ -0,0 +1,240 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.util;
+
+import java.io.File;
+import java.nio.file.DirectoryStream;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Collection;
+import org.apache.ignite.IgniteDataStreamer;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.management.snapshot.SnapshotDeleteCommand;
+import org.apache.ignite.internal.util.typedef.F;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.junit.Test;
+import org.junit.runners.Parameterized.Parameter;
+import org.junit.runners.Parameterized.Parameters;
+
+import static java.nio.file.Files.newDirectoryStream;
+import static org.apache.ignite.cluster.ClusterState.ACTIVE;
+import static
org.apache.ignite.internal.commandline.CommandHandler.EXIT_CODE_OK;
+import static
org.apache.ignite.internal.processors.cache.persistence.snapshot.AbstractSnapshotSelfTest.snp;
+import static org.apache.ignite.testframework.GridTestUtils.waitForCondition;
+import static org.junit.Assume.assumeTrue;
+
+/** Test for the command '--snapshot delete'. */
+public class GridCommandHandlerDeleteSnapshotTest extends
GridCommandHandlerAbstractTest {
+ /** Flag to add an extra server node after the cluster start and the
snapshot creation. */
+ @Parameter(1)
+ public boolean extraServerNode;
+
+ /** */
+ @Parameter(2)
+ public boolean incremental;
+
+ /** */
+ @Parameter(3)
+ public boolean changeBaseline;
+
+ /** */
+ @Parameter(4)
+ public boolean customPath;
+
+ /** */
+ @Parameter(5)
+ public boolean separatedWorkDir;
+
+ /** */
+ @Parameters(name =
"client={0},useExtraNode={1},inc={2},chBaseln={3},cstSnpPath={4},ownWorkDir={5}")
Review Comment:
Common style is to use spase between enumeration i.e. : `"client={0},
useExtraNode={1}` and so on
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotRestoreProcess.java:
##########
@@ -659,6 +659,11 @@ private
IgniteInternalFuture<SnapshotRestoreOperationResponse> prepare(UUID igno
if (snpMgr.isSnapshotCreating())
throw new IgniteCheckedException(OP_REJECT_MSG + "A cluster
snapshot operation is in progress.");
+ if (snpMgr.isSnapshotDeleting(req.snapshotName(),
req.snapshotPath())) {
+ throw new IgniteCheckedException(OP_REJECT_MSG + "A snapshot
'" + req.snapshotName() +
Review Comment:
Message and exception if different, is it ok ?
from
`SnapshotCheckProcess#prepareAndCheckMetas`
```
if
(kctx.cache().context().snapshotMgr().isSnapshotDeleting(req.snapshotName(),
req.snapshotPath())) {
return new GridFinishedFuture<>(new
IgniteIllegalStateException("Snapshot '" + req.snapshotName()
+ "' is being deleted [req=" + req + ']'));
}
```
##########
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteClusterSnapshotSelfTest.java:
##########
@@ -615,6 +615,43 @@ public void testSnapshotExistsException() throws Exception
{
waitForEvents(EVT_CLUSTER_SNAPSHOT_STARTED,
EVT_CLUSTER_SNAPSHOT_FAILED);
}
+ /**
+ * Tests that snapshot create detects concurrent deletion, or detects
still existing snapshot or successfully
+ * proceeds if snapshot already deleted.
+ */
+ @Test
+ public void testConcurrentSnapshotDeleteOperation() throws Exception {
Review Comment:
This is not about concurrent snp deletion.
Look in `doConcurrentSnapshotOperationWhenDeletionIsActive`
u call :
`prepareCluster.run();`
then call
```
IgniteInternalFuture<SnapshotDeleteProcessResult> delFut =
snp(grid(0)).deleteSnapshot(SNAPSHOT_NAME, null);
assertTrue(delProcInitLatch.await(getTestTimeout(),
TimeUnit.MILLISECONDS));
```
but this deletion is blocked on latch:
`assertTrue(delProcProceedLatch.await(getTestTimeout(),
TimeUnit.MILLISECONDS));`
after u call :
`concurrentOp.run();`
and where here we can found a concurrent deletion ?
##########
modules/core/src/test/java/org/apache/ignite/testsuites/IgniteSnapshotTestSuite8.java:
##########
@@ -48,6 +50,8 @@ public static List<Class<?>> suite(Collection<Class>
ignoredTests) {
GridTestUtils.addTestIfNeeded(suite,
IgniteClusterSnapshotCheckTest.class, ignoredTests);
GridTestUtils.addTestIfNeeded(suite,
IgniteClusterSnapshotHandlerTest.class, ignoredTests);
+ GridTestUtils.addTestIfNeeded(suite,
IgniteClusterSnapshotDeleteRollingUpgradeTest.class, ignoredTests);
+ GridTestUtils.addTestIfNeeded(suite,
IgniteClusterSnapshotDeleteTest.class, ignoredTests);
Review Comment:
Seems you need to remove both of them ?
##########
modules/control-utility/src/test/java/org/apache/ignite/util/GridCommandHandlerDeleteSnapshotTest.java:
##########
@@ -0,0 +1,240 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.util;
+
+import java.io.File;
+import java.nio.file.DirectoryStream;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Collection;
+import org.apache.ignite.IgniteDataStreamer;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.management.snapshot.SnapshotDeleteCommand;
+import org.apache.ignite.internal.util.typedef.F;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.junit.Test;
+import org.junit.runners.Parameterized.Parameter;
+import org.junit.runners.Parameterized.Parameters;
+
+import static java.nio.file.Files.newDirectoryStream;
+import static org.apache.ignite.cluster.ClusterState.ACTIVE;
+import static
org.apache.ignite.internal.commandline.CommandHandler.EXIT_CODE_OK;
+import static
org.apache.ignite.internal.processors.cache.persistence.snapshot.AbstractSnapshotSelfTest.snp;
+import static org.apache.ignite.testframework.GridTestUtils.waitForCondition;
+import static org.junit.Assume.assumeTrue;
+
+/** Test for the command '--snapshot delete'. */
+public class GridCommandHandlerDeleteSnapshotTest extends
GridCommandHandlerAbstractTest {
+ /** Flag to add an extra server node after the cluster start and the
snapshot creation. */
+ @Parameter(1)
+ public boolean extraServerNode;
+
+ /** */
+ @Parameter(2)
+ public boolean incremental;
+
+ /** */
+ @Parameter(3)
+ public boolean changeBaseline;
+
+ /** */
+ @Parameter(4)
+ public boolean customPath;
+
+ /** */
+ @Parameter(5)
+ public boolean separatedWorkDir;
+
+ /** */
+ @Parameters(name =
"client={0},useExtraNode={1},inc={2},chBaseln={3},cstSnpPath={4},ownWorkDir={5}")
+ public static Collection<?> parameters() {
+ return GridTestUtils.cartesianProduct(
+ commandHandlers(),
+ F.asList(false, true), // Use or not extra server node after the
cluster start and the snapshot creation;
+ F.asList(false, true), // Add incremental snapshot;
+ F.asList(false, true), // Change baseline;
+ F.asList(false, true), // Use custom snapshot path;
+ F.asList(false, true) // Separated (own) work directory.
+ );
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void afterTest() throws Exception {
+ super.afterTest();
+
+ stopAllGrids();
+
+ cleanPersistenceDir();
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void beforeTest() throws Exception {
+ super.beforeTest();
+
+ /** Handy if test running is interrupted and {@link #afterTest()}
isn't invoked. */
+ cleanPersistenceDir();
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void cleanPersistenceDir() throws Exception {
+ super.cleanPersistenceDir();
+
+ // Also cleans separated snapshot working directories and custom
snapshot paths.
+ try (DirectoryStream<Path> files =
newDirectoryStream(Paths.get(U.defaultWorkDirectory()))) {
+ for (Path path : files)
+ U.delete(path);
+ }
+ }
+
+ /** {@inheritDoc} */
+ @Override protected IgniteConfiguration getConfiguration(String
igniteInstanceName) throws Exception {
+ IgniteConfiguration cfg = super.getConfiguration(igniteInstanceName);
+
+ if (separatedWorkDir)
+ cfg.setWorkDirectory(new File(U.defaultWorkDirectory(),
igniteInstanceName).getAbsolutePath());
+
+ return cfg;
+ }
+
+ /** */
+ @Test
+ public void testSnapshotDelete() throws Exception {
+ // A custom snapshot path actually puts snapshots in a shared
directory. This skews the results when dedicated
+ // work directories are set.
+ assumeTrue(!customPath || !separatedWorkDir);
+
+ int entriesCnt = 100;
+ int initNodes = 3;
+
+ walCompactionEnabled(incremental);
+
+ IgniteEx ig = (IgniteEx)startGridsMultiThreaded(initNodes);
+
+ if (changeBaseline) {
+ ig.cluster().baselineAutoAdjustEnabled(false);
+
+ ig.cluster().setBaselineTopology(ig.cluster().topologyVersion());
+ }
+
+ ig.cluster().state(ACTIVE);
+
+ createCacheAndPreload(ig, entriesCnt);
+
+ File cstSnpsRoot = customPath
+ ? new
File(grid(0).context().pdsFolderResolver().fileTree().snapshotsRoot(),
"ex_snapshots")
+ : null;
+ File snpDir = new File(customPath ? cstSnpsRoot :
ig.context().pdsFolderResolver().fileTree().snapshotsRoot(), "testSnapshot");
+
+ snp(ig).createSnapshot("testSnapshot", customPath ?
cstSnpsRoot.getAbsolutePath() : null, false, false)
+ .get(getTestTimeout());
+
+ if (incremental) {
+ for (int i = 0; i < 3; ++i) {
+ int dataIdx = entriesCnt + entriesCnt / 4 * i;
+
+ try (IgniteDataStreamer<Object, Object> streamer =
ig.dataStreamer(DEFAULT_CACHE_NAME)) {
+ for (int d = dataIdx; d < dataIdx + entriesCnt / 4; ++d)
+ streamer.addData(d, d);
+ }
+
+ snp(ig).createSnapshot("testSnapshot", customPath ?
cstSnpsRoot.getAbsolutePath() : null, true, false)
+ .get(getTestTimeout());
+ }
+ }
+
+ String baselineGone = null;
+
+ // Optionally restarts with the same servers number, but changed
baseline. The snapshot is kept on the same
+ // previous nodes independently of the baseline.
+ if (changeBaseline) {
+ baselineGone = grid(initNodes - 1).name();
+
+ ig.destroyCache(DEFAULT_CACHE_NAME);
+ awaitPartitionMapExchange();
+
+ stopAllGrids();
+
+ ig = (IgniteEx)startGridsMultiThreaded(initNodes - 1);
+
+ ig.cluster().setBaselineTopology(ig.cluster().topologyVersion());
+
+ startGrid(initNodes - 1);
+
+ assertEquals(initNodes - 1,
ig.cluster().currentBaselineTopology().size());
+ assertEquals(initNodes, ig.cluster().nodes().size());
+ }
+
+ // Optionally adds extra server.
+ if (extraServerNode)
+ startGrid(initNodes);
+
+ injectTestSystemOut();
+
+ // Tests missing snapshot deletion.
+ if (customPath) {
+ assertEquals(EXIT_CODE_OK,
execute(newCommandHandler(createTestLogger()), "--snapshot", "delete", "--src",
+ cstSnpsRoot.getAbsolutePath(), "wrongSnapshot"));
+
+ assertEquals(EXIT_CODE_OK,
execute(newCommandHandler(createTestLogger()), "--snapshot", "delete", "--src",
+ cstSnpsRoot.getAbsolutePath() + "_wrongPath", "testSnapshot"));
+ }
+ else
+ assertEquals(EXIT_CODE_OK,
execute(newCommandHandler(createTestLogger()), "--snapshot", "delete",
"wrongSnapshot"));
+
+ String out = testOut.toString();
+
+ assertFalse(out.contains(SnapshotDeleteCommand.REMOVED_PREF));
+ assertFalse(out.contains(SnapshotDeleteCommand.NODE_NOT_FOUND_PREF));
+ assertTrue(out.contains(SnapshotDeleteCommand.NOT_FOUND));
+
+ testOut.reset();
+ assertTrue(testOut.toString().isEmpty());
+
+ if (customPath) {
+ assertEquals(EXIT_CODE_OK,
execute(newCommandHandler(createTestLogger()), "--snapshot", "delete", "--src",
+ cstSnpsRoot.getAbsolutePath(), "testSnapshot"));
+ }
+ else
+ assertEquals(EXIT_CODE_OK,
execute(newCommandHandler(createTestLogger()), "--snapshot", "delete",
"testSnapshot"));
+
+ out = testOut.toString();
+
+ if (changeBaseline)
+ assertTrue(out.contains(SnapshotDeleteCommand.MISSING_BASELINES +
baselineGone));
+
+ if (separatedWorkDir) {
+ // When the nodes use own separated work directory, we expect a
strict result.
+ assertTrue(out.contains(SnapshotDeleteCommand.REMOVED_PREF));
+
+ if (extraServerNode)
+
assertTrue(out.contains(SnapshotDeleteCommand.NODE_NOT_FOUND_PREF));
+ }
+ else {
+ // When nodes use a shared work directory, there is a race for the
delete operation. One node can get faster
+ // than others and remove snapshot completely quickly. The others
might not find snapshot files. We can be
Review Comment:
completely quickly - errorneous form, change it
##########
modules/control-utility/src/test/java/org/apache/ignite/util/GridCommandHandlerDeleteSnapshotTest.java:
##########
@@ -0,0 +1,240 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.util;
+
+import java.io.File;
+import java.nio.file.DirectoryStream;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Collection;
+import org.apache.ignite.IgniteDataStreamer;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.management.snapshot.SnapshotDeleteCommand;
+import org.apache.ignite.internal.util.typedef.F;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.junit.Test;
+import org.junit.runners.Parameterized.Parameter;
+import org.junit.runners.Parameterized.Parameters;
+
+import static java.nio.file.Files.newDirectoryStream;
+import static org.apache.ignite.cluster.ClusterState.ACTIVE;
+import static
org.apache.ignite.internal.commandline.CommandHandler.EXIT_CODE_OK;
+import static
org.apache.ignite.internal.processors.cache.persistence.snapshot.AbstractSnapshotSelfTest.snp;
+import static org.apache.ignite.testframework.GridTestUtils.waitForCondition;
+import static org.junit.Assume.assumeTrue;
+
+/** Test for the command '--snapshot delete'. */
+public class GridCommandHandlerDeleteSnapshotTest extends
GridCommandHandlerAbstractTest {
+ /** Flag to add an extra server node after the cluster start and the
snapshot creation. */
+ @Parameter(1)
+ public boolean extraServerNode;
+
+ /** */
+ @Parameter(2)
+ public boolean incremental;
+
+ /** */
+ @Parameter(3)
+ public boolean changeBaseline;
+
+ /** */
+ @Parameter(4)
+ public boolean customPath;
+
+ /** */
+ @Parameter(5)
+ public boolean separatedWorkDir;
+
+ /** */
+ @Parameters(name =
"client={0},useExtraNode={1},inc={2},chBaseln={3},cstSnpPath={4},ownWorkDir={5}")
+ public static Collection<?> parameters() {
+ return GridTestUtils.cartesianProduct(
+ commandHandlers(),
+ F.asList(false, true), // Use or not extra server node after the
cluster start and the snapshot creation;
+ F.asList(false, true), // Add incremental snapshot;
+ F.asList(false, true), // Change baseline;
+ F.asList(false, true), // Use custom snapshot path;
+ F.asList(false, true) // Separated (own) work directory.
+ );
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void afterTest() throws Exception {
+ super.afterTest();
+
+ stopAllGrids();
+
+ cleanPersistenceDir();
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void beforeTest() throws Exception {
+ super.beforeTest();
+
+ /** Handy if test running is interrupted and {@link #afterTest()}
isn't invoked. */
+ cleanPersistenceDir();
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void cleanPersistenceDir() throws Exception {
+ super.cleanPersistenceDir();
+
+ // Also cleans separated snapshot working directories and custom
snapshot paths.
+ try (DirectoryStream<Path> files =
newDirectoryStream(Paths.get(U.defaultWorkDirectory()))) {
+ for (Path path : files)
+ U.delete(path);
+ }
+ }
+
+ /** {@inheritDoc} */
+ @Override protected IgniteConfiguration getConfiguration(String
igniteInstanceName) throws Exception {
+ IgniteConfiguration cfg = super.getConfiguration(igniteInstanceName);
+
+ if (separatedWorkDir)
+ cfg.setWorkDirectory(new File(U.defaultWorkDirectory(),
igniteInstanceName).getAbsolutePath());
+
+ return cfg;
+ }
+
+ /** */
+ @Test
+ public void testSnapshotDelete() throws Exception {
+ // A custom snapshot path actually puts snapshots in a shared
directory. This skews the results when dedicated
+ // work directories are set.
+ assumeTrue(!customPath || !separatedWorkDir);
+
+ int entriesCnt = 100;
+ int initNodes = 3;
+
+ walCompactionEnabled(incremental);
+
+ IgniteEx ig = (IgniteEx)startGridsMultiThreaded(initNodes);
+
+ if (changeBaseline) {
+ ig.cluster().baselineAutoAdjustEnabled(false);
+
+ ig.cluster().setBaselineTopology(ig.cluster().topologyVersion());
+ }
+
+ ig.cluster().state(ACTIVE);
+
+ createCacheAndPreload(ig, entriesCnt);
+
+ File cstSnpsRoot = customPath
+ ? new
File(grid(0).context().pdsFolderResolver().fileTree().snapshotsRoot(),
"ex_snapshots")
+ : null;
+ File snpDir = new File(customPath ? cstSnpsRoot :
ig.context().pdsFolderResolver().fileTree().snapshotsRoot(), "testSnapshot");
+
+ snp(ig).createSnapshot("testSnapshot", customPath ?
cstSnpsRoot.getAbsolutePath() : null, false, false)
+ .get(getTestTimeout());
+
+ if (incremental) {
+ for (int i = 0; i < 3; ++i) {
Review Comment:
```suggestion
for (int i = 0; i < initNodes; ++i) {
```
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/SnapshotCheckProcess.java:
##########
@@ -131,7 +133,7 @@ private IgniteInternalFuture<?>
reduceValidatePartsAndFinish(
return new GridFinishedFuture<>();
try {
- contexts.remove(ctx.req.snapshotName());
+ contexts.remove(ctx.req.snapshotName().toLowerCase(Locale.ROOT));
Review Comment:
If we have no appropriate tests for such a case - no need to change it and
just reject Copilot comment. If i`m right I suggest to revert this change and
fill appropriate issue.
##########
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/EncryptedSnapshotTest.java:
##########
@@ -232,6 +232,8 @@ public void testValidatingSnapshotFailsWithNoEncryption()
throws Exception {
assertTrue(snpDir.renameTo(tmpSnpDir));
+ removeAllWorkingDirectories = false;
Review Comment:
global abstract test flag for only one redefinition looks weird
##########
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/AbstractSnapshotSelfTest.java:
##########
@@ -148,6 +153,9 @@ public abstract class AbstractSnapshotSelfTest extends
GridCommonAbstractTest {
protected static final int PAGE_SIZE =
IgniteSystemProperties.getInteger(IGNITE_DEFAULT_DATA_STORAGE_PAGE_SIZE,
DFLT_PAGE_SIZE);
+ /** */
+ protected static boolean caseInsensitiveFs;
Review Comment:
I don\`t understand why we need this flag globally ?
Look, in my vision all we need for FS sensitivity tests (in pseudo code) is
one test like :
1. create 'snap'
2. create 'SNAP' (in case insensitive FS - expect exception ?)
3. delete 'snap'
4. delete 'SNAP' (in case insensitive FS - expect exception ?)
all other tests no need to know about such a flag in my opinion.
##########
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/EncryptedSnapshotTest.java:
##########
@@ -232,6 +232,8 @@ public void testValidatingSnapshotFailsWithNoEncryption()
throws Exception {
assertTrue(snpDir.renameTo(tmpSnpDir));
+ removeAllWorkingDirectories = false;
Review Comment:
I suggest to remove this flag and just call : `cleanPersistenceDir(false);`
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IncrementalSnapshotProcessor.java:
##########
@@ -138,7 +138,8 @@ void process(
WALRecord rec = walRec.getValue();
- 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()))
{
Review Comment:
If we have no appropriate tests for such a case - no need to change it and
just reject Copilot comment. If i\`m right I suggest to revert this change and
fill appropriate issue.
##########
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/persistence/snapshot/IgniteClusterSnapshotRestoreSelfTest.java:
##########
@@ -333,6 +333,47 @@ public void testCreateSnapshotDuringRestore() throws
Exception {
assertCacheKeys(ignite.cache(DEFAULT_CACHE_NAME), CACHE_KEYS_RANGE);
}
+ /** Tests that snapshot restore is declined when the same snapshot is
being deleted. */
+ @Test
+ public void testConcurrentSnapshotDeleteAndRestoreOperations() throws
Exception {
+ String snapshotName = caseInsensitiveFs ? SNAPSHOT_NAME.toLowerCase()
: SNAPSHOT_NAME;
+
+ doConcurrentSnapshotOperationWhenDeletionIsActive(
+ () -> startGridsWithSnapshot(3, CACHE_KEYS_RANGE),
+ () -> snp(grid(2)).restoreSnapshot(snapshotName, null).get(),
+ e -> e.getMessage().contains("Snapshot '%s' is being
deleted".formatted(snapshotName)),
+ true
+ );
+ }
+
+ /** */
+ @Test
+ public void
testConcurrentSnapshotDeleteAndRestoreOperationsWithDifferentPath() throws
Exception {
+ String snpPath = new File(U.defaultWorkDirectory(),
"ex_snapshots").getAbsolutePath();
+
+ doConcurrentSnapshotOperationWhenDeletionIsActive(
+ () -> {
+ startGridsWithSnapshot(3, CACHE_KEYS_RANGE);
+
+ grid(0).createCache(DEFAULT_CACHE_NAME);
+
+ try (var ds = grid(0).dataStreamer(DEFAULT_CACHE_NAME)) {
Review Comment:
**var** usage not acceptable
--
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]
