You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cassandra.apache.org by sm...@apache.org on 2025/01/16 13:57:04 UTC

(cassandra) branch trunk updated (33feca3d84 -> 407dbacb0a)

This is an automated email from the ASF dual-hosted git repository.

smiklosovic pushed a change to branch trunk
in repository https://gitbox.apache.org/repos/asf/cassandra.git


    from 33feca3d84 Merge branch 'cassandra-5.0' into trunk
     new b15a1f04b6 Propagate true size of snapshot in SnapshotDetailsTabularData to not call JMX twice in nodetool listsnapshots
     new b4e9399865 Create manifest upon loading where it does not exist or enrich it
     new 407dbacb0a Enable filtering of snapshots on keyspace, table and snapshot name in nodetool listsnapshots

The 3 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails.  The revisions
listed as "add" were already present in the repository and have only
been added to this reference.


Summary of changes:
 CHANGES.txt                                        |   5 +-
 .../config/CassandraRelevantProperties.java        |   6 +
 .../snapshot/SnapshotDetailsTabularData.java       |  25 ++-
 .../service/snapshot/SnapshotManager.java          |   9 +
 .../service/snapshot/SnapshotManagerMBean.java     |  26 ++-
 .../cassandra/service/snapshot/TableSnapshot.java  | 146 +++++++++++-
 .../cassandra/tools/nodetool/ListSnapshots.java    |  42 +++-
 .../cassandra/distributed/test/SnapshotsTest.java  | 245 ++++++++++++++++++++-
 .../apache/cassandra/db/ColumnFamilyStoreTest.java |  62 +++---
 .../org/apache/cassandra/db/DirectoriesTest.java   |  10 +-
 .../service/snapshot/SnapshotLoaderTest.java       |  57 +++--
 11 files changed, 557 insertions(+), 76 deletions(-)


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org


(cassandra) 01/03: Propagate true size of snapshot in SnapshotDetailsTabularData to not call JMX twice in nodetool listsnapshots

Posted by sm...@apache.org.
This is an automated email from the ASF dual-hosted git repository.

smiklosovic pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/cassandra.git

commit b15a1f04b61e11bc3b9719c06bffb9fa672a25cd
Author: Stefan Miklosovic <sm...@apache.org>
AuthorDate: Wed Dec 18 11:58:30 2024 +0100

    Propagate true size of snapshot in SnapshotDetailsTabularData to not call JMX twice in nodetool listsnapshots
    
    This patch also opportunistically adds a new MBean method for SnapshotManager to
    return true snapshot size for a particular snapshot.
    
    patch by Stefan Miklosovic; reviewed by Maxim Muzafarov, Bernardo Botella for CASSANDRA-20149
---
 CHANGES.txt                                        |  1 +
 .../snapshot/SnapshotDetailsTabularData.java       | 25 +++++----
 .../service/snapshot/SnapshotManager.java          |  9 ++++
 .../service/snapshot/SnapshotManagerMBean.java     | 12 ++++-
 .../cassandra/tools/nodetool/ListSnapshots.java    | 21 ++++----
 .../apache/cassandra/db/ColumnFamilyStoreTest.java | 62 ++++++++++++----------
 6 files changed, 84 insertions(+), 46 deletions(-)

diff --git a/CHANGES.txt b/CHANGES.txt
index 02a0be0571..b20e0f8b1f 100644
--- a/CHANGES.txt
+++ b/CHANGES.txt
@@ -1,4 +1,5 @@
 5.1
+ * Propagate true size of snapshot in SnapshotDetailsTabularData to not call JMX twice in nodetool listsnapshots (CASSANDRA-20149)
  * Implementation of CEP-43 (CASSANDRA-19964)
  * Periodically disconnect roles that are revoked or have LOGIN=FALSE set (CASSANDRA-19385)
  * AST library for CQL-based fuzz tests (CASSANDRA-20198)
diff --git a/src/java/org/apache/cassandra/service/snapshot/SnapshotDetailsTabularData.java b/src/java/org/apache/cassandra/service/snapshot/SnapshotDetailsTabularData.java
index 23c84e5056..3d9609f2e7 100644
--- a/src/java/org/apache/cassandra/service/snapshot/SnapshotDetailsTabularData.java
+++ b/src/java/org/apache/cassandra/service/snapshot/SnapshotDetailsTabularData.java
@@ -18,13 +18,18 @@
 package org.apache.cassandra.service.snapshot;
 
 import java.util.Set;
-import javax.management.openmbean.*;
+import javax.management.openmbean.CompositeDataSupport;
+import javax.management.openmbean.CompositeType;
+import javax.management.openmbean.OpenDataException;
+import javax.management.openmbean.OpenType;
+import javax.management.openmbean.SimpleType;
+import javax.management.openmbean.TabularDataSupport;
+import javax.management.openmbean.TabularType;
 
 import org.apache.cassandra.io.util.FileUtils;
 
 public class SnapshotDetailsTabularData
 {
-
     private static final String[] ITEM_NAMES = new String[]{"Snapshot name",
                                                             "Keyspace name",
                                                             "Column family name",
@@ -32,7 +37,8 @@ public class SnapshotDetailsTabularData
                                                             "Size on disk",
                                                             "Creation time",
                                                             "Expiration time",
-                                                            "Ephemeral"};
+                                                            "Ephemeral",
+                                                            "Raw true size"};
 
     private static final String[] ITEM_DESCS = new String[]{"snapshot_name",
                                                             "keyspace_name",
@@ -41,7 +47,8 @@ public class SnapshotDetailsTabularData
                                                             "TotalDiskSpaceUsed",
                                                             "created_at",
                                                             "expires_at",
-                                                            "ephemeral"};
+                                                            "ephemeral",
+                                                            "raw_true_disk_space_used"};
 
     private static final String TYPE_NAME = "SnapshotDetails";
 
@@ -57,7 +64,7 @@ public class SnapshotDetailsTabularData
     {
         try
         {
-            ITEM_TYPES = new OpenType[]{ SimpleType.STRING, SimpleType.STRING, SimpleType.STRING, SimpleType.STRING, SimpleType.STRING, SimpleType.STRING, SimpleType.STRING, SimpleType.STRING };
+            ITEM_TYPES = new OpenType[]{ SimpleType.STRING, SimpleType.STRING, SimpleType.STRING, SimpleType.STRING, SimpleType.STRING, SimpleType.STRING, SimpleType.STRING, SimpleType.STRING, SimpleType.LONG };
 
             COMPOSITE_TYPE = new CompositeType(TYPE_NAME, ROW_DESC, ITEM_NAMES, ITEM_DESCS, ITEM_TYPES);
 
@@ -74,14 +81,14 @@ public class SnapshotDetailsTabularData
     {
         try
         {
-            final String totalSize = FileUtils.stringifyFileSize(details.computeSizeOnDiskBytes());
+            String totalSize = FileUtils.stringifyFileSize(details.computeSizeOnDiskBytes());
             long trueSizeBytes = details.computeTrueSizeBytes(files);
-            final String liveSize =  FileUtils.stringifyFileSize(details.computeTrueSizeBytes());
+            String liveSize =  FileUtils.stringifyFileSize(trueSizeBytes);
             String createdAt = safeToString(details.getCreatedAt());
             String expiresAt = safeToString(details.getExpiresAt());
             String ephemeral = Boolean.toString(details.isEphemeral());
             result.put(new CompositeDataSupport(COMPOSITE_TYPE, ITEM_NAMES,
-                                                new Object[]{ details.getTag(), details.getKeyspaceName(), details.getTableName(), liveSize, totalSize, createdAt, expiresAt, ephemeral }));
+                                                new Object[]{ details.getTag(), details.getKeyspaceName(), details.getTableName(), liveSize, totalSize, createdAt, expiresAt, ephemeral, trueSizeBytes }));
         }
         catch (OpenDataException e)
         {
@@ -93,4 +100,4 @@ public class SnapshotDetailsTabularData
     {
         return object == null ? null : object.toString();
     }
-}
\ No newline at end of file
+}
diff --git a/src/java/org/apache/cassandra/service/snapshot/SnapshotManager.java b/src/java/org/apache/cassandra/service/snapshot/SnapshotManager.java
index bea0c04669..f7fa1e4ece 100644
--- a/src/java/org/apache/cassandra/service/snapshot/SnapshotManager.java
+++ b/src/java/org/apache/cassandra/service/snapshot/SnapshotManager.java
@@ -471,6 +471,15 @@ public class SnapshotManager implements SnapshotManagerMBean, INotificationConsu
         return new TrueSnapshotSizeTask(this, s -> s.getKeyspaceName().equals(keyspace) && s.getTableName().equals(table)).call();
     }
 
