pauloricardomg commented on code in PR #1781:
URL: https://github.com/apache/cassandra/pull/1781#discussion_r948487090
##########
src/java/org/apache/cassandra/service/snapshot/SnapshotLoader.java:
##########
@@ -79,15 +79,19 @@ public SnapshotLoader(Directories directories)
this(directories.getCFDirectories().stream().map(File::toPath).collect(Collectors.toList()));
}
- public Set<TableSnapshot> loadSnapshots()
+ public Set<TableSnapshot> loadSnapshots(String keyspace)
{
+ int maxDepth = keyspace == null ? 5 : 4;
Review Comment:
Can you elaborate why this is needed?
##########
src/java/org/apache/cassandra/db/Keyspace.java:
##########
@@ -299,15 +301,34 @@ public boolean snapshotExists(String snapshotName)
/**
* Clear all the snapshots for a given keyspace.
*
- * @param snapshotName the user supplied snapshot name. It empty or null,
+ * @param snapshotName the user supplied snapshot name. If empty or null,
* all the snapshots will be cleaned
+ * @param keyspace keyspace to remove snapshots for
*/
public static void clearSnapshot(String snapshotName, String keyspace)
{
+ Set<TableSnapshot> snapshotsToClear = new
SnapshotLoader().loadSnapshots(keyspace)
+ .stream()
+ .filter(ts
-> {
+ if
(snapshotName == null || snapshotName.isEmpty())
+
return true;
+ return
ts.getTag().equals(snapshotName);
+ })
+ .filter(ts
-> {
+ if
(snapshotName != null && !snapshotName.isEmpty() && ts.isEphemeral())
+
logger.info("Skipping deletion of ephemeral snapshot '{}' in keyspace {}. " +
+
"Ephemeral snapshots are not removable by a user.",
+
snapshotName, keyspace);
+
+ return
!ts.isEphemeral();
+ })
+
.collect(Collectors.toSet());
+
RateLimiter clearSnapshotRateLimiter =
DatabaseDescriptor.getSnapshotRateLimiter();
- List<File> tableDirectories =
Directories.getKSChildDirectories(keyspace);
- Directories.clearSnapshot(snapshotName, tableDirectories,
clearSnapshotRateLimiter);
+ for (TableSnapshot snapshot : snapshotsToClear)
+ for (File snapshotDirectory : snapshot.getDirectories())
+ Directories.removeSnapshotDirectory(clearSnapshotRateLimiter,
snapshotDirectory);
Review Comment:
It seems like this logic is duplicated on `SnapshotManager.clearSnapshot`.
We also need to ensure expiring snapshots are properly removed from
`SnapshotManager`. I think this logic is in `Keyspace` for historical reasons,
now we can move it to `StorageService` so we can properly centralize snapshot
clearing via `SnapshotManager. I created [this
PR](https://github.com/instaclustr/cassandra/pull/47) moving
`Keyspace.clearSnapshot` to `StorageService.clearKeyspaceSnapshot`. Let me know
what do you think.
##########
src/java/org/apache/cassandra/db/Keyspace.java:
##########
@@ -299,15 +301,34 @@ public boolean snapshotExists(String snapshotName)
/**
* Clear all the snapshots for a given keyspace.
*
- * @param snapshotName the user supplied snapshot name. It empty or null,
+ * @param snapshotName the user supplied snapshot name. If empty or null,
* all the snapshots will be cleaned
+ * @param keyspace keyspace to remove snapshots for
*/
public static void clearSnapshot(String snapshotName, String keyspace)
{
+ Set<TableSnapshot> snapshotsToClear = new
SnapshotLoader().loadSnapshots(keyspace)
+ .stream()
+ .filter(ts
-> {
+ if
(snapshotName == null || snapshotName.isEmpty())
Review Comment:
I simplified this filtering a bit
[here](https://github.com/instaclustr/cassandra/pull/47/files#diff-9bf2c26bc294ef9085e16bf287490223665eaa2eb8ec24bcf5bd8653c713644bR4213)
by encapsulating the filtering logic on
[TableSnapshot.shouldClearSnapshot](https://github.com/instaclustr/cassandra/pull/47/files#diff-7d6d1bafcad95c5715c91c9065a4a8c58c3d5c98d0699d9c913717f5c0086bb7R328).
Let me know what do you think.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
