Fix variable quoting and profile path in optimized_clang.sh

Co-authored-by: mykaul <4655593+mykaul@users.noreply.github.com>

api/api-doc/storage_service.json

@@ -729,14 +729,6 @@
                      "allowMultiple":false,
                      "type":"boolean",
                      "paramType":"query"
-                  },
-                  {
-                     "name":"use_sstable_identifier",
-                     "description":"Use the sstable identifier UUID, if available, rather than the sstable generation.",
-                     "required":false,
-                     "allowMultiple":false,
-                     "type":"boolean",
-                     "paramType":"query"
                   }
                ]
             },

api/storage_service.cc

@@ -2020,16 +2020,12 @@ void set_snapshot(http_context& ctx, routes& r, sharded<db::snapshot_ctl>& snap_
         auto tag = req->get_query_param("tag");
         auto column_families = split(req->get_query_param("cf"), ",");
         auto sfopt = req->get_query_param("sf");
-        auto usiopt = req->get_query_param("use_sstable_identifier");
-        db::snapshot_options opts = {
-            .skip_flush = strcasecmp(sfopt.c_str(), "true") == 0,
-            .use_sstable_identifier = strcasecmp(usiopt.c_str(), "true") == 0
-        };
+        auto sf = db::snapshot_ctl::skip_flush(strcasecmp(sfopt.c_str(), "true") == 0);
 
         std::vector<sstring> keynames = split(req->get_query_param("kn"), ",");
         try {
             if (column_families.empty()) {
-                co_await snap_ctl.local().take_snapshot(tag, keynames, opts);
+                co_await snap_ctl.local().take_snapshot(tag, keynames, sf);
             } else {
                 if (keynames.empty()) {
                     throw httpd::bad_param_exception("The keyspace of column families must be specified");
@@ -2037,7 +2033,7 @@ void set_snapshot(http_context& ctx, routes& r, sharded<db::snapshot_ctl>& snap_
                 if (keynames.size() > 1) {
                     throw httpd::bad_param_exception("Only one keyspace allowed when specifying a column family");
                 }
-                co_await snap_ctl.local().take_column_family_snapshot(keynames[0], column_families, tag, opts);
+                co_await snap_ctl.local().take_column_family_snapshot(keynames[0], column_families, tag, sf);
             }
             co_return json_void();
         } catch (...) {
@@ -2072,8 +2068,7 @@ void set_snapshot(http_context& ctx, routes& r, sharded<db::snapshot_ctl>& snap_
         auto info = parse_scrub_options(ctx, std::move(req));
 
         if (!info.snapshot_tag.empty()) {
-            db::snapshot_options opts = {.skip_flush = false, .use_sstable_identifier = false};
-            co_await snap_ctl.local().take_column_family_snapshot(info.keyspace, info.column_families, info.snapshot_tag, opts);
+            co_await snap_ctl.local().take_column_family_snapshot(info.keyspace, info.column_families, info.snapshot_tag, db::snapshot_ctl::skip_flush::no);
         }
 
         compaction::compaction_stats stats;

api/tasks.cc

@@ -146,8 +146,7 @@ void set_tasks_compaction_module(http_context& ctx, routes& r, sharded<service::
         auto info = parse_scrub_options(ctx, std::move(req));
 
         if (!info.snapshot_tag.empty()) {
-            db::snapshot_options opts = {.skip_flush = false, .use_sstable_identifier = false};
-            co_await snap_ctl.local().take_column_family_snapshot(info.keyspace, info.column_families, info.snapshot_tag, opts);
+            co_await snap_ctl.local().take_column_family_snapshot(info.keyspace, info.column_families, info.snapshot_tag, db::snapshot_ctl::skip_flush::no);
         }
 
         auto& compaction_module = db.local().get_compaction_manager().get_task_manager_module();

db/snapshot-ctl.cc

@@ -65,7 +65,7 @@ future<> snapshot_ctl::run_snapshot_modify_operation(noncopyable_function<future
     });
 }
 
-future<> snapshot_ctl::take_snapshot(sstring tag, std::vector<sstring> keyspace_names, snapshot_options opts) {
+future<> snapshot_ctl::take_snapshot(sstring tag, std::vector<sstring> keyspace_names, skip_flush sf) {
     if (tag.empty()) {
         throw std::runtime_error("You must supply a snapshot name.");
     }
@@ -74,21 +74,21 @@ future<> snapshot_ctl::take_snapshot(sstring tag, std::vector<sstring> keyspace_
         std::ranges::copy(_db.local().get_keyspaces() | std::views::keys, std::back_inserter(keyspace_names));
     };
 
-    return run_snapshot_modify_operation([tag = std::move(tag), keyspace_names = std::move(keyspace_names), opts, this] () mutable {
-        return do_take_snapshot(std::move(tag), std::move(keyspace_names), opts);
+    return run_snapshot_modify_operation([tag = std::move(tag), keyspace_names = std::move(keyspace_names), sf, this] () mutable {
+        return do_take_snapshot(std::move(tag), std::move(keyspace_names), sf);
     });
 }
 
-future<> snapshot_ctl::do_take_snapshot(sstring tag, std::vector<sstring> keyspace_names, snapshot_options opts) {
+future<> snapshot_ctl::do_take_snapshot(sstring tag, std::vector<sstring> keyspace_names, skip_flush sf) {
     co_await coroutine::parallel_for_each(keyspace_names, [tag, this] (const auto& ks_name) {
         return check_snapshot_not_exist(ks_name, tag);
     });
-    co_await coroutine::parallel_for_each(keyspace_names, [this, tag = std::move(tag), opts] (const auto& ks_name) {
-        return replica::database::snapshot_keyspace_on_all_shards(_db, ks_name, tag, opts);
+    co_await coroutine::parallel_for_each(keyspace_names, [this, tag = std::move(tag), sf] (const auto& ks_name) {
+        return replica::database::snapshot_keyspace_on_all_shards(_db, ks_name, tag, bool(sf));
     });
 }
 
-future<> snapshot_ctl::take_column_family_snapshot(sstring ks_name, std::vector<sstring> tables, sstring tag, snapshot_options opts) {
+future<> snapshot_ctl::take_column_family_snapshot(sstring ks_name, std::vector<sstring> tables, sstring tag, skip_flush sf) {
     if (ks_name.empty()) {
         throw std::runtime_error("You must supply a keyspace name");
     }
@@ -99,14 +99,14 @@ future<> snapshot_ctl::take_column_family_snapshot(sstring ks_name, std::vector<
         throw std::runtime_error("You must supply a snapshot name.");
     }
 
-    return run_snapshot_modify_operation([this, ks_name = std::move(ks_name), tables = std::move(tables), tag = std::move(tag), opts] () mutable {
-        return do_take_column_family_snapshot(std::move(ks_name), std::move(tables), std::move(tag), opts);
+    return run_snapshot_modify_operation([this, ks_name = std::move(ks_name), tables = std::move(tables), tag = std::move(tag), sf] () mutable {
+        return do_take_column_family_snapshot(std::move(ks_name), std::move(tables), std::move(tag), sf);
     });
 }
 
-future<> snapshot_ctl::do_take_column_family_snapshot(sstring ks_name, std::vector<sstring> tables, sstring tag, snapshot_options opts) {
+future<> snapshot_ctl::do_take_column_family_snapshot(sstring ks_name, std::vector<sstring> tables, sstring tag, skip_flush sf) {
     co_await check_snapshot_not_exist(ks_name, tag, tables);
-    co_await replica::database::snapshot_tables_on_all_shards(_db, ks_name, std::move(tables), std::move(tag), opts);
+    co_await replica::database::snapshot_tables_on_all_shards(_db, ks_name, std::move(tables), std::move(tag), bool(sf));
 }
 
 future<> snapshot_ctl::clear_snapshot(sstring tag, std::vector<sstring> keyspace_names, sstring cf_name) {

db/snapshot-ctl.hh

@@ -38,13 +38,10 @@ class backup_task_impl;
 
 } // snapshot namespace
 
-struct snapshot_options {
-    bool skip_flush = false;
-    bool use_sstable_identifier = false;
-};
-
 class snapshot_ctl : public peering_sharded_service<snapshot_ctl> {
 public:
+    using skip_flush = bool_class<class skip_flush_tag>;
+
     struct table_snapshot_details {
         int64_t total;
         int64_t live;
@@ -73,8 +70,8 @@ public:
      *
      * @param tag the tag given to the snapshot; may not be null or empty
      */
-    future<> take_snapshot(sstring tag, snapshot_options opts = {}) {
-        return take_snapshot(tag, {}, opts);
+    future<> take_snapshot(sstring tag, skip_flush sf = skip_flush::no) {
+        return take_snapshot(tag, {}, sf);
     }
 
     /**
@@ -83,7 +80,7 @@ public:
      * @param tag the tag given to the snapshot; may not be null or empty
      * @param keyspace_names the names of the keyspaces to snapshot; empty means "all"
      */
-    future<> take_snapshot(sstring tag, std::vector<sstring> keyspace_names, snapshot_options opts = {});
+    future<> take_snapshot(sstring tag, std::vector<sstring> keyspace_names, skip_flush sf = skip_flush::no);
 
     /**
      * Takes the snapshot of multiple tables. A snapshot name must be specified.
@@ -92,7 +89,7 @@ public:
      * @param tables a vector of tables names to snapshot
      * @param tag the tag given to the snapshot; may not be null or empty
      */
-    future<> take_column_family_snapshot(sstring ks_name, std::vector<sstring> tables, sstring tag, snapshot_options opts = {});
+    future<> take_column_family_snapshot(sstring ks_name, std::vector<sstring> tables, sstring tag, skip_flush sf = skip_flush::no);
 
     /**
      * Remove the snapshot with the given name from the given keyspaces.
@@ -130,8 +127,8 @@ private:
 
     friend class snapshot::backup_task_impl;
 
-    future<> do_take_snapshot(sstring tag, std::vector<sstring> keyspace_names, snapshot_options opts = {}  );
-    future<> do_take_column_family_snapshot(sstring ks_name, std::vector<sstring> tables, sstring tag, snapshot_options opts = {});
+    future<> do_take_snapshot(sstring tag, std::vector<sstring> keyspace_names, skip_flush sf = skip_flush::no);
+    future<> do_take_column_family_snapshot(sstring ks_name, std::vector<sstring> tables, sstring tag, skip_flush sf = skip_flush::no);
 };
 
 }

docs/operating-scylla/nodetool-commands/snapshot.rst

@@ -17,7 +17,7 @@ SYNOPSIS
                    [(-u <username> | --username <username>)] snapshot
                    [(-cf <table> | --column-family <table> | --table <table>)]
                    [(-kc <kclist> | --kc.list <kclist>)]
-                   [(-sf | --skip-flush)] [--use-sstable-identifier] [(-t <tag> | --tag <tag>)] [--] [<keyspaces...>]
+                   [(-sf | --skip-flush)] [(-t <tag> | --tag <tag>)] [--] [<keyspaces...>]
 
 OPTIONS
 .......
@@ -37,8 +37,6 @@ Parameter                                                             Descriptio
 --------------------------------------------------------------------  -------------------------------------------------------------------------------------
 -sf / --skip-flush                                                    Do not flush memtables before snapshotting (snapshot will not contain unflushed data)
 --------------------------------------------------------------------  -------------------------------------------------------------------------------------
---use-sstable-identifier                                              Use the sstable identifier UUID, if available, rather than the sstable generation.
---------------------------------------------------------------------  -------------------------------------------------------------------------------------
 -t <tag> / --tag <tag>                                                The name of the snapshot
 ====================================================================  =====================================================================================
 

replica/database.cc

@@ -2810,26 +2810,26 @@ future<> database::drop_cache_for_keyspace_on_all_shards(sharded<database>& shar
     });
 }
 
-future<> database::snapshot_table_on_all_shards(sharded<database>& sharded_db, table_id uuid, sstring tag, db::snapshot_options opts) {
-    if (!opts.skip_flush) {
+future<> database::snapshot_table_on_all_shards(sharded<database>& sharded_db, table_id uuid, sstring tag, bool skip_flush) {
+    if (!skip_flush) {
         co_await flush_table_on_all_shards(sharded_db, uuid);
     }
     auto table_shards = co_await get_table_on_all_shards(sharded_db, uuid);
-    co_await table::snapshot_on_all_shards(sharded_db, table_shards, tag, opts);
+    co_await table::snapshot_on_all_shards(sharded_db, table_shards, tag);
 }
 
-future<> database::snapshot_tables_on_all_shards(sharded<database>& sharded_db, std::string_view ks_name, std::vector<sstring> table_names, sstring tag, db::snapshot_options opts) {
-    return parallel_for_each(table_names, [&sharded_db, ks_name, tag = std::move(tag), opts] (auto& table_name) {
+future<> database::snapshot_tables_on_all_shards(sharded<database>& sharded_db, std::string_view ks_name, std::vector<sstring> table_names, sstring tag, bool skip_flush) {
+    return parallel_for_each(table_names, [&sharded_db, ks_name, tag = std::move(tag), skip_flush] (auto& table_name) {
         auto uuid = sharded_db.local().find_uuid(ks_name, table_name);
-        return snapshot_table_on_all_shards(sharded_db, uuid, tag, opts);
+        return snapshot_table_on_all_shards(sharded_db, uuid, tag, skip_flush);
     });
 }
 
-future<> database::snapshot_keyspace_on_all_shards(sharded<database>& sharded_db, std::string_view ks_name, sstring tag, db::snapshot_options opts) {
+future<> database::snapshot_keyspace_on_all_shards(sharded<database>& sharded_db, std::string_view ks_name, sstring tag, bool skip_flush) {
     auto& ks = sharded_db.local().find_keyspace(ks_name);
-    co_await coroutine::parallel_for_each(ks.metadata()->cf_meta_data(), [&, tag = std::move(tag), opts] (const auto& pair) -> future<> {
+    co_await coroutine::parallel_for_each(ks.metadata()->cf_meta_data(), [&, tag = std::move(tag), skip_flush] (const auto& pair) -> future<> {
         auto uuid = pair.second->id();
-        co_await snapshot_table_on_all_shards(sharded_db, uuid, tag, opts);
+        co_await snapshot_table_on_all_shards(sharded_db, uuid, tag, skip_flush);
     });
 }
 
@@ -2951,12 +2951,7 @@ future<> database::truncate_table_on_all_shards(sharded<database>& sharded_db, s
         auto truncated_at = truncated_at_opt.value_or(db_clock::now());
         auto name = snapshot_name_opt.value_or(
             format("{:d}-{}", truncated_at.time_since_epoch().count(), cf.schema()->cf_name()));
-        // Use the sstable identifier in snapshot names to allow de-duplication of sstables
-        // at backup time even if they were migrated across shards or nodes and were renamed a given a new generation.
-        // We hard-code that here since we have no way to pass this option to auto-snapshot and
-        // it is always safe to use the sstable identifier for the sstable generation.
-        auto opts = db::snapshot_options{.use_sstable_identifier = true};
-        co_await table::snapshot_on_all_shards(sharded_db, table_shards, name, opts);
+        co_await table::snapshot_on_all_shards(sharded_db, table_shards, name);
     }
 
     co_await sharded_db.invoke_on_all([&] (database& db) {

replica/database.hh

@@ -1040,12 +1040,12 @@ public:
 private:
     using snapshot_file_set = foreign_ptr<std::unique_ptr<std::unordered_set<sstring>>>;
 
-    future<snapshot_file_set> take_snapshot(sstring jsondir, db::snapshot_options opts);
+    future<snapshot_file_set> take_snapshot(sstring jsondir);
     // Writes the table schema and the manifest of all files in the snapshot directory.
     future<> finalize_snapshot(const global_table_ptr& table_shards, sstring jsondir, std::vector<snapshot_file_set> file_sets);
     static future<> seal_snapshot(sstring jsondir, std::vector<snapshot_file_set> file_sets);
 public:
-    static future<> snapshot_on_all_shards(sharded<database>& sharded_db, const global_table_ptr& table_shards, sstring name, db::snapshot_options opts);
+    static future<> snapshot_on_all_shards(sharded<database>& sharded_db, const global_table_ptr& table_shards, sstring name);
 
     future<std::unordered_map<sstring, snapshot_details>> get_snapshot_details();
     static future<snapshot_details> get_snapshot_details(std::filesystem::path snapshot_dir, std::filesystem::path datadir);
@@ -2009,9 +2009,9 @@ public:
     static future<> drop_cache_for_table_on_all_shards(sharded<database>& sharded_db, table_id id);
     static future<> drop_cache_for_keyspace_on_all_shards(sharded<database>& sharded_db, std::string_view ks_name);
 
-    static future<> snapshot_table_on_all_shards(sharded<database>& sharded_db, table_id id, sstring tag, db::snapshot_options opts);
-    static future<> snapshot_tables_on_all_shards(sharded<database>& sharded_db, std::string_view ks_name, std::vector<sstring> table_names, sstring tag, db::snapshot_options opts);
-    static future<> snapshot_keyspace_on_all_shards(sharded<database>& sharded_db, std::string_view ks_name, sstring tag, db::snapshot_options opts);
+    static future<> snapshot_table_on_all_shards(sharded<database>& sharded_db, table_id id, sstring tag, bool skip_flush);
+    static future<> snapshot_tables_on_all_shards(sharded<database>& sharded_db, std::string_view ks_name, std::vector<sstring> table_names, sstring tag, bool skip_flush);
+    static future<> snapshot_keyspace_on_all_shards(sharded<database>& sharded_db, std::string_view ks_name, sstring tag, bool skip_flush);
 
 public:
     bool update_column_family(schema_ptr s);

replica/table.cc

@@ -3268,7 +3268,7 @@ future<> table::write_schema_as_cql(const global_table_ptr& table_shards, sstrin
 }
 
 // Runs the orchestration code on an arbitrary shard to balance the load.
-future<> table::snapshot_on_all_shards(sharded<database>& sharded_db, const global_table_ptr& table_shards, sstring name, db::snapshot_options opts) {
+future<> table::snapshot_on_all_shards(sharded<database>& sharded_db, const global_table_ptr& table_shards, sstring name) {
     auto* so = std::get_if<storage_options::local>(&table_shards->get_storage_options().value);
     if (so == nullptr) {
         throw std::runtime_error("Snapshotting non-local tables is not implemented");
@@ -3291,7 +3291,7 @@ future<> table::snapshot_on_all_shards(sharded<database>& sharded_db, const glob
         co_await io_check([&jsondir] { return recursive_touch_directory(jsondir); });
         co_await coroutine::parallel_for_each(smp::all_cpus(), [&] (unsigned shard) -> future<> {
             file_sets.emplace_back(co_await smp::submit_to(shard, [&] {
-                return table_shards->take_snapshot(jsondir, opts);
+                return table_shards->take_snapshot(jsondir);
             }));
         });
         co_await io_check(sync_directory, jsondir);
@@ -3300,22 +3300,19 @@ future<> table::snapshot_on_all_shards(sharded<database>& sharded_db, const glob
     });
 }
 
-future<table::snapshot_file_set> table::take_snapshot(sstring jsondir, db::snapshot_options opts) {
-    tlogger.trace("take_snapshot {}: use_sstable_identifier={}", jsondir, opts.use_sstable_identifier);
+future<table::snapshot_file_set> table::take_snapshot(sstring jsondir) {
+    tlogger.trace("take_snapshot {}", jsondir);
 
     auto sstable_deletion_guard = co_await get_sstable_list_permit();
 
     auto tables = *_sstables->all() | std::ranges::to<std::vector<sstables::shared_sstable>>();
     auto table_names = std::make_unique<std::unordered_set<sstring>>();
 
-    auto& ks_name = schema()->ks_name();
-    auto& cf_name = schema()->cf_name();
-    co_await _sstables_manager.dir_semaphore().parallel_for_each(tables, [&, opts] (sstables::shared_sstable sstable) -> future<> {
-        auto gen = co_await io_check([sstable, &dir = jsondir, opts] {
-            return sstable->snapshot(dir, opts.use_sstable_identifier);
+    co_await _sstables_manager.dir_semaphore().parallel_for_each(tables, [&jsondir, &table_names] (sstables::shared_sstable sstable) {
+        table_names->insert(sstable->component_basename(sstables::component_type::Data));
+        return io_check([sstable, &dir = jsondir] {
+            return sstable->snapshot(dir);
         });
-        auto fname = sstable->component_basename(ks_name, cf_name, sstable->get_version(), gen, sstable->get_format(), sstables::component_type::Data);
-        table_names->insert(fname);
     });
     co_return make_foreign(std::move(table_names));
 }

sstables/sstables.cc

@@ -2117,14 +2117,11 @@ sstable::write_scylla_metadata(shard_id shard, struct run_identifier identifier,
     }
 
     sstable_id sid;
-    // Force a random sstable_id for testing purposes
-    bool random_sstable_identifier = utils::get_local_injector().is_enabled("random_sstable_identifier");
-    if (!random_sstable_identifier && generation().is_uuid_based()) {
+    if (generation().is_uuid_based()) {
         sid = sstable_id(generation().as_uuid());
     } else {
         sid = sstable_id(utils::UUID_gen::get_time_UUID());
-        auto msg = random_sstable_identifier ? "forced random sstable_id" : "has numerical generation";
-        sstlog.info("SSTable {} {}. SSTable identifier in scylla_metadata set to {}", get_filename(), msg, sid);
+        sstlog.info("SSTable {} has numerical generation. SSTable identifier in scylla_metadata set to {}", get_filename(), sid);
     }
     _components->scylla_metadata->data.set<scylla_metadata_type::SSTableIdentifier>(scylla_metadata::sstable_identifier{sid});
 
@@ -2538,11 +2535,8 @@ std::vector<std::pair<component_type, sstring>> sstable::all_components() const
     return all;
 }
 
-future<generation_type> sstable::snapshot(const sstring& dir, bool use_sstable_identifier) const {
-    // Use the sstable identifier UUID if available to enable global de-duplication of sstables in backup.
-    generation_type gen = (use_sstable_identifier && _sstable_identifier) ? generation_type(_sstable_identifier->uuid()) : _generation;
-    co_await _storage->snapshot(*this, dir, storage::absolute_path::yes, gen);
-    co_return gen;
+future<> sstable::snapshot(const sstring& dir) const {
+    return _storage->snapshot(*this, dir, storage::absolute_path::yes);
 }
 
 future<> sstable::change_state(sstable_state to, delayed_commit_changes* delay_commit) {

sstables/sstables.hh

@@ -397,10 +397,6 @@ public:
         return _version;
     }
 
-    format_types get_format() const {
-        return _format;
-    }
-
     // Returns the total bytes of all components.
     uint64_t bytes_on_disk() const;
     file_size_stats get_file_size_stats() const;
@@ -442,10 +438,7 @@ public:
 
     std::vector<std::pair<component_type, sstring>> all_components() const;
 
-    // When use_sstable_identifier is true and the sstable identifier is available,
-    // use it to name the sstable in the snapshot, rather than the sstable generation.
-    // Returns the generation used for snapshot.
-    future<generation_type> snapshot(const sstring& dir, bool use_sstable_identifier = false) const;
+    future<> snapshot(const sstring& dir) const;
 
     // Delete the sstable by unlinking all sstable files
     // Ignores all errors.

test/boost/database_test.cc

@@ -31,7 +31,6 @@
 #include "replica/database.hh"
 #include "utils/assert.hh"
 #include "utils/lister.hh"
-#include "utils/rjson.hh"
 #include "partition_slice_builder.hh"
 #include "mutation/frozen_mutation.hh"
 #include "test/lib/mutation_source_test.hh"
@@ -39,7 +38,6 @@
 #include "service/migration_manager.hh"
 #include "sstables/sstables.hh"
 #include "sstables/generation_type.hh"
-#include "sstables/sstable_version.hh"
 #include "db/config.hh"
 #include "db/commitlog/commitlog_replayer.hh"
 #include "db/commitlog/commitlog.hh"
@@ -53,7 +51,6 @@
 #include "db/system_keyspace.hh"
 #include "db/view/view_builder.hh"
 #include "replica/mutation_dump.hh"
-#include "utils/error_injection.hh"
 
 using namespace std::chrono_literals;
 using namespace sstables;
@@ -615,13 +612,13 @@ future<> do_with_some_data(std::vector<sstring> cf_names, std::function<future<>
     });
 }
 
-future<> take_snapshot(cql_test_env& e, sstring ks_name = "ks", sstring cf_name = "cf", sstring snapshot_name = "test", db::snapshot_options opts = {}) {
+future<> take_snapshot(cql_test_env& e, sstring ks_name = "ks", sstring cf_name = "cf", sstring snapshot_name = "test", bool skip_flush = false) {
     try {
         auto uuid = e.db().local().find_uuid(ks_name, cf_name);
-        co_await replica::database::snapshot_table_on_all_shards(e.db(), uuid, snapshot_name, opts);
+        co_await replica::database::snapshot_table_on_all_shards(e.db(), uuid, snapshot_name, skip_flush);
     } catch (...) {
-        testlog.error("Could not take snapshot for {}.{} snapshot_name={} skip_flush={} use_sstable_identifier={}: {}",
-                ks_name, cf_name, snapshot_name, opts.skip_flush, opts.use_sstable_identifier, std::current_exception());
+        testlog.error("Could not take snapshot for {}.{} snapshot_name={} skip_flush={}: {}",
+                ks_name, cf_name, snapshot_name, skip_flush, std::current_exception());
         throw;
     }
 }
@@ -635,37 +632,6 @@ future<std::set<sstring>> collect_files(fs::path path) {
     co_return ret;
 }
 
-static bool is_component(const sstring& fname, const sstring& suffix) {
-    return fname.ends_with(suffix);
-}
-
-static std::set<sstring> collect_sstables(const std::set<sstring>& all_files, const sstring& suffix) {
-    // Verify manifest against the files in the snapshots dir
-    auto pred = [&suffix] (const sstring& fname) {
-        return is_component(fname, suffix);
-    };
-    return std::ranges::filter_view(all_files, pred) | std::ranges::to<std::set<sstring>>();
-}
-
-// Validate that the manifest.json lists exactly the SSTables present in the snapshot directory
-static future<> validate_manifest(const fs::path& snapshot_dir, const std::set<sstring>& in_snapshot_dir) {
-    sstring suffix = "-Data.db";
-    auto sstables_in_snapshot = collect_sstables(in_snapshot_dir, suffix);
-
-    std::set<sstring> sstables_in_manifest;
-    auto manifest_str = co_await util::read_entire_file_contiguous(snapshot_dir / "manifest.json");
-    auto manifest_json = rjson::parse(manifest_str);
-    auto& manifest_files = manifest_json["files"];
-    BOOST_REQUIRE(manifest_files.IsArray());
-    for (auto& f : manifest_files.GetArray()) {
-        if (is_component(f.GetString(), suffix)) {
-            sstables_in_manifest.insert(f.GetString());
-        }
-    }
-    testlog.debug("SSTables in manifest.json: {}", fmt::join(sstables_in_manifest, ", "));
-    BOOST_REQUIRE_EQUAL(sstables_in_snapshot, sstables_in_manifest);
-}
-
 static future<> snapshot_works(const std::string& table_name) {
     return do_with_some_data({"cf"}, [table_name] (cql_test_env& e) {
         take_snapshot(e, "ks", table_name).get();
@@ -685,8 +651,6 @@ static future<> snapshot_works(const std::string& table_name) {
         // all files were copied and manifest was generated
         BOOST_REQUIRE_EQUAL(in_table_dir, in_snapshot_dir);
 
-        validate_manifest(snapshot_dir, in_snapshot_dir).get();
-
         return make_ready_future<>();
     }, true);
 }
@@ -705,8 +669,7 @@ SEASTAR_TEST_CASE(index_snapshot_works) {
 
 SEASTAR_TEST_CASE(snapshot_skip_flush_works) {
     return do_with_some_data({"cf"}, [] (cql_test_env& e) {
-        db::snapshot_options opts = {.skip_flush = true};
-        take_snapshot(e, "ks", "cf", "test", opts).get();
+        take_snapshot(e, "ks", "cf", "test", true /* skip_flush */).get();
 
         auto& cf = e.local_db().find_column_family("ks", "cf");
 
@@ -719,41 +682,6 @@ SEASTAR_TEST_CASE(snapshot_skip_flush_works) {
     });
 }
 
-SEASTAR_TEST_CASE(snapshot_use_sstable_identifier_works) {
-#ifndef SCYLLA_ENABLE_ERROR_INJECTION
-        fmt::print("Skipping test as it depends on error injection. Please run in mode where it's enabled (debug,dev).\n");
-        return make_ready_future<>();
-#endif
-    sstring table_name = "cf";
-    // Force random sstable identifiers, otherwise the initial sstable_id is equal
-    // to the sstable generation and the test can't distinguish between them.
-    utils::get_local_injector().enable("random_sstable_identifier", false);
-    return do_with_some_data({table_name}, [table_name] (cql_test_env& e) -> future<> {
-        sstring tag = "test";
-        db::snapshot_options opts = {.use_sstable_identifier = true};
-        co_await take_snapshot(e, "ks", table_name, tag, opts);
-
-        auto& cf = e.local_db().find_column_family("ks", table_name);
-        auto table_directory = table_dir(cf);
-        auto snapshot_dir = table_directory / sstables::snapshots_dir / tag;
-        auto in_table_dir = co_await collect_files(table_directory);
-        // snapshot triggered a flush and wrote the data down.
-        BOOST_REQUIRE_GE(in_table_dir.size(), 9);
-        testlog.info("Files in table dir: {}", fmt::join(in_table_dir, ", "));
-
-        auto in_snapshot_dir = co_await collect_files(snapshot_dir);
-        testlog.info("Files in snapshot dir: {}", fmt::join(in_snapshot_dir, ", "));
-
-        in_table_dir.insert("manifest.json");
-        in_table_dir.insert("schema.cql");
-        // all files were copied and manifest was generated
-        BOOST_REQUIRE_EQUAL(in_table_dir.size(), in_snapshot_dir.size());
-        BOOST_REQUIRE_NE(in_table_dir, in_snapshot_dir);
-
-        co_await validate_manifest(snapshot_dir, in_snapshot_dir);
-    }, true);
-}
-
 SEASTAR_TEST_CASE(snapshot_list_okay) {
     return do_with_some_data({"cf"}, [] (cql_test_env& e) {
         auto& cf = e.local_db().find_column_family("ks", "cf");
@@ -1528,7 +1456,7 @@ SEASTAR_TEST_CASE(snapshot_with_quarantine_works) {
         }
         BOOST_REQUIRE(found);
 
-        co_await take_snapshot(e, "ks", "cf", "test", db::snapshot_options{.skip_flush = true});
+        co_await take_snapshot(e, "ks", "cf", "test", true /* skip_flush */);
 
         testlog.debug("Expected: {}", expected);
 

test/cqlpy/nodetool.py

@@ -115,7 +115,7 @@ def compact_keyspace(cql, ks, flush_memtables=True):
         args.extend([ks, cf])
         run_nodetool(cql, "compact", *args)
 
-def take_snapshot(cql, table, tag, skip_flush, use_sstable_identifier=False):
+def take_snapshot(cql, table, tag, skip_flush):
     ks, cf = table.split('.')
     if has_rest_api(cql):
         requests.post(f'{rest_api_url(cql)}/storage_service/snapshots/', params={'kn': ks, 'cf' : cf, 'tag': tag, 'sf': skip_flush})
@@ -123,8 +123,6 @@ def take_snapshot(cql, table, tag, skip_flush, use_sstable_identifier=False):
         args = ['--tag', tag, '--table', cf]
         if skip_flush:
             args.append('--skip-flush')
-        if use_sstable_identifier:
-            args.append('--use-sstable-identifier')
         args.append(ks)
         run_nodetool(cql, "snapshot", *args)
 

test/nodetool/test_snapshot.py

@@ -99,7 +99,7 @@ def test_listsnapshots_no_snapshots(nodetool, request):
         assert res.stdout == "Snapshot Details: \nThere are no snapshots\n"
 
 
-def check_snapshot_out(res, tag, ktlist, skip_flush, use_sstable_identifier=False):
+def check_snapshot_out(res, tag, ktlist, skip_flush):
     """Check that the output of nodetool snapshot contains the expected messages"""
 
     if len(ktlist) == 0:
@@ -110,7 +110,7 @@ def check_snapshot_out(res, tag, ktlist, skip_flush, use_sstable_identifier=Fals
     pattern = re.compile("Requested creating snapshot\\(s\\)"
                          f" for \\[{keyspaces}\\]"
                          f" with snapshot name \\[(.+)\\]"
-                         f" and options \\{{skip_flush={str(skip_flush).lower()}, use_sstable_identifier={str(use_sstable_identifier).lower()}\\}}")
+                         f" and options \\{{skipFlush={str(skip_flush).lower()}\\}}")
 
     print(res)
     print(pattern)
@@ -138,13 +138,13 @@ def test_snapshot_keyspace(nodetool):
 
     res = nodetool("snapshot", "--tag", tag, "ks1", expected_requests=[
         expected_request("POST", "/storage_service/snapshots",
-                         params={"tag": tag, "sf": "false", "use_sstable_identifier": "false", "kn": "ks1"})
+                         params={"tag": tag, "sf": "false", "kn": "ks1"})
     ])
     check_snapshot_out(res.stdout, tag, ["ks1"], False)
 
     res = nodetool("snapshot", "--tag", tag, "ks1", "ks2", expected_requests=[
         expected_request("POST", "/storage_service/snapshots",
-                         params={"tag": tag, "sf": "false", "use_sstable_identifier": "false", "kn": "ks1,ks2"})
+                         params={"tag": tag, "sf": "false", "kn": "ks1,ks2"})
     ])
     check_snapshot_out(res.stdout, tag, ["ks1", "ks2"], False)
 
@@ -155,13 +155,13 @@ def test_snapshot_keyspace_with_table(nodetool, option_name):
 
     res = nodetool("snapshot", "--tag", tag, "ks1", option_name, "tbl", expected_requests=[
         expected_request("POST", "/storage_service/snapshots",
-                         params={"tag": tag, "sf": "false", "use_sstable_identifier": "false", "kn": "ks1", "cf": "tbl"})
+                         params={"tag": tag, "sf": "false", "kn": "ks1", "cf": "tbl"})
     ])
     check_snapshot_out(res.stdout, tag, ["ks1"], False)
 
     res = nodetool("snapshot", "--tag", tag, "ks1", option_name, "tbl1,tbl2", expected_requests=[
         expected_request("POST", "/storage_service/snapshots",
-                         params={"tag": tag, "sf": "false", "use_sstable_identifier": "false", "kn": "ks1", "cf": "tbl1,tbl2"})
+                         params={"tag": tag, "sf": "false", "kn": "ks1", "cf": "tbl1,tbl2"})
     ])
     check_snapshot_out(res.stdout, tag, ["ks1"], False)
 
@@ -186,7 +186,7 @@ class kn_param(NamedTuple):
 def test_snapshot_keyspace_table_single_arg(nodetool, param, scylla_only):
     tag = "my_snapshot"
 
-    req_params = {"tag": tag, "sf": "false", "use_sstable_identifier": "false", "kn": param.kn}
+    req_params = {"tag": tag, "sf": "false", "kn": param.kn}
     if param.cf:
         req_params["cf"] = param.cf
 
@@ -202,19 +202,19 @@ def test_snapshot_ktlist(nodetool, option_name):
 
     res = nodetool("snapshot", "--tag", tag, option_name, "ks1.tbl1", expected_requests=[
         expected_request("POST", "/storage_service/snapshots",
-                         params={"tag": tag, "sf": "false", "use_sstable_identifier": "false", "kn": "ks1", "cf": "tbl1"})
+                         params={"tag": tag, "sf": "false", "kn": "ks1", "cf": "tbl1"})
     ])
     check_snapshot_out(res.stdout, tag, ["ks1.tbl1"], False)
 
     res = nodetool("snapshot", "--tag", tag, option_name, "ks1.tbl1,ks2.tbl2", expected_requests=[
         expected_request("POST", "/storage_service/snapshots",
-                         params={"tag": tag, "sf": "false", "use_sstable_identifier": "false", "kn": "ks1.tbl1,ks2.tbl2"})
+                         params={"tag": tag, "sf": "false", "kn": "ks1.tbl1,ks2.tbl2"})
     ])
     check_snapshot_out(res.stdout, tag, ["ks1.tbl1", "ks2.tbl2"], False)
 
     res = nodetool("snapshot", "--tag", tag, option_name, "ks1,ks2", expected_requests=[
         expected_request("POST", "/storage_service/snapshots",
-                         params={"tag": tag, "sf": "false", "use_sstable_identifier": "false", "kn": "ks1,ks2"})
+                         params={"tag": tag, "sf": "false", "kn": "ks1,ks2"})
     ])
     check_snapshot_out(res.stdout, tag, ["ks1" ,"ks2"], False)
 
@@ -229,8 +229,7 @@ def test_snapshot_ktlist(nodetool, option_name):
     {"ks": ["ks1", "ks2"], "tbl": []},
 ])
 @pytest.mark.parametrize("skip_flush", [False, True])
-@pytest.mark.parametrize("use_sstable_identifier", [False, True])
-def test_snapshot_options_matrix(nodetool, tag, ktlist, skip_flush, use_sstable_identifier):
+def test_snapshot_options_matrix(nodetool, tag, ktlist, skip_flush):
     cmd = ["snapshot"]
     params = {}
 
@@ -243,11 +242,8 @@ def test_snapshot_options_matrix(nodetool, tag, ktlist, skip_flush, use_sstable_
 
     if skip_flush:
         cmd.append("--skip-flush")
-    if use_sstable_identifier:
-        cmd.append("--use-sstable-identifier")
 
     params["sf"] = str(skip_flush).lower()
-    params["use_sstable_identifier"] = str(use_sstable_identifier).lower()
 
     if ktlist:
         if "tbl" in ktlist:
@@ -277,7 +273,7 @@ def test_snapshot_options_matrix(nodetool, tag, ktlist, skip_flush, use_sstable_
         expected_request("POST", "/storage_service/snapshots", params=params)
     ])
 
-    check_snapshot_out(res.stdout, tag, keyspaces, skip_flush, use_sstable_identifier)
+    check_snapshot_out(res.stdout, tag, keyspaces, skip_flush)
 
 
 def test_snapshot_multiple_keyspace_with_table(nodetool):

tools/scylla-nodetool.cc

@@ -2362,23 +2362,16 @@ void snapshot_operation(scylla_rest_client& client, const bpo::variables_map& vm
         params["sf"] = "false";
     }
 
-    if (vm.contains("use-sstable-identifier")) {
-        params["use_sstable_identifier"] = "true";
-    } else {
-        params["use_sstable_identifier"] = "false";
-    }
-
     client.post("/storage_service/snapshots", params);
 
     if (kn_msg.empty()) {
         kn_msg = params["kn"];
     }
 
-    fmt::print(std::cout, "Requested creating snapshot(s) for [{}] with snapshot name [{}] and options {{skip_flush={}, use_sstable_identifier={}}}\n",
+    fmt::print(std::cout, "Requested creating snapshot(s) for [{}] with snapshot name [{}] and options {{skipFlush={}}}\n",
             kn_msg,
             params["tag"],
-            params["sf"],
-            params["use_sstable_identifier"]);
+            params["sf"]);
     fmt::print(std::cout, "Snapshot directory: {}\n", params["tag"]);
 }
 
@@ -4605,7 +4598,6 @@ For more information, see: {}
                     typed_option<sstring>("keyspace-table-list", "The keyspace.table pair(s) to snapshot, multiple ones can be joined with ','"),
                     typed_option<sstring>("tag,t", "The name of the snapshot"),
                     typed_option<>("skip-flush", "Do not flush memtables before snapshotting (snapshot will not contain unflushed data)"),
-                    typed_option<>("use-sstable-identifier", "Use the sstable identifier UUID, if available, rather than the sstable generation for the sstable file names within the snapshot dir and the manifest file"),
                 },
                 {
                     typed_option<std::vector<sstring>>("keyspaces", "The keyspaces to snapshot", -1),

tools/toolchain/optimized_clang.sh

@@ -123,7 +123,7 @@ _get_distribution_components() {
                 continue
                 ;;
         esac
-        echo $target
+        echo "$target"
     done
 }
 
@@ -164,7 +164,7 @@ if [[ "${CLANG_BUILD}" = "INSTALL" ]]; then
 
     echo "[clang-stage3] build the compiler applied CSPGO profile"
     cd "${CLANG_BUILD_DIR}"
-    llvm-profdata merge build/csprofiles/default_*.profraw -output=csir.prof
+    llvm-profdata merge build/profiles/csir-*.profraw -output=csir.prof
     llvm-profdata merge ir.prof csir.prof -output=combined.prof
     rm -rf build
     # linker flags are needed for BOLT