+    @Override
+    public long getTrueSnapshotsSize(String keyspace, String table, String snapshotName)
+    {
+        return new TrueSnapshotSizeTask(this,
+                                        s -> s.getKeyspaceName().equals(keyspace)
+                                             && s.getTableName().equals(table)
+                                             && s.getTag().equals(snapshotName)).call();
+    }
+
     @Override
     public void setSnapshotLinksPerSecond(long throttle)
     {
diff --git a/src/java/org/apache/cassandra/service/snapshot/SnapshotManagerMBean.java b/src/java/org/apache/cassandra/service/snapshot/SnapshotManagerMBean.java
index 2d69a7ca97..214adcc445 100644
--- a/src/java/org/apache/cassandra/service/snapshot/SnapshotManagerMBean.java
+++ b/src/java/org/apache/cassandra/service/snapshot/SnapshotManagerMBean.java
@@ -78,7 +78,7 @@ public interface SnapshotManagerMBean
     long getTrueSnapshotsSize(String keyspace);
 
     /**
-     * Get the true size take by all snapshots in given keyspace and table.
+     * Get the true size taken by all snapshots in given keyspace and table.
      *
      * @param keyspace keyspace to get true size of all snapshots of
      * @param table table in a keyspace to get true size of all snapshots of
@@ -86,6 +86,16 @@ public interface SnapshotManagerMBean
      */
     long getTrueSnapshotsSize(String keyspace, String table);
 
+    /**
+     * Get the true size of a snapshot in given keyspace and table.
+     *
+     * @param keyspace keyspace to get true size of all snapshots of
+     * @param table table in a keyspace to get true size of all snapshots of
+     * @param snapshotName name of snapshot in given keyspace and table to get true size of
+     * @return true size of all snapshots in given keyspace and table
+     */
+    long getTrueSnapshotsSize(String keyspace, String table, String snapshotName);
+
     /**
      * Set the current hardlink-per-second throttle for snapshots
      * A setting of zero indicates no throttling
diff --git a/src/java/org/apache/cassandra/tools/nodetool/ListSnapshots.java b/src/java/org/apache/cassandra/tools/nodetool/ListSnapshots.java
index 6a1c71d386..e1b61d4b51 100644
--- a/src/java/org/apache/cassandra/tools/nodetool/ListSnapshots.java
+++ b/src/java/org/apache/cassandra/tools/nodetool/ListSnapshots.java
@@ -25,7 +25,6 @@ import java.util.Set;
 import javax.management.openmbean.TabularData;
 
 import io.airlift.airline.Command;
-
 import io.airlift.airline.Option;
 import org.apache.cassandra.io.util.FileUtils;
 import org.apache.cassandra.tools.NodeProbe;
@@ -64,15 +63,17 @@ public class ListSnapshots extends NodeToolCmd
                 return;
             }
 
-            final long trueSnapshotsSize = probe.trueSnapshotsSize();
             TableBuilder table = new TableBuilder();
             // display column names only once
-            final List<String> indexNames = snapshotDetails.entrySet().iterator().next().getValue().getTabularType().getIndexNames();
+            List<String> indexNames = snapshotDetails.entrySet().iterator().next().getValue().getTabularType().getIndexNames();
+            indexNames.subList(0, indexNames.size() - 1);
 
             if (includeEphemeral)
-                table.add(indexNames.toArray(new String[indexNames.size()]));
-            else
                 table.add(indexNames.subList(0, indexNames.size() - 1).toArray(new String[indexNames.size() - 1]));
+            else
+                table.add(indexNames.subList(0, indexNames.size() - 2).toArray(new String[indexNames.size() - 2]));
+
+            long totalTrueDiskSize = 0;
 
             for (final Map.Entry<String, TabularData> snapshotDetail : snapshotDetails.entrySet())
             {
@@ -81,18 +82,20 @@ public class ListSnapshots extends NodeToolCmd
                 {
                     final List<?> value = (List<?>) eachValue;
                     if (includeEphemeral)
-                        table.add(value.toArray(new String[value.size()]));
-                    else
                         table.add(value.subList(0, value.size() - 1).toArray(new String[value.size() - 1]));
+                    else
+                        table.add(value.subList(0, value.size() - 2).toArray(new String[value.size() - 2]));
+
+                    totalTrueDiskSize += (Long) value.get(value.size() - 1);
                 }
             }
             table.printTo(out);
 
-            out.println("\nTotal TrueDiskSpaceUsed: " + FileUtils.stringifyFileSize(trueSnapshotsSize) + '\n');
+            out.println("\nTotal TrueDiskSpaceUsed: " + FileUtils.stringifyFileSize(totalTrueDiskSize) + '\n');
         }
         catch (Exception e)
         {
             throw new RuntimeException("Error during list snapshot", e);
         }
     }
-}
\ No newline at end of file
+}
diff --git a/test/unit/org/apache/cassandra/db/ColumnFamilyStoreTest.java b/test/unit/org/apache/cassandra/db/ColumnFamilyStoreTest.java
index ba08437cd6..7f5b28c759 100644
--- a/test/unit/org/apache/cassandra/db/ColumnFamilyStoreTest.java
+++ b/test/unit/org/apache/cassandra/db/ColumnFamilyStoreTest.java
@@ -382,18 +382,21 @@ public class ColumnFamilyStoreTest
 
         assertThat(cfs.trueSnapshotsSize()).isZero();
 
-        SnapshotManager.instance.takeSnapshot("snapshot_without_index", cfs.getKeyspaceTableName());
+        SnapshotManager.instance.takeSnapshot("snapshot0_without_index", cfs.getKeyspaceTableName());
 
-        long firstSnapshotsSize = cfs.trueSnapshotsSize();
+        long sizeOfOneSnapshot = cfs.trueSnapshotsSize();
+        // test that true snapshot size computed from manager is same as true snapshot size from cfs with one snapshot
+        long trueSnapshotSizeViaManagerWithoutIndex = SnapshotManager.instance.getTrueSnapshotsSize(cfs.getKeyspaceName(), cfs.getTableName(), "snapshot0_without_index");
+        Assert.assertEquals(sizeOfOneSnapshot, trueSnapshotSizeViaManagerWithoutIndex);
         Map<String, TableSnapshot> listedSnapshots = Util.listSnapshots(cfs);
-        assertThat(firstSnapshotsSize).isPositive();
+        assertThat(sizeOfOneSnapshot).isPositive();
         assertThat(listedSnapshots.size()).isEqualTo(1);
-        assertThat(listedSnapshots.get("snapshot_without_index")).isNotNull();
-        long withoutIndexSize = listedSnapshots.get("snapshot_without_index").computeSizeOnDiskBytes();
-        long withoutIndexTrueSize = listedSnapshots.get("snapshot_without_index").computeTrueSizeBytes();
+        assertThat(listedSnapshots.get("snapshot0_without_index")).isNotNull();
+        long withoutIndexSize = listedSnapshots.get("snapshot0_without_index").computeSizeOnDiskBytes();
+        long withoutIndexTrueSize = listedSnapshots.get("snapshot0_without_index").computeTrueSizeBytes();
 
         assertThat(withoutIndexSize).isGreaterThan(withoutIndexTrueSize);
-        assertEquals(firstSnapshotsSize, withoutIndexTrueSize);
+        assertEquals(sizeOfOneSnapshot, withoutIndexTrueSize);
 
         // add index, trueSnapshotSize should reflect that
         ColumnIdentifier col = ColumnIdentifier.getInterned("val", true);
@@ -414,36 +417,41 @@ public class ColumnFamilyStoreTest
 
         rebuildIndices(cfs);
 
-        SnapshotManager.instance.takeSnapshot("snapshot_with_index", new HashMap<>(), cfs.getKeyspaceTableName());
+        SnapshotManager.instance.takeSnapshot("snapshot1_with_index", new HashMap<>(), cfs.getKeyspaceTableName());
 
-        long secondSnapshotSize = cfs.trueSnapshotsSize();
-        Map<String, TableSnapshot> secondListedSnapshots = Util.listSnapshots(cfs);
-        assertThat(secondSnapshotSize).isPositive();
-        assertThat(secondSnapshotSize).isGreaterThan(firstSnapshotsSize);
+        long sizeOfTwoSnapshots = cfs.trueSnapshotsSize();
+        long trueSnapshotSizeViaManagerWithIndex = SnapshotManager.instance.getTrueSnapshotsSize(cfs.getKeyspaceName(), cfs.getTableName(), "snapshot1_with_index");
 
-        assertThat(secondListedSnapshots.size()).isEqualTo(2);
-        assertThat(secondListedSnapshots.get("snapshot_with_index")).isNotNull();
-        assertThat(secondListedSnapshots.get("snapshot_without_index")).isNotNull();
+        assertThat(trueSnapshotSizeViaManagerWithIndex).isGreaterThan(trueSnapshotSizeViaManagerWithoutIndex);
 
-        long withIndexSize = secondListedSnapshots.get("snapshot_with_index").computeSizeOnDiskBytes();
-        long withIndexTrueSize = secondListedSnapshots.get("snapshot_with_index").computeTrueSizeBytes();
+        assertThat(sizeOfTwoSnapshots).isPositive();
+        assertThat(sizeOfTwoSnapshots).isGreaterThan(sizeOfOneSnapshot);
+
+        listedSnapshots = Util.listSnapshots(cfs);
+        assertThat(listedSnapshots.size()).isEqualTo(2);
+        assertThat(listedSnapshots.get("snapshot1_with_index")).isNotNull();
+        assertThat(listedSnapshots.get("snapshot0_without_index")).isNotNull();
+
+        long withIndexSize = listedSnapshots.get("snapshot1_with_index").computeSizeOnDiskBytes();
+        long withIndexTrueSize = listedSnapshots.get("snapshot1_with_index").computeTrueSizeBytes();
 
         assertThat(withIndexSize).isGreaterThan(withIndexTrueSize);
-        assertEquals(secondSnapshotSize, withIndexTrueSize + withoutIndexTrueSize);
+        assertEquals(sizeOfTwoSnapshots, withIndexTrueSize + withoutIndexTrueSize);
 
         // taking another one is basically a copy of the previous
-        SnapshotManager.instance.takeSnapshot("another_snapshot_with_index", new HashMap<>(), cfs.getKeyspaceTableName());
+        SnapshotManager.instance.takeSnapshot("snapshot2_with_index", new HashMap<>(), cfs.getKeyspaceTableName());
 
-        long thirdSnapshotSize = cfs.trueSnapshotsSize();
-        Map<String, TableSnapshot> thirdListedSnapshots = Util.listSnapshots(cfs);
-        assertThat(thirdSnapshotSize).isPositive();
-        assertThat(thirdSnapshotSize).isGreaterThan(secondSnapshotSize);
+        long sizeOfThreeSnapshots = cfs.trueSnapshotsSize();
+        assertThat(sizeOfThreeSnapshots).isPositive();
+        assertThat(sizeOfThreeSnapshots).isGreaterThan(sizeOfTwoSnapshots);
 
-        long anotherWithIndexSize = thirdListedSnapshots.get("another_snapshot_with_index").computeSizeOnDiskBytes();
-        long anotherWithIndexTrueSize = thirdListedSnapshots.get("another_snapshot_with_index").computeTrueSizeBytes();
+        listedSnapshots = Util.listSnapshots(cfs);
+        long anotherWithIndexSize = listedSnapshots.get("snapshot2_with_index").computeSizeOnDiskBytes();
+        long anotherWithIndexTrueSize = listedSnapshots.get("snapshot2_with_index").computeTrueSizeBytes();
 
-        assertEquals(withIndexSize, anotherWithIndexSize);
-        assertEquals(withIndexTrueSize, anotherWithIndexTrueSize);
+        // TODO CASSANDRA-20209
+        assertTrue(withIndexSize == anotherWithIndexSize || (withIndexSize + 4 == anotherWithIndexSize) || (withIndexSize - 4 == anotherWithIndexSize));
+        assertTrue(withIndexTrueSize == anotherWithIndexTrueSize || (withIndexTrueSize + 4 == anotherWithIndexTrueSize) || (withIndexTrueSize - 4 == anotherWithIndexTrueSize));
     }
 
     private void rebuildIndices(ColumnFamilyStore cfs)


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org


(cassandra) 03/03: Enable filtering of snapshots on keyspace, table and snapshot name in nodetool listsnapshots

Posted by sm...@apache.org.
This is an automated email from the ASF dual-hosted git repository.

smiklosovic pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/cassandra.git

commit 407dbacb0a0fcd5eca28abe781be635476325a65
Author: Stefan Miklosovic <sm...@apache.org>
AuthorDate: Wed Dec 18 21:04:44 2024 +0100

    Enable filtering of snapshots on keyspace, table and snapshot name in nodetool listsnapshots
    
    patch by Stefan Miklosovic; reviewed by Jordan West, Bernardo Botella, Cheng Wang, Maxim Muzafarov for CASSANDRA-20151
---
 CHANGES.txt                                        |  3 +-
 .../service/snapshot/SnapshotManagerMBean.java     | 14 +++-
 .../cassandra/tools/nodetool/ListSnapshots.java    | 21 ++++++
 .../cassandra/distributed/test/SnapshotsTest.java  | 78 ++++++++++++++++++++--
 4 files changed, 109 insertions(+), 7 deletions(-)

diff --git a/CHANGES.txt b/CHANGES.txt
index ee4e28789e..97c6386821 100644
--- a/CHANGES.txt
+++ b/CHANGES.txt
@@ -1,7 +1,8 @@
 5.1
+ * Enable filtering of snapshots on keyspace, table and snapshot name in nodetool listsnapshots (CASSANDRA-20151)
  * Create manifest upon loading where it does not exist or enrich it (CASSANDRA-20150)
  * Propagate true size of snapshot in SnapshotDetailsTabularData to not call JMX twice in nodetool listsnapshots (CASSANDRA-20149)
- * Implementation of CEP-43 (CASSANDRA-19964)
+ * Implementation of CEP-43 - copying a table via CQL by CREATE TABLE LIKE (CASSANDRA-19964)
  * Periodically disconnect roles that are revoked or have LOGIN=FALSE set (CASSANDRA-19385)
  * AST library for CQL-based fuzz tests (CASSANDRA-20198)
  * Support audit logging for JMX operations (CASSANDRA-20128)
diff --git a/src/java/org/apache/cassandra/service/snapshot/SnapshotManagerMBean.java b/src/java/org/apache/cassandra/service/snapshot/SnapshotManagerMBean.java
index 214adcc445..332ebb4b54 100644
--- a/src/java/org/apache/cassandra/service/snapshot/SnapshotManagerMBean.java
+++ b/src/java/org/apache/cassandra/service/snapshot/SnapshotManagerMBean.java
@@ -55,7 +55,19 @@ public interface SnapshotManagerMBean
     void clearSnapshot(String tag, Map<String, Object> options, String... keyspaceNames) throws IOException;
 
     /**
-     * Get the details of all the snapshots
+     * Get the details of all the snapshots. Options might be:
+     *
+     * <pre>
+     * no_ttl: "true" or "false"
+     * include_ephemeral: "true" or "false"
+     * keyspace: name of keyspace to get snapshots of
+     * table: name of table to get tables of
+     * snapshot: name of snapshot to list
+     * </pre>
+     *
+     * There is no requirement as what option has to be specified.
+     * Values of 'null' for keyspace, table or snapshot do not have any effect / will
+     * not be part of the filtering.
      *
      * @param options map of options used for filtering of snapshots
      * @return A map of snapshotName to all its details in Tabular form.
diff --git a/src/java/org/apache/cassandra/tools/nodetool/ListSnapshots.java b/src/java/org/apache/cassandra/tools/nodetool/ListSnapshots.java
index e1b61d4b51..97ea809fdd 100644
--- a/src/java/org/apache/cassandra/tools/nodetool/ListSnapshots.java
+++ b/src/java/org/apache/cassandra/tools/nodetool/ListSnapshots.java
@@ -44,6 +44,21 @@ public class ListSnapshots extends NodeToolCmd
     description = "Include ephememeral snapshots")
     private boolean includeEphemeral = false;
 
+    @Option(title = "keyspace",
+    name = { "-k", "--keyspace" },
+    description = "Include snapshots of specified keyspace name")
+    private String keyspace = null;
+
+    @Option(title = "table",
+    name = { "-t", "--table" },
+    description = "Include snapshots of specified table name")
+    private String table = null;
+
+    @Option(title = "snapshot",
+    name = { "-n", "--snapshot"},
+    description = "Include snapshots of specified name")
+    private String snapshotName = null;
+
     @Override
     public void execute(NodeProbe probe)
     {
@@ -55,6 +70,12 @@ public class ListSnapshots extends NodeToolCmd
             Map<String, String> options = new HashMap<>();
             options.put("no_ttl", Boolean.toString(noTTL));
             options.put("include_ephemeral", Boolean.toString(includeEphemeral));
+            if (keyspace != null)
+                options.put("keyspace", keyspace);
+            if (table != null)
+                options.put("table", table);
+            if (snapshotName != null)
+                options.put("snapshot", snapshotName);
 
             final Map<String, TabularData> snapshotDetails = probe.getSnapshotDetails(options);
             if (snapshotDetails.isEmpty())
diff --git a/test/distributed/org/apache/cassandra/distributed/test/SnapshotsTest.java b/test/distributed/org/apache/cassandra/distributed/test/SnapshotsTest.java
index f04a3403ca..d0b78112a8 100644
--- a/test/distributed/org/apache/cassandra/distributed/test/SnapshotsTest.java
+++ b/test/distributed/org/apache/cassandra/distributed/test/SnapshotsTest.java
@@ -512,6 +512,49 @@ public class SnapshotsTest extends TestBaseImpl
         cluster.get(1).nodetoolResult("snapshot", "-t", "somename", "-kt", String.format("%s.tbl2", KEYSPACE)).asserts().success();
     }
 
+    @Test
+    public void testListingOfSnapshotsByKeyspaceAndTable()
+    {
+        IInvokableInstance instance = cluster.get(1);
+        cluster.schemaChange("CREATE KEYSPACE IF NOT EXISTS ks1 WITH replication = {'class': 'SimpleStrategy', 'replication_factor': 1};");
+        cluster.schemaChange("CREATE KEYSPACE IF NOT EXISTS ks2 WITH replication = {'class': 'SimpleStrategy', 'replication_factor': 1};");
+        cluster.schemaChange("CREATE TABLE IF NOT EXISTS ks1.tbl (key int, value text, PRIMARY KEY (key))");
+        cluster.schemaChange("CREATE TABLE IF NOT EXISTS ks1.tbl2 (key int, value text, PRIMARY KEY (key))");
+        cluster.schemaChange("CREATE TABLE IF NOT EXISTS ks2.tbl (key int, value text, PRIMARY KEY (key))");
+        cluster.schemaChange("CREATE TABLE IF NOT EXISTS ks2.tbl2 (key int, value text, PRIMARY KEY (key))");
+
+        populate(cluster, "ks1", "tbl");
+        populate(cluster, "ks1", "tbl2");
+        populate(cluster, "ks2", "tbl");
+        populate(cluster, "ks2", "tbl2");
+
+        instance.nodetoolResult("snapshot", "-t", "tagks1tbl", "-kt", "ks1.tbl").asserts().success();
+        instance.nodetoolResult("snapshot", "-t", "tagks1tbl2", "-kt", "ks1.tbl2").asserts().success();
+        instance.nodetoolResult("snapshot", "-t", "tagks2tbl", "-kt", "ks2.tbl").asserts().success();
+        instance.nodetoolResult("snapshot", "-t", "tagks2tbl2", "-kt", "ks2.tbl2").asserts().success();
+
+        waitForSnapshot("ks1", null, "tagks1tbl", true, false);
+        waitForSnapshot("ks1", null, "tagks1tbl2", true, false);
+        waitForSnapshot("ks1", null, "tagks2tbl", false, false);
+        waitForSnapshot("ks1", null, "tagks2tbl2", false, false);
+
+        waitForSnapshot("ks1", "tbl", "tagks1tbl", true, false);
+        waitForSnapshot("ks1", "tbl", "tagks1tbl2", false, false);
+        waitForSnapshot("ks1", "tbl", "tagks2tbl", false, false);
+        waitForSnapshot("ks1", "tbl", "tagks2tbl2", false, false);
+
+        waitForSnapshot(null, "tbl", "tagks1tbl", true, false);
+        waitForSnapshot(null, "tbl", "tagks1tbl2", false, false);
+        waitForSnapshot(null, "tbl", "tagks2tbl", true, false);
+        waitForSnapshot(null, "tbl", "tagks2tbl2", false, false);
+
+        NodeToolResult nodeToolResult = instance.nodetoolResult("listsnapshots", "-n", "tagks1tbl");
+        nodeToolResult.asserts().success();
+        List<String> snapshots = extractSnapshots(nodeToolResult.getStdout());
+        assertEquals(1, snapshots.size());
+        assertTrue(snapshots.get(0).contains("tagks1tbl"));
+    }
+
     private void populate(Cluster cluster)
     {
         for (int i = 0; i < 100; i++)
@@ -555,13 +598,27 @@ public class SnapshotsTest extends TestBaseImpl
         if (noTTL)
             args.add("-nt");
 
+        if (keyspaceName != null)
+        {
+            args.add("-k");
+            args.add(keyspaceName);
+        }
+
+        if (tableName != null)
+        {
+            args.add("-t");
+            args.add(tableName);
+        }
+
+        if (snapshotName != null)
+        {
+            args.add("-n");
+            args.add(snapshotName);
+        }
+
         listsnapshots = cluster.get(1).nodetoolResult(args.toArray(new String[0]));
 
-        List<String> lines = Arrays.stream(listsnapshots.getStdout().split("\n"))
-                                   .filter(line -> !line.isEmpty())
-                                   .filter(line -> !line.startsWith("Snapshot Details:") && !line.startsWith("There are no snapshots"))
-                                   .filter(line -> !line.startsWith("Snapshot name") && !line.startsWith("Total TrueDiskSpaceUsed"))
-                                   .collect(toList());
+        List<String> lines = extractSnapshots(listsnapshots.getStdout());
 
         return expectPresent == lines.stream().anyMatch(line -> line.startsWith(snapshotName));
     }
@@ -632,4 +689,15 @@ public class SnapshotsTest extends TestBaseImpl
             return result.toArray(new String[0]);
         }, forSystemKeyspaces);
     }
+
+    private List<String> extractSnapshots(String listSnapshotsStdOut)
+    {
+        return Arrays.stream(listSnapshotsStdOut.split("\n"))
+                     .filter(line -> !line.isEmpty())
+                     .filter(line -> !line.startsWith("Snapshot Details:"))
+                     .filter(line -> !line.startsWith("There are no snapshots"))
+                     .filter(line -> !line.startsWith("Snapshot name"))
+                     .filter(line -> !line.startsWith("Total TrueDiskSpaceUsed"))
+                     .collect(toList());
+    }
 }


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org


(cassandra) 02/03: Create manifest upon loading where it does not exist or enrich it

Posted by sm...@apache.org.
This is an automated email from the ASF dual-hosted git repository.

smiklosovic pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/cassandra.git

commit b4e9399865eb51077f51d34c4c383b8418a9e6ed
Author: Stefan Miklosovic <sm...@apache.org>
AuthorDate: Wed Dec 18 14:25:29 2024 +0100

    Create manifest upon loading where it does not exist or enrich it
    
    When snapshot manifest does not exist, no snapshots would be found, because even
    they would be loaded, since CASSANDRA-18111, snapshot's presence is logically
    determined by the existence of at least one manifest.json file in any data dir. That
    would result in snapshots not being shown e.g. in the output of
    nodetool listsnapshots.
    
    The fix consists of creating a manifest in each snapshot directory when not present.
    
    When a manifest.json exists, its older format has not contained created_at field (pre-16789)
    When this is detected, we will proceed to enrich such manifest by overwriting it
    with a manifest of new format.
    
    patch by Stefan Miklosovic; reviewed by Jordan West, Cheng Wang, Bernardo Botella for CASSANDRA-20150
---
 CHANGES.txt                                        |   1 +
 .../config/CassandraRelevantProperties.java        |   6 +
 .../cassandra/service/snapshot/TableSnapshot.java  | 146 +++++++++++++++++-
 .../cassandra/distributed/test/SnapshotsTest.java  | 167 ++++++++++++++++++++-
 .../org/apache/cassandra/db/DirectoriesTest.java   |  10 +-
 .../service/snapshot/SnapshotLoaderTest.java       |  57 +++++--
 6 files changed, 364 insertions(+), 23 deletions(-)

diff --git a/CHANGES.txt b/CHANGES.txt
index b20e0f8b1f..ee4e28789e 100644
--- a/CHANGES.txt
+++ b/CHANGES.txt
@@ -1,4 +1,5 @@
 5.1
+ * Create manifest upon loading where it does not exist or enrich it (CASSANDRA-20150)
  * Propagate true size of snapshot in SnapshotDetailsTabularData to not call JMX twice in nodetool listsnapshots (CASSANDRA-20149)
  * Implementation of CEP-43 (CASSANDRA-19964)
  * Periodically disconnect roles that are revoked or have LOGIN=FALSE set (CASSANDRA-19385)
diff --git a/src/java/org/apache/cassandra/config/CassandraRelevantProperties.java b/src/java/org/apache/cassandra/config/CassandraRelevantProperties.java
index 8e5b0c6dd9..70a6ea702e 100644
--- a/src/java/org/apache/cassandra/config/CassandraRelevantProperties.java
+++ b/src/java/org/apache/cassandra/config/CassandraRelevantProperties.java
@@ -502,6 +502,12 @@ public enum CassandraRelevantProperties
     SNAPSHOT_CLEANUP_INITIAL_DELAY_SECONDS("cassandra.snapshot.ttl_cleanup_initial_delay_seconds", "5"),
     /** snapshots ttl cleanup period in seconds */
     SNAPSHOT_CLEANUP_PERIOD_SECONDS("cassandra.snapshot.ttl_cleanup_period_seconds", "60"),
+    /**
+     * When there is a snapshot with old / basic format (basically pre-CASSANDRA-16789),
+     * it will enrich it with more metadata upon snapshot's loading at startup.
+     * Defaults to true, when set to false, no enriching will be done.
+     * */
+    SNAPSHOT_MANIFEST_ENRICH_OR_CREATE_ENABLED("cassandra.snapshot.enrich.or.create.enabled", "true"),
     /** minimum allowed TTL for snapshots */
     SNAPSHOT_MIN_ALLOWED_TTL_SECONDS("cassandra.snapshot.min_allowed_ttl_seconds", "60"),
     SSL_ENABLE("ssl.enable"),
diff --git a/src/java/org/apache/cassandra/service/snapshot/TableSnapshot.java b/src/java/org/apache/cassandra/service/snapshot/TableSnapshot.java
index 549dbb6db6..7698ea1e3d 100644
--- a/src/java/org/apache/cassandra/service/snapshot/TableSnapshot.java
+++ b/src/java/org/apache/cassandra/service/snapshot/TableSnapshot.java
@@ -19,10 +19,13 @@ package org.apache.cassandra.service.snapshot;
 
 import java.io.IOException;
 import java.nio.file.Files;
+import java.nio.file.NoSuchFileException;
 import java.nio.file.Path;
 import java.time.Instant;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
@@ -35,6 +38,7 @@ import java.util.stream.Stream;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import org.apache.cassandra.config.CassandraRelevantProperties;
 import org.apache.cassandra.db.ColumnFamilyStore;
 import org.apache.cassandra.db.Directories;
 import org.apache.cassandra.db.Keyspace;
@@ -42,10 +46,13 @@ import org.apache.cassandra.db.lifecycle.SSTableSet;
 import org.apache.cassandra.io.FSReadError;
 import org.apache.cassandra.io.sstable.Component;
 import org.apache.cassandra.io.sstable.Descriptor;
+import org.apache.cassandra.io.sstable.SSTable;
 import org.apache.cassandra.io.sstable.SSTableId;
+import org.apache.cassandra.io.sstable.format.SSTableFormat;
 import org.apache.cassandra.io.sstable.format.SSTableReader;
 import org.apache.cassandra.io.util.File;
 import org.apache.cassandra.io.util.FileUtils;
+import org.apache.cassandra.utils.Clock;
 import org.apache.cassandra.utils.FBUtilities;
 import org.apache.cassandra.utils.LocalizeString;
 import org.apache.cassandra.utils.Throwables;
@@ -262,7 +269,8 @@ public class TableSnapshot
         }
         catch (Throwable t)
         {
-            logger.error("Could not list directory content {}", dir, t);
+            if (!(t instanceof NoSuchFileException))
+                logger.error("Could not list directory content {}", dir, t);
         }
 
         return paths;
