From 44b811ce191e0c9653c612d485bf8ce82c5bf8e3 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Fri, 26 May 2023 17:10:08 +0300 Subject: [PATCH 1/8] test: Don't create directory for system tables in cql_test_env The distributed_loader::init_system_keyspaces() does it when called few lines above this place Signed-off-by: Pavel Emelyanov --- test/lib/cql_test_env.cc | 5 ----- 1 file changed, 5 deletions(-) diff --git a/test/lib/cql_test_env.cc b/test/lib/cql_test_env.cc index 959d9c53f0..9a7db1d0cc 100644 --- a/test/lib/cql_test_env.cc +++ b/test/lib/cql_test_env.cc @@ -843,11 +843,6 @@ public: replica::distributed_loader::init_system_keyspace(sys_ks, db, ss, gossiper, raft_gr, *cfg, p).get(); } - auto& ks = db.local().find_keyspace(db::system_keyspace::NAME); - parallel_for_each(ks.metadata()->cf_meta_data(), [&ks] (auto& pair) { - auto cfm = pair.second; - return ks.make_directory_for_column_family(cfm->cf_name(), cfm->id()); - }).get(); replica::distributed_loader::init_non_system_keyspaces(db, proxy, sys_ks).get(); db.invoke_on_all([] (replica::database& db) { From 6db5f08eabddff3cdd8b122a0b10cb78424b96ab Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Fri, 26 May 2023 17:14:50 +0300 Subject: [PATCH 2/8] distributed_loader: Use cf.dir() instead of ks.column_family_directory() These two return the same, but the latter makes it the harder way Signed-off-by: Pavel Emelyanov --- replica/distributed_loader.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/replica/distributed_loader.cc b/replica/distributed_loader.cc index d9d23f78ba..a54220907a 100644 --- a/replica/distributed_loader.cc +++ b/replica/distributed_loader.cc @@ -723,7 +723,6 @@ future<> distributed_loader::populate_keyspace(distributed& d } sstring cfname = cf->schema()->cf_name(); - auto sstdir = ks.column_family_directory(ksdir, cfname, uuid); dblog.info("Keyspace {}: Reading CF {} id={} version={} storage={}", ks_name, cfname, uuid, s->version(), cf->get_storage_options().type_string()); auto gtable = co_await get_table_on_all_shards(db, ks_name, cfname); @@ -738,9 +737,9 @@ future<> distributed_loader::populate_keyspace(distributed& d std::exception_ptr eptr = std::current_exception(); std::string msg = format("Exception while populating keyspace '{}' with column family '{}' from file '{}': {}", - ks_name, cfname, sstdir, eptr); + ks_name, cfname, cf->dir(), eptr); dblog.error("Exception while populating keyspace '{}' with column family '{}' from file '{}': {}", - ks_name, cfname, sstdir, eptr); + ks_name, cfname, cf->dir(), eptr); try { std::rethrow_exception(eptr); } catch (sstables::compaction_stopped_exception& e) { From a19b8af187c83d35139188e3e2bc8d6241ee4026 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Fri, 26 May 2023 17:20:31 +0300 Subject: [PATCH 3/8] table: Relocate ks.make_directory_for_column_family() This method initializes storage for table naturally belongs to that class. So rename it while moving. Also, there's no longer need to carry table name and uuid as arguments, being table method it can just get the paths to work on from config Signed-off-by: Pavel Emelyanov --- replica/database.cc | 10 +++++----- replica/database.hh | 2 +- replica/distributed_loader.cc | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/replica/database.cc b/replica/database.cc index a0e208a789..c4c30b8e85 100644 --- a/replica/database.cc +++ b/replica/database.cc @@ -993,8 +993,9 @@ void database::add_column_family(keyspace& ks, schema_ptr schema, column_family: future<> database::add_column_family_and_make_directory(schema_ptr schema) { auto& ks = find_keyspace(schema->ks_name()); add_column_family(ks, schema, ks.make_column_family_config(*schema, *this)); - find_column_family(schema).get_index_manager().reload(); - return ks.make_directory_for_column_family(schema->cf_name(), schema->id()); + auto& cf = find_column_family(schema); + cf.get_index_manager().reload(); + return cf.init_storage(); } bool database::update_column_family(schema_ptr new_schema) { @@ -1316,11 +1317,10 @@ keyspace::column_family_directory(const sstring& base_path, const sstring& name, return format("{}/{}-{}", base_path, name, uuid_sstring); } -future<> -keyspace::make_directory_for_column_family(const sstring& name, table_id uuid) { +future<> table::init_storage() { std::vector cfdirs; for (auto& extra : _config.all_datadirs) { - cfdirs.push_back(column_family_directory(extra, name, uuid)); + cfdirs.push_back(extra); } return parallel_for_each(cfdirs, [] (sstring cfdir) { return io_check([cfdir] { return recursive_touch_directory(cfdir); }); diff --git a/replica/database.hh b/replica/database.hh index 2ade3fffd7..c16dd1eb34 100644 --- a/replica/database.hh +++ b/replica/database.hh @@ -626,6 +626,7 @@ public: const storage_options& get_storage_options() const noexcept { return *_storage_opts; } lw_shared_ptr get_storage_options_ptr() const noexcept { return _storage_opts; } + future<> init_storage(); seastar::gate& async_gate() { return _async_gate; } @@ -1225,7 +1226,6 @@ public: locator::vnode_effective_replication_map_ptr get_effective_replication_map() const; column_family::config make_column_family_config(const schema& s, const database& db) const; - future<> make_directory_for_column_family(const sstring& name, table_id uuid); void add_or_update_column_family(const schema_ptr& s); void add_user_type(const user_type ut); void remove_user_type(const user_type ut); diff --git a/replica/distributed_loader.cc b/replica/distributed_loader.cc index a54220907a..1860b79f1b 100644 --- a/replica/distributed_loader.cc +++ b/replica/distributed_loader.cc @@ -730,7 +730,7 @@ future<> distributed_loader::populate_keyspace(distributed& d std::exception_ptr ex; try { - co_await ks.make_directory_for_column_family(cfname, uuid); + co_await cf->init_storage(); co_await metadata.start(); } catch (...) { From 99dfade020140c9d04463849ab78241233c17503 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Fri, 26 May 2023 18:09:01 +0300 Subject: [PATCH 4/8] table: Coroutinize init_storage() Signed-off-by: Pavel Emelyanov --- replica/database.cc | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/replica/database.cc b/replica/database.cc index c4c30b8e85..ba352e0c15 100644 --- a/replica/database.cc +++ b/replica/database.cc @@ -1322,13 +1322,11 @@ future<> table::init_storage() { for (auto& extra : _config.all_datadirs) { cfdirs.push_back(extra); } - return parallel_for_each(cfdirs, [] (sstring cfdir) { + co_await coroutine::parallel_for_each(cfdirs, [] (sstring cfdir) { return io_check([cfdir] { return recursive_touch_directory(cfdir); }); - }).then([cfdirs0 = cfdirs[0]] { - return io_check([cfdirs0] { return touch_directory(cfdirs0 + "/upload"); }); - }).then([cfdirs0 = cfdirs[0]] { - return io_check([cfdirs0] { return touch_directory(cfdirs0 + "/staging"); }); }); + co_await io_check([cfdirs0 = cfdirs[0]] { return touch_directory(cfdirs0 + "/upload"); }); + co_await io_check([cfdirs0 = cfdirs[0]] { return touch_directory(cfdirs0 + "/staging"); }); } column_family& database::find_column_family(const schema_ptr& schema) { From 7ae49f513e1d93a9564ba931e871278bac149b4a Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Fri, 26 May 2023 17:33:01 +0300 Subject: [PATCH 5/8] table: Simplify init_storage() There's no need in copying the datadirs vector to call parallel_for_each upon. The datadirs[0] is in fact datadir field. Signed-off-by: Pavel Emelyanov --- replica/database.cc | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/replica/database.cc b/replica/database.cc index ba352e0c15..81e56cd773 100644 --- a/replica/database.cc +++ b/replica/database.cc @@ -1318,15 +1318,11 @@ keyspace::column_family_directory(const sstring& base_path, const sstring& name, } future<> table::init_storage() { - std::vector cfdirs; - for (auto& extra : _config.all_datadirs) { - cfdirs.push_back(extra); - } - co_await coroutine::parallel_for_each(cfdirs, [] (sstring cfdir) { + co_await coroutine::parallel_for_each(_config.all_datadirs, [] (sstring cfdir) { return io_check([cfdir] { return recursive_touch_directory(cfdir); }); }); - co_await io_check([cfdirs0 = cfdirs[0]] { return touch_directory(cfdirs0 + "/upload"); }); - co_await io_check([cfdirs0 = cfdirs[0]] { return touch_directory(cfdirs0 + "/staging"); }); + co_await io_check([this] { return touch_directory(_config.datadir + "/upload"); }); + co_await io_check([this] { return touch_directory(_config.datadir + "/staging"); }); } column_family& database::find_column_family(const schema_ptr& schema) { From 0e50fc609cfec84ccaa8d6084f87211e2cc17866 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Fri, 26 May 2023 17:52:39 +0300 Subject: [PATCH 6/8] table: Introduce destroy_storage() When table is DROP-ed the directory with all its sstables is removed (unless it contains snapshots). Wrap this into table.destroy_storage() method, later it will need to become sstable::storage-specific Signed-off-by: Pavel Emelyanov --- replica/database.cc | 7 +++++-- replica/database.hh | 1 + 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/replica/database.cc b/replica/database.cc index 81e56cd773..cf70002761 100644 --- a/replica/database.cc +++ b/replica/database.cc @@ -1082,7 +1082,6 @@ future<> database::drop_table_on_all_shards(sharded& sharded_db, sstri auto uuid = sharded_db.local().find_uuid(ks_name, cf_name); auto table_shards = co_await get_table_on_all_shards(sharded_db, uuid); - auto table_dir = fs::path(table_shards->dir()); std::optional snapshot_name_opt; if (with_snapshot) { snapshot_name_opt = format("pre-drop-{}", db_clock::now().time_since_epoch().count()); @@ -1099,7 +1098,7 @@ future<> database::drop_table_on_all_shards(sharded& sharded_db, sstri return table_shards->stop(); }); f.get(); // re-throw exception from truncate() if any - co_await sstables::remove_table_directory_if_has_no_snapshots(table_dir); + co_await table_shards->destroy_storage(); } const table_id& database::find_uuid(std::string_view ks, std::string_view cf) const { @@ -1325,6 +1324,10 @@ future<> table::init_storage() { co_await io_check([this] { return touch_directory(_config.datadir + "/staging"); }); } +future<> table::destroy_storage() { + return sstables::remove_table_directory_if_has_no_snapshots(fs::path(_config.datadir)); +} + column_family& database::find_column_family(const schema_ptr& schema) { return find_column_family(schema->id()); } diff --git a/replica/database.hh b/replica/database.hh index c16dd1eb34..8675d6e75a 100644 --- a/replica/database.hh +++ b/replica/database.hh @@ -627,6 +627,7 @@ public: const storage_options& get_storage_options() const noexcept { return *_storage_opts; } lw_shared_ptr get_storage_options_ptr() const noexcept { return _storage_opts; } future<> init_storage(); + future<> destroy_storage(); seastar::gate& async_gate() { return _async_gate; } From 93d8240bfbe9440547207c1647020c2a31d1071e Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Fri, 26 May 2023 17:22:58 +0300 Subject: [PATCH 7/8] keyspace: Remove column_family_directory() It's no longer used outside of make_column_family_config(). Not to encourage people to use it -- drop it and open-code into that single caller Signed-off-by: Pavel Emelyanov --- replica/database.cc | 11 +++-------- replica/database.hh | 2 -- 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/replica/database.cc b/replica/database.cc index cf70002761..7367a56128 100644 --- a/replica/database.cc +++ b/replica/database.cc @@ -1277,7 +1277,9 @@ keyspace::make_column_family_config(const schema& s, const database& db) const { const db::config& db_config = db.get_config(); for (auto& extra : _config.all_datadirs) { - cfg.all_datadirs.push_back(column_family_directory(extra, s.cf_name(), s.id())); + auto uuid_sstring = s.id().to_sstring(); + boost::erase_all(uuid_sstring, "-"); + cfg.all_datadirs.push_back(format("{}/{}-{}", extra, s.cf_name(), uuid_sstring)); } cfg.datadir = cfg.all_datadirs[0]; cfg.enable_disk_reads = _config.enable_disk_reads; @@ -1309,13 +1311,6 @@ keyspace::make_column_family_config(const schema& s, const database& db) const { return cfg; } -sstring -keyspace::column_family_directory(const sstring& base_path, const sstring& name, table_id uuid) const { - auto uuid_sstring = uuid.to_sstring(); - boost::erase_all(uuid_sstring, "-"); - return format("{}/{}-{}", base_path, name, uuid_sstring); -} - future<> table::init_storage() { co_await coroutine::parallel_for_each(_config.all_datadirs, [] (sstring cfdir) { return io_check([cfdir] { return recursive_touch_directory(cfdir); }); diff --git a/replica/database.hh b/replica/database.hh index 8675d6e75a..6778682f96 100644 --- a/replica/database.hh +++ b/replica/database.hh @@ -1242,8 +1242,6 @@ public: const sstring& datadir() const { return _config.datadir; } - - sstring column_family_directory(const sstring& base_path, const sstring& name, table_id uuid) const; }; using no_such_keyspace = data_dictionary::no_such_keyspace; From 29d80d1fe91c5a62721ca71752f866350fed7cc8 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Fri, 26 May 2023 17:27:01 +0300 Subject: [PATCH 8/8] keyspace: Introduce init_storage() Similarly to class table, the keyspace class also needs to create directory for itself for some reason. It looks excessive as table creation would call recursive_touch_directory() and would create the ks directory too, but this call is there Signed-off-by: Pavel Emelyanov --- replica/database.cc | 12 +++++++----- replica/database.hh | 2 ++ 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/replica/database.cc b/replica/database.cc index 7367a56128..b104a1bf6c 100644 --- a/replica/database.cc +++ b/replica/database.cc @@ -1323,6 +1323,12 @@ future<> table::destroy_storage() { return sstables::remove_table_directory_if_has_no_snapshots(fs::path(_config.datadir)); } +future<> keyspace::init_storage() { + if (_config.datadir != "") { + co_await io_check([this] { return touch_directory(_config.datadir); }); + } +} + column_family& database::find_column_family(const schema_ptr& schema) { return find_column_family(schema->id()); } @@ -1394,11 +1400,7 @@ database::create_keyspace(const lw_shared_ptr& ksm, locator:: co_await create_in_memory_keyspace(ksm, erm_factory, system); auto& ks = _keyspaces.at(ksm->name()); - auto& datadir = ks.datadir(); - - if (datadir != "") { - co_await io_check([&datadir] { return touch_directory(datadir); }); - } + co_await ks.init_storage(); } future<> diff --git a/replica/database.hh b/replica/database.hh index 6778682f96..cda4f96119 100644 --- a/replica/database.hh +++ b/replica/database.hh @@ -1204,6 +1204,8 @@ public: future<> update_from(const locator::shared_token_metadata& stm, lw_shared_ptr); + future<> init_storage(); + /** Note: return by shared pointer value, since the meta data is * semi-volatile. I.e. we could do alter keyspace at any time, and * boom, it is replaced.