review feedback

apache · Dec 18, 2024 · 2438131 · 2438131
1 parent e25ce93
commit 2438131
Show file tree

Hide file tree

Showing 3 changed files with 67 additions and 4 deletions.
diff --git a/src/java/org/apache/cassandra/config/CassandraRelevantProperties.java b/src/java/org/apache/cassandra/config/CassandraRelevantProperties.java
@@ -507,7 +507,7 @@ public enum CassandraRelevantProperties
      * 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_ENABLED("cassandra.snapshot.enrich.enabled", "true"),
+    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
@@ -505,10 +505,31 @@ private void maybeCreateOrEnrichManifest()
             // this is caused by not reading any manifest of a new format or that snapshot had none upon loading,
             // that might be the case when upgrading e.g. from 4.0 where basic manifest is created when taking a snapshot,
             // so we just go ahead and enrich it in each snapshot dir
-            if (createdAt != null)
+
+            boolean oldManifestExists = false;
+
+            if (createdAt == null)
+            {
+                for (File snapshotDir : snapshotDirs)
+                {
+                    File maybeManifest = new File(snapshotDir.toPath().resolve("manifest.json"));
+                    if (maybeManifest.exists())
+                    {
+                        oldManifestExists = true;
+                        break;
+                    }
+                }
+                if (oldManifestExists)
+                    logger.debug("Manifest in the old format for snapshot {} was detected, going to enrich it.", this);
+                else
+                    logger.debug("There is no manifest for {}, going to create it.", this);
+            }
+            else
+            {
                 return;
+            }
 
-            if (!CassandraRelevantProperties.SNAPSHOT_MANIFEST_ENRICH_ENABLED.getBoolean())
+            if (!CassandraRelevantProperties.SNAPSHOT_MANIFEST_ENRICH_OR_CREATE_ENABLED.getBoolean())
                 return;
 
             long lastModified = -1;
@@ -573,6 +594,10 @@ private void maybeCreateOrEnrichManifest()
                 try
                 {
                     snapshotManifest.serializeToJsonFile(manifestFile);
+                    if (oldManifestExists)
+                        logger.debug("Manifest file {} was rewritten for snapshot {}", manifestFile.absolutePath(), this);
+                    else
+                        logger.debug("Manifest file {} was created for snapshot {}", manifestFile.absolutePath(), this);
                 }
                 catch (IOException ex)
                 {

diff --git a/test/distributed/org/apache/cassandra/distributed/test/SnapshotsTest.java b/test/distributed/org/apache/cassandra/distributed/test/SnapshotsTest.java
@@ -49,6 +49,7 @@
 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;
@@ -164,7 +165,7 @@ public void testLocalOrReplicatedSystemTablesSnapshotsDoNotHaveSchema()
     }
 
     @Test
-    public void testMissingManifestIsCreatedOnStartup()
+    public void testMissingManifestIsCreatedOnStartupWithEnrichmentEnabled()
     {
         cluster.schemaChange(withKeyspace("CREATE TABLE %s.tbl (key int, value text, PRIMARY KEY (key))"));
         populate(cluster);
@@ -196,6 +197,43 @@ public void testMissingManifestIsCreatedOnStartup()
         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()
     {