@@ -490,8 +498,144 @@ public class TableSnapshot
 
         TableSnapshot build()
         {
+            maybeCreateOrEnrichManifest();
             return new TableSnapshot(keyspaceName, tableName, tableId, tag, createdAt, expiresAt, snapshotDirs, ephemeral);
         }
+
+        private List<File> getSnapshotDirsWithoutManifest()
+        {
+            List<File> snapshotDirNotHavingManifests = new ArrayList<>();
+            for (File snapshotDir : snapshotDirs)
+            {
+                if (!new File(snapshotDir.toPath().resolve("manifest.json")).exists())
+                    snapshotDirNotHavingManifests.add(snapshotDir);
+            }
+
+            return snapshotDirNotHavingManifests;
+        }
+
+        private void maybeCreateOrEnrichManifest()
+        {
+            if (!CassandraRelevantProperties.SNAPSHOT_MANIFEST_ENRICH_OR_CREATE_ENABLED.getBoolean())
+                return;
+
+            // this is caused by not reading any manifest or that snapshot had a basic manifest just with sstables
+            // enumerated (pre CASSANDRA-16789), so we just go ahead and enrich it in each snapshot dir
+
+            List<File> snapshotDirsWithoutManifests = getSnapshotDirsWithoutManifest();
+            if (createdAt != null && snapshotDirsWithoutManifests.isEmpty())
+                return;
+
+            if (createdAt == null && snapshotDirsWithoutManifests.isEmpty())
+                logger.info("Manifest in the old format for snapshot {} for {}.{} was detected, going to enrich it.",
+                            tag, keyspaceName, tableName);
+
+            if (!snapshotDirsWithoutManifests.isEmpty())
+                logger.info("There is no manifest for snapshot {} for {}.{} at paths {}, going to create it.",
+                            tag, keyspaceName, tableName, snapshotDirsWithoutManifests);
+
+            long lastModified = createdAt == null ? -1 : createdAt.toEpochMilli();
+
+            if (lastModified == -1)
+            {
+                for (File snapshotDir : snapshotDirs)
+                {
+                    // we will consider time of the creation the oldest last modified
+                    // timestamp on any snapshot directory
+                    long currentLastModified = snapshotDir.lastModified();
+                    if ((currentLastModified < lastModified || lastModified == -1) && currentLastModified > 0)
+                        lastModified = currentLastModified;
+                }
+            }
+
+            List<String> allDataFiles = new ArrayList<>();
+            for (File snapshotDir : snapshotDirs)
+            {
+                List<File> dataFiles = new ArrayList<>();
+                try
+                {
+                    List<File> indicesDirs = new ArrayList<>();
+                    File[] snapshotFiles = snapshotDir.list(file -> {
+                        if (file.isDirectory() && file.name().startsWith("."))
+                        {
+                            indicesDirs.add(file);
+                            return false;
+                        }
+                        else
+                        {
+                            return file.name().endsWith('-' + SSTableFormat.Components.DATA.type.repr);
+                        }
+                    });
+
+                    Collections.addAll(dataFiles, snapshotFiles);
+
+                    for (File indexDir : indicesDirs)
+                        dataFiles.addAll(Arrays.asList(indexDir.list(file -> file.name().endsWith('-' + SSTableFormat.Components.DATA.type.repr))));
+                }
+                catch (IOException ex)
+                {
+                    logger.error("Unable to list a directory for data components: {}", snapshotDir);
+                }
+
+                for (File dataFile : dataFiles)
+                {
+                    Descriptor descriptor = SSTable.tryDescriptorFromFile(dataFile);
+                    if (descriptor != null)
+                    {
+                        String relativeDataFileName = descriptor.relativeFilenameFor(SSTableFormat.Components.DATA);
+                        allDataFiles.add(relativeDataFileName);
+                    }
+                }
+            }
+
+            // in any case of not being able to resolve it, set it to current time
+            if (lastModified < 0)
+                createdAt = Instant.ofEpochMilli(Clock.Global.currentTimeMillis());
+            else
+                createdAt = Instant.ofEpochMilli(lastModified);
+
+            for (File snapshotDir : snapshotDirs)
+            {
+                SnapshotManifest snapshotManifest = new SnapshotManifest(allDataFiles, null, createdAt, ephemeral);
+                File manifestFile = new File(snapshotDir, "manifest.json");
+                if (snapshotDirsWithoutManifests.contains(snapshotDir))
+                {
+                    writeManifest(snapshotManifest, manifestFile);
+                }
+                else
+                {
+                    SnapshotManifest existingManifest = readManifest(manifestFile);
+                    if (existingManifest != null && existingManifest.createdAt == null)
+                        writeManifest(snapshotManifest, manifestFile);
+                }
+            }
+        }
+
+        private SnapshotManifest readManifest(File manifestFile)
+        {
+            try
+            {
+                return SnapshotManifest.deserializeFromJsonFile(manifestFile);
+            }
+            catch (IOException ex)
+            {
+                logger.error("Unable to read a manifest.json file in {}", manifestFile.absolutePath());
+            }
+
+            return null;
+        }
+
+        private void writeManifest(SnapshotManifest snapshotManifest, File manifestFile)
+        {
+            try
+            {
+                snapshotManifest.serializeToJsonFile(manifestFile);
+            }
+            catch (IOException ex)
+            {
+                logger.error("Unable to create a manifest.json file in {}", manifestFile.absolutePath());
+            }
+        }
     }
 
     protected static String buildSnapshotId(String keyspaceName, String tableName, UUID tableId, String tag)
diff --git a/test/distributed/org/apache/cassandra/distributed/test/SnapshotsTest.java b/test/distributed/org/apache/cassandra/distributed/test/SnapshotsTest.java
index 2c57654695..f04a3403ca 100644
--- a/test/distributed/org/apache/cassandra/distributed/test/SnapshotsTest.java
+++ b/test/distributed/org/apache/cassandra/distributed/test/SnapshotsTest.java
@@ -33,32 +33,40 @@ import org.junit.Test;
 
 import org.apache.cassandra.config.CassandraRelevantProperties;
 import org.apache.cassandra.db.ColumnFamilyStore;
+import org.apache.cassandra.db.Keyspace;
 import org.apache.cassandra.distributed.Cluster;
 import org.apache.cassandra.distributed.api.ConsistencyLevel;
 import org.apache.cassandra.distributed.api.IInvokableInstance;
+import org.apache.cassandra.distributed.api.IIsolatedExecutor;
 import org.apache.cassandra.distributed.api.IIsolatedExecutor.SerializableCallable;
 import org.apache.cassandra.distributed.api.NodeToolResult;
 import org.apache.cassandra.distributed.shared.WithProperties;
+import org.apache.cassandra.io.util.File;
+import org.apache.cassandra.service.snapshot.SnapshotManager;
 import org.apache.cassandra.utils.Clock;
 import org.apache.cassandra.utils.FBUtilities;
 
 import static java.lang.String.format;
 import static java.util.concurrent.TimeUnit.SECONDS;
 import static java.util.stream.Collectors.toList;
+import static org.apache.cassandra.config.CassandraRelevantProperties.SNAPSHOT_MANIFEST_ENRICH_OR_CREATE_ENABLED;
 import static org.apache.cassandra.distributed.shared.ClusterUtils.stopUnchecked;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.apache.cassandra.schema.SchemaConstants.LOCAL_SYSTEM_KEYSPACE_NAMES;
+import static org.apache.cassandra.schema.SchemaConstants.REPLICATED_SYSTEM_KEYSPACE_NAMES;
 import static org.awaitility.Awaitility.await;
 import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.CoreMatchers.not;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assume.assumeThat;
 import static oshi.PlatformEnum.MACOS;
 
 public class SnapshotsTest extends TestBaseImpl
 {
-    public static final Integer SNAPSHOT_CLEANUP_PERIOD_SECONDS = 1;
+    public static final Integer SNAPSHOT_CLEANUP_PERIOD_SECONDS = 2;
     public static final Integer FIVE_SECONDS = 5;
     public static final Integer TEN_SECONDS = 10;
     private static final WithProperties properties = new WithProperties();
@@ -130,10 +138,117 @@ public class SnapshotsTest extends TestBaseImpl
         }
     }
 
+    @Test
+    public void testLocalOrReplicatedSystemTablesSnapshotsDoNotHaveSchema()
+    {
+        cluster.get(1)
+               .nodetoolResult("snapshot", "-t", "snapshot_with_local_or_replicated")
+               .asserts()
+               .success();
+
+        String[] dataDirs = (String[]) cluster.get(1).config().get("data_file_directories");
+        String[] paths = getSnapshotPaths(true);
+
+        for (String dataDir : dataDirs)
+        {
+            for (String path : paths)
+            {
+                Path snapshotDir = Paths.get(dataDir)
+                                        .resolve(path)
+                                        .resolve("snapshots")
+                                        .resolve("snapshot_with_local_or_replicated");
+
+                if (snapshotDir.toFile().exists())
+                    assertFalse(new File(snapshotDir, "schema.cql").exists());
+            }
+        }
+    }
+
+    @Test
+    public void testMissingManifestIsCreatedOnStartupWithEnrichmentEnabled()
+    {
+        cluster.schemaChange(withKeyspace("CREATE TABLE %s.tbl (key int, value text, PRIMARY KEY (key))"));
+        populate(cluster);
+
+        cluster.get(1)
+               .nodetoolResult("snapshot", "-t", "snapshot_with_local_or_replicated")
+               .asserts()
+               .success();
+
+        String[] dataDirs = (String[]) cluster.get(1).config().get("data_file_directories");
+        String[] paths = getSnapshotPaths(false);
+
+        assertManifestsPresence(dataDirs, paths, true);
+
+        // remove all manifest files
+        removeAllManifests(dataDirs, paths);
+        assertManifestsPresence(dataDirs, paths, false);
+
+        // restart which should add them back
+        cluster.get(1).shutdown(true);
+        cluster.get(1).startup();
+
+        assertManifestsPresence(dataDirs, paths, true);
+
+        // remove them again and reload by mbean, that should create them too
+        removeAllManifests(dataDirs, paths);
+        assertManifestsPresence(dataDirs, paths, false);
+        cluster.get(1).runOnInstance((IIsolatedExecutor.SerializableRunnable) () -> SnapshotManager.instance.restart(true));
+        assertManifestsPresence(dataDirs, paths, true);
+
+        cluster.get(1).shutdown(true);
+
+        // remove manifest only in the first data dir
+        removeAllManifests(new String[] {dataDirs[0]}, paths);
+
+        // they will be still created for that first dir
+        cluster.get(1).startup();
+        assertManifestsPresence(dataDirs, paths, true);
+    }
+
+    @Test
+    public void testMissingManifestIsNotCreatedOnStartupWithEnrichmentDisabled()
+    {
+        cluster.schemaChange(withKeyspace("CREATE TABLE %s.tbl (key int, value text, PRIMARY KEY (key))"));
+        populate(cluster);
+
+        cluster.get(1)
+               .nodetoolResult("snapshot", "-t", "snapshot_with_local_or_replicated")
+               .asserts()
+               .success();
+
+        String[] dataDirs = (String[]) cluster.get(1).config().get("data_file_directories");
+        String[] paths = getSnapshotPaths(false);
+
+        assertManifestsPresence(dataDirs, paths, true);
+
+        cluster.get(1).shutdown(true);
+
+        // remove all manifest files
+        removeAllManifests(dataDirs, paths);
+        assertManifestsPresence(dataDirs, paths, false);
+
+        try (WithProperties ignored = new WithProperties().set(SNAPSHOT_MANIFEST_ENRICH_OR_CREATE_ENABLED, false))
+        {
+            // restart which should NOT add them back because we disabled it by system property
+            cluster.get(1).startup();
+
+            // no manifests created
+            assertManifestsPresence(dataDirs, paths, false);
+
+            cluster.get(1).runOnInstance((IIsolatedExecutor.SerializableRunnable) () -> SnapshotManager.instance.restart(true));
+
+            // no manifests created, even after restart of SnapshotManager
+            assertManifestsPresence(dataDirs, paths, false);
+        }
+    }
+
     @Test
     public void testSnapshotsCleanupByTTL()
     {
-        cluster.get(1).nodetoolResult("snapshot", "--ttl", format("%ds", FIVE_SECONDS),
+        cluster.schemaChange(withKeyspace("CREATE TABLE %s.tbl (key int, value text, PRIMARY KEY (key))"));
+        populate(cluster, withKeyspace("%s"), "tbl");
+        cluster.get(1).nodetoolResult("snapshot", "-kt", withKeyspace("%s.tbl"), "--ttl", format("%ds", FIVE_SECONDS),
                                       "-t", "basic").asserts().success();
         waitForSnapshotPresent("basic");
         waitForSnapshotCleared("basic");
@@ -469,4 +584,52 @@ public class SnapshotsTest extends TestBaseImpl
             waitForSnapshot(tag, true, true);
         }
     }
+
+    private void assertManifestsPresence(String[] dataDirs, String[] paths, boolean shouldExist)
+    {
+        for (String dataDir : dataDirs)
+        {
+            for (String path : paths)
+            {
+                Path snapshotDir = Paths.get(dataDir).resolve(path).resolve("snapshots").resolve("snapshot_with_local_or_replicated");
+                assertEquals(shouldExist, new File(snapshotDir, "manifest.json").exists());
+            }
+        }
+    }
+
+    private void removeAllManifests(String[] dataDirs, String[] paths)
+    {
+        for (String dataDir : dataDirs)
+        {
+            for (String path : paths)
+            {
+                Path snapshotDir = Paths.get(dataDir).resolve(path).resolve("snapshots").resolve("snapshot_with_local_or_replicated");
+
+                File manifest = new File(snapshotDir, "manifest.json");
+                assertTrue(manifest.exists());
+                manifest.delete();
+                assertFalse(manifest.exists());
+            }
+        }
+    }
+
+    private String[] getSnapshotPaths(boolean forSystemKeyspaces)
+    {
+        return cluster.get(1).applyOnInstance((IIsolatedExecutor.SerializableFunction<Boolean, String[]>) forSystem -> {
+            List<String> result = new ArrayList<>();
+
+            for (Keyspace keyspace : Keyspace.all())
+            {
+                if (forSystem && !LOCAL_SYSTEM_KEYSPACE_NAMES.contains(keyspace.getName()) && !REPLICATED_SYSTEM_KEYSPACE_NAMES.contains(keyspace.getName()))
+                    continue;
+                else if (LOCAL_SYSTEM_KEYSPACE_NAMES.contains(keyspace.getName()) || REPLICATED_SYSTEM_KEYSPACE_NAMES.contains(keyspace.getName()))
+                    continue;
+
+                for (ColumnFamilyStore cfs : keyspace.getColumnFamilyStores())
+                    result.add(format("%s/%s-%s", keyspace.getName(), cfs.name, cfs.metadata().id.toHexString()));
+            }
+
+            return result.toArray(new String[0]);
+        }, forSystemKeyspaces);
+    }
 }
diff --git a/test/unit/org/apache/cassandra/db/DirectoriesTest.java b/test/unit/org/apache/cassandra/db/DirectoriesTest.java
index b6d4126fc9..06db7e4a02 100644
--- a/test/unit/org/apache/cassandra/db/DirectoriesTest.java
+++ b/test/unit/org/apache/cassandra/db/DirectoriesTest.java
@@ -351,11 +351,11 @@ public class DirectoriesTest
 
         // Create snapshot with and without manifest
         FakeSnapshot snapshot1 = createFakeSnapshot(fakeTable, SNAPSHOT1, true, false);
-        FakeSnapshot snapshot2 = createFakeSnapshot(fakeTable, SNAPSHOT2, false, false);
-        // ephemeral without manifst
-        FakeSnapshot snapshot3 = createFakeSnapshot(fakeTable, SNAPSHOT3, false, true);
+        FakeSnapshot snapshot2 = createFakeSnapshot(fakeTable, SNAPSHOT2, true, false);
+        // ephemeral without manifest
+        FakeSnapshot snapshot3 = createFakeSnapshot(fakeTable, SNAPSHOT3, true, true);
 
-        // Both snapshots should be present
+        // All snapshots should be present
         Map<String, TableSnapshot> snapshots = listSnapshots(directories);
         assertThat(snapshots.keySet()).isEqualTo(Sets.newHashSet(SNAPSHOT1, SNAPSHOT2, SNAPSHOT3));
         assertThat(snapshots.get(SNAPSHOT1)).isEqualTo(snapshot1.asTableSnapshot());
@@ -459,7 +459,7 @@ public class DirectoriesTest
             }
         });
         // CASSANDRA-17357: include indexes when computing true size of parent table
-        assertEquals(70L, parentSnapshotDetail.get("test").computeTrueSizeBytes(files));
+        assertEquals(70L, parentSnapshotDetail.get("test").computeTrueSizeBytes(files) - parentSnapshotDetail.get("test").getManifestsSize());
 
         // check backup directory
         File parentBackupDirectory = Directories.getBackupsDirectory(parentDesc);
diff --git a/test/unit/org/apache/cassandra/service/snapshot/SnapshotLoaderTest.java b/test/unit/org/apache/cassandra/service/snapshot/SnapshotLoaderTest.java
index 54ecf19d98..ee76cedd63 100644
--- a/test/unit/org/apache/cassandra/service/snapshot/SnapshotLoaderTest.java
+++ b/test/unit/org/apache/cassandra/service/snapshot/SnapshotLoaderTest.java
@@ -23,6 +23,7 @@ import java.nio.file.Paths;
 import java.time.Instant;
 import java.util.Arrays;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Set;
 import java.util.UUID;
 import java.util.concurrent.ThreadLocalRandom;
@@ -37,6 +38,7 @@ import org.apache.cassandra.config.DatabaseDescriptor;
 import org.apache.cassandra.config.DurationSpec;
 import org.apache.cassandra.db.Directories;
 import org.apache.cassandra.io.util.File;
+import org.apache.cassandra.utils.Clock;
 import org.assertj.core.util.Lists;
 
 import static org.apache.cassandra.service.snapshot.SnapshotLoader.SNAPSHOT_DIR_PATTERN;
@@ -110,9 +112,9 @@ public class SnapshotLoaderTest
     @Test
     public void testSnapshotsWithoutManifests() throws IOException
     {
-        Set<File> tag1Files = new HashSet<>();
-        Set<File> tag2Files = new HashSet<>();
-        Set<File> tag3Files = new HashSet<>();
+        Set<File> firstSnapshotDirs = new HashSet<>();
+        Set<File> secondSnapshotDirs = new HashSet<>();
+        Set<File> thirdSnapshotDirs = new HashSet<>();
 
         // Create one snapshot per table - without manifests:
         // - ks1.t1 : tag1
@@ -121,20 +123,26 @@ public class SnapshotLoaderTest
         File baseDir  = new File(tmpDir.newFolder());
         for (String dataDir : DATA_DIRS)
         {
-            tag1Files.add(createDir(baseDir, dataDir, KEYSPACE_1, tableDirName(TABLE1_NAME, TABLE1_ID), Directories.SNAPSHOT_SUBDIR, TAG1));
-            tag2Files.add(createDir(baseDir, dataDir, KEYSPACE_1, tableDirName(TABLE2_NAME, TABLE2_ID), Directories.SNAPSHOT_SUBDIR, TAG2));
-            tag3Files.add(createDir(baseDir, dataDir, KEYSPACE_2, tableDirName(TABLE3_NAME, TABLE3_ID), Directories.SNAPSHOT_SUBDIR, TAG3));
+            firstSnapshotDirs.add(createDir(baseDir, dataDir, KEYSPACE_1, tableDirName(TABLE1_NAME, TABLE1_ID), Directories.SNAPSHOT_SUBDIR, TAG1));
+            secondSnapshotDirs.add(createDir(baseDir, dataDir, KEYSPACE_1, tableDirName(TABLE2_NAME, TABLE2_ID), Directories.SNAPSHOT_SUBDIR, TAG2));
+            thirdSnapshotDirs.add(createDir(baseDir, dataDir, KEYSPACE_2, tableDirName(TABLE3_NAME, TABLE3_ID), Directories.SNAPSHOT_SUBDIR, TAG3));
         }
 
+        Instant createdAt = Instant.ofEpochMilli(Clock.Global.currentTimeMillis());
+
+        createManifests(firstSnapshotDirs, createdAt);
+        createManifests(secondSnapshotDirs, createdAt);
+        createManifests(thirdSnapshotDirs, createdAt);
+
         // Verify all 3 snapshots are found correctly from data directories
         SnapshotLoader loader = new SnapshotLoader(Arrays.asList(Paths.get(baseDir.toString(), DATA_DIR_1),
                                                                  Paths.get(baseDir.toString(), DATA_DIR_2),
                                                                  Paths.get(baseDir.toString(), DATA_DIR_3)));
         Set<TableSnapshot> snapshots = loader.loadSnapshots();
         assertThat(snapshots).hasSize(3);
-        assertThat(snapshots).contains(new TableSnapshot(KEYSPACE_1, TABLE1_NAME, TABLE1_ID, TAG1, null, null, tag1Files, false));
-        assertThat(snapshots).contains(new TableSnapshot(KEYSPACE_1, TABLE2_NAME, TABLE2_ID,  TAG2, null, null, tag2Files, false));
-        assertThat(snapshots).contains(new TableSnapshot(KEYSPACE_2, TABLE3_NAME, TABLE3_ID,  TAG3, null, null, tag3Files, false));
+        assertThat(snapshots).contains(new TableSnapshot(KEYSPACE_1, TABLE1_NAME, TABLE1_ID, TAG1, createdAt, null, firstSnapshotDirs, false));
+        assertThat(snapshots).contains(new TableSnapshot(KEYSPACE_1, TABLE2_NAME, TABLE2_ID, TAG2, createdAt, null, secondSnapshotDirs, false));
+        assertThat(snapshots).contains(new TableSnapshot(KEYSPACE_2, TABLE3_NAME, TABLE3_ID, TAG3, createdAt, null, thirdSnapshotDirs, false));
 
         // Verify snapshot loading for a specific keyspace
         loader = new SnapshotLoader(Arrays.asList(Paths.get(baseDir.toString(), DATA_DIR_1),
@@ -143,21 +151,37 @@ public class SnapshotLoaderTest
 
         snapshots = loader.loadSnapshots(KEYSPACE_1);
         assertThat(snapshots).hasSize(2);
-        assertThat(snapshots).contains(new TableSnapshot(KEYSPACE_1, TABLE1_NAME, TABLE1_ID, TAG1, null, null, tag1Files, false));
-        assertThat(snapshots).contains(new TableSnapshot(KEYSPACE_1, TABLE2_NAME, TABLE2_ID,  TAG2, null, null, tag2Files, false));
+        assertThat(snapshots).contains(new TableSnapshot(KEYSPACE_1, TABLE1_NAME, TABLE1_ID, TAG1, createdAt, null, firstSnapshotDirs, false));
+        assertThat(snapshots).contains(new TableSnapshot(KEYSPACE_1, TABLE2_NAME, TABLE2_ID,  TAG2, createdAt, null, secondSnapshotDirs, false));
 
         loader = new SnapshotLoader(Arrays.asList(Paths.get(baseDir.toString(), DATA_DIR_1),
                                                   Paths.get(baseDir.toString(), DATA_DIR_2),
                                                   Paths.get(baseDir.toString(), DATA_DIR_3)));
         snapshots = loader.loadSnapshots(KEYSPACE_2);
         assertThat(snapshots).hasSize(1);
-        assertThat(snapshots).contains(new TableSnapshot(KEYSPACE_2, TABLE3_NAME, TABLE3_ID,  TAG3, null, null, tag3Files, false));
+        assertThat(snapshots).contains(new TableSnapshot(KEYSPACE_2, TABLE3_NAME, TABLE3_ID,  TAG3, createdAt, null, thirdSnapshotDirs, false));
+    }
+
+    private void createManifests(Set<File> snapshotDirs, Instant createdAt)
+    {
+        SnapshotManifest snapshotManifest = new SnapshotManifest(List.of(), null, createdAt, false);
+        for (File snapshotDir : snapshotDirs)
+        {
+            try
+            {
+                snapshotManifest.serializeToJsonFile(new File(snapshotDir, "manifest.json"));
+            }
+            catch (Throwable t)
+            {
+                throw new RuntimeException("Unable to write manifest file", t);
+            }
+        }
     }
 
     @Test
     public void testEphemeralSnapshotWithoutManifest() throws IOException
     {
-        Set<File> tag1Files = new HashSet<>();
+        Set<File> snapshotDirs = new HashSet<>();
 
         // Create one snapshot per table - without manifests:
         // - ks1.t1 : tag1
@@ -166,7 +190,7 @@ public class SnapshotLoaderTest
         for (String dataDir : DATA_DIRS)
         {
             File dir = createDir(baseDir, dataDir, KEYSPACE_1, tableDirName(TABLE1_NAME, TABLE1_ID), Directories.SNAPSHOT_SUBDIR, TAG1);
-            tag1Files.add(dir);
+            snapshotDirs.add(dir);
             if (!ephemeralFileCreated)
             {
                 createEphemeralMarkerFile(dir);
@@ -179,9 +203,12 @@ public class SnapshotLoaderTest
                                                                  Paths.get(baseDir.toString(), DATA_DIR_2),
                                                                  Paths.get(baseDir.toString(), DATA_DIR_3)));
 
+        Instant createdAt = Instant.ofEpochMilli(Clock.Global.currentTimeMillis());
+        createManifests(snapshotDirs, createdAt);
+
         Set<TableSnapshot> snapshots = loader.loadSnapshots();
         assertThat(snapshots).hasSize(1);
-        assertThat(snapshots).contains(new TableSnapshot(KEYSPACE_1, TABLE1_NAME, TABLE1_ID, TAG1, null, null, tag1Files, true));
+        assertThat(snapshots).contains(new TableSnapshot(KEYSPACE_1, TABLE1_NAME, TABLE1_ID, TAG1, createdAt, null, snapshotDirs, true));
         Assert.assertTrue(snapshots.stream().findFirst().get().isEphemeral());
     }
 


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org