From bcfb2e509bf02b6affea56f714242bf7727c5329 Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Tue, 22 Jan 2019 18:00:52 +0200 Subject: [PATCH 1/8] distributed_loader: push future returned by rmdir into futures vector --- distributed_loader.cc | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/distributed_loader.cc b/distributed_loader.cc index b6fe385b0f..f2aa9a79d9 100644 --- a/distributed_loader.cc +++ b/distributed_loader.cc @@ -536,8 +536,12 @@ future<> distributed_loader::populate_column_family(distributed& db, s return lister::scan_dir(sstdir, { directory_entry_type::regular }, [&db, verifier, &futures] (fs::path sstdir, directory_entry de) { // FIXME: The secondary indexes are in this level, but with a directory type, (starting with ".") + // push future returned by probe_file/rmdir into an array of futures, + // so that the supplied callback will not block scan_dir() from + // reading the next entry in the directory. if (de.type && *de.type == directory_entry_type::directory && sstables::sstable::is_temp_dir(de.name)) { - return lister::rmdir(sstdir / de.name); + futures.push_back(lister::rmdir(sstdir / de.name)); + return make_ready_future<>(); } auto f = distributed_loader::probe_file(db, sstdir.native(), de.name).then([verifier, sstdir, de] (auto entry) { @@ -571,9 +575,6 @@ future<> distributed_loader::populate_column_family(distributed& db, s return make_ready_future<>(); }); - // push future returned by probe_file into an array of futures, - // so that the supplied callback will not block scan_dir() from - // reading the next entry in the directory. futures.push_back(std::move(f)); return make_ready_future<>(); From 9bd7b2f4e62693b50e94c4e4fbbf5d70939c0d7b Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Tue, 22 Jan 2019 18:51:15 +0200 Subject: [PATCH 2/8] distributed_loader: remove temporary sstable directories only on shard 0 Similar to calling remove_sstable_with_temp_toc later on in populate_column_family(), we need only one thread to do the cleanup work and the existing convention is that it's shard 0. Since lister::rmdir is checking remove_file of all entries (recursively) and the dir itself, doing that concurrently would fail. Signed-off-by: Benny Halevy --- distributed_loader.cc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/distributed_loader.cc b/distributed_loader.cc index f2aa9a79d9..3cb0049a84 100644 --- a/distributed_loader.cc +++ b/distributed_loader.cc @@ -540,7 +540,11 @@ future<> distributed_loader::populate_column_family(distributed& db, s // so that the supplied callback will not block scan_dir() from // reading the next entry in the directory. if (de.type && *de.type == directory_entry_type::directory && sstables::sstable::is_temp_dir(de.name)) { - futures.push_back(lister::rmdir(sstdir / de.name)); + if (engine().cpu_id() == 0) { + fs::path dirpath = sstdir / de.name; + dblog.info("Found temporary sstable directory: {}, removing", dirpath); + futures.push_back(lister::rmdir(dirpath)); + } return make_ready_future<>(); } From c2a5f3b8424da2786e4078e618594027222f8611 Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Wed, 23 Jan 2019 10:18:31 +0200 Subject: [PATCH 3/8] distributed_loader: populate_column_family: ignore directories other than sstable::is_temp_dir populate_column_family currently lists only regular files. ignoring all directories. A later patch in this series allows it to list also directories so to cleanup the temporary sstable directories, yet valid sub-directories, like staging|upload|snapshots, may still exist and need to be ignored. Other kinds of handling, like validating recgnized sub-directories and halting on unrecognized sub-directories are possible, yet out of scope for this patch(set). Signed-off-by: Benny Halevy --- distributed_loader.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/distributed_loader.cc b/distributed_loader.cc index 3cb0049a84..1b50db20fd 100644 --- a/distributed_loader.cc +++ b/distributed_loader.cc @@ -539,8 +539,8 @@ future<> distributed_loader::populate_column_family(distributed& db, s // push future returned by probe_file/rmdir into an array of futures, // so that the supplied callback will not block scan_dir() from // reading the next entry in the directory. - if (de.type && *de.type == directory_entry_type::directory && sstables::sstable::is_temp_dir(de.name)) { - if (engine().cpu_id() == 0) { + if (de.type && *de.type == directory_entry_type::directory) { + if (engine().cpu_id() == 0 && sstables::sstable::is_temp_dir(de.name)) { fs::path dirpath = sstdir / de.name; dblog.info("Found temporary sstable directory: {}, removing", dirpath); futures.push_back(lister::rmdir(dirpath)); From bd859752773b384894942de2fa985514afa21a7d Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Wed, 23 Jan 2019 10:12:41 +0200 Subject: [PATCH 4/8] sstables: fix is_temp_dir 1. fs::canonical required that the path will exist. and there is no need for fs::canonical here. 2. fs::path::extension will return the leading dot. Signed-off-by: Benny Halevy --- distributed_loader.cc | 4 ++-- sstables/sstables.hh | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/distributed_loader.cc b/distributed_loader.cc index 1b50db20fd..99656600eb 100644 --- a/distributed_loader.cc +++ b/distributed_loader.cc @@ -540,8 +540,8 @@ future<> distributed_loader::populate_column_family(distributed& db, s // so that the supplied callback will not block scan_dir() from // reading the next entry in the directory. if (de.type && *de.type == directory_entry_type::directory) { - if (engine().cpu_id() == 0 && sstables::sstable::is_temp_dir(de.name)) { - fs::path dirpath = sstdir / de.name; + fs::path dirpath = sstdir / de.name; + if (engine().cpu_id() == 0 && sstables::sstable::is_temp_dir(dirpath)) { dblog.info("Found temporary sstable directory: {}, removing", dirpath); futures.push_back(lister::rmdir(dirpath)); } diff --git a/sstables/sstables.hh b/sstables/sstables.hh index 9fc89e4b7c..6731bdb690 100644 --- a/sstables/sstables.hh +++ b/sstables/sstables.hh @@ -382,9 +382,9 @@ public: return dir + "/" + sst_dir_basename(gen); } - static bool is_temp_dir(const sstring& dirpath) + static bool is_temp_dir(const fs::path& dirpath) { - return fs::canonical(fs::path(dirpath)).extension().string() == "sstable"; + return dirpath.extension().string() == ".sstable"; } const sstring& get_dir() const { From 74ef09a3a20bca7769196a0f8d85cca7e0b06a87 Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Wed, 23 Jan 2019 10:16:14 +0200 Subject: [PATCH 5/8] distributed_loader: populate_column_family should scan directories too To detect and cleanup leftover temporary sstable directories. Signed-off-by: Benny Halevy --- distributed_loader.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/distributed_loader.cc b/distributed_loader.cc index 99656600eb..3ec418ef2f 100644 --- a/distributed_loader.cc +++ b/distributed_loader.cc @@ -533,7 +533,7 @@ future<> distributed_loader::populate_column_family(distributed& db, s auto verifier = make_lw_shared>(); return do_with(std::vector>(), [&db, sstdir = std::move(sstdir), verifier, ks, cf] (std::vector>& futures) { - return lister::scan_dir(sstdir, { directory_entry_type::regular }, [&db, verifier, &futures] (fs::path sstdir, directory_entry de) { + return lister::scan_dir(sstdir, { directory_entry_type::regular, directory_entry_type::directory }, [&db, verifier, &futures] (fs::path sstdir, directory_entry de) { // FIXME: The secondary indexes are in this level, but with a directory type, (starting with ".") // push future returned by probe_file/rmdir into an array of futures, From 441809094a83baa7959ec66ad90351c2d1a2cce6 Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Thu, 24 Jan 2019 19:32:04 +0200 Subject: [PATCH 6/8] tests: single_node_cql_env::_data_dir is not used Signed-off-by: Benny Halevy --- tests/cql_test_env.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/cql_test_env.cc b/tests/cql_test_env.cc index bbceefef2f..950968e9a1 100644 --- a/tests/cql_test_env.cc +++ b/tests/cql_test_env.cc @@ -99,7 +99,6 @@ private: ::shared_ptr> _auth_service; ::shared_ptr> _view_builder; ::shared_ptr> _view_update_generator; - lw_shared_ptr _data_dir; private: struct core_local_state { service::client_state client_state; From 64a23ea3bc682741f99008ed35bd8016df799ef0 Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Thu, 24 Jan 2019 18:24:05 +0200 Subject: [PATCH 7/8] tests: single_node_cql_env::do_with: use the provided data_file_directories path if available Signed-off-by: Benny Halevy --- tests/cql_test_env.cc | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/tests/cql_test_env.cc b/tests/cql_test_env.cc index 950968e9a1..d581ea5e7f 100644 --- a/tests/cql_test_env.cc +++ b/tests/cql_test_env.cc @@ -318,17 +318,20 @@ public: auto db = ::make_shared>(); auto cfg = make_lw_shared(std::move(cfg_in)); tmpdir data_dir; + auto& data_dir_path = data_dir.path; if (!cfg->data_file_directories.is_set()) { - cfg->data_file_directories() = {data_dir.path}; + cfg->data_file_directories() = {data_dir_path}; + } else { + data_dir_path = cfg->data_file_directories()[0]; } - cfg->commitlog_directory() = data_dir.path + "/commitlog.dir"; - cfg->hints_directory() = data_dir.path + "/hints.dir"; - cfg->view_hints_directory() = data_dir.path + "/view_hints.dir"; + cfg->commitlog_directory() = data_dir_path + "/commitlog.dir"; + cfg->hints_directory() = data_dir_path + "/hints.dir"; + cfg->view_hints_directory() = data_dir_path + "/view_hints.dir"; cfg->num_tokens() = 256; cfg->ring_delay_ms() = 500; cfg->experimental() = true; cfg->shutdown_announce_in_ms() = 0; - boost::filesystem::create_directories((data_dir.path + "/system").c_str()); + boost::filesystem::create_directories((data_dir_path + "/system").c_str()); boost::filesystem::create_directories(cfg->commitlog_directory().c_str()); boost::filesystem::create_directories(cfg->hints_directory().c_str()); boost::filesystem::create_directories(cfg->view_hints_directory().c_str()); From 36b6a3ebcfafaa9607b19b439a04e7c70816f593 Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Thu, 24 Jan 2019 18:55:50 +0200 Subject: [PATCH 8/8] tests: add test_distributed_loader_with_incomplete_sstables Test removal of sstables with temporary TOC file, with and without temporary sstable directory. Temporary sstable directories may be empty or still have leftover components in them. Signed-off-by: Benny Halevy --- tests/database_test.cc | 57 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/tests/database_test.cc b/tests/database_test.cc index c4f9e17279..6fbcaab78c 100644 --- a/tests/database_test.cc +++ b/tests/database_test.cc @@ -20,6 +20,7 @@ */ +#include #include #include @@ -32,6 +33,9 @@ #include "mutation_source_test.hh" #include "schema_registry.hh" #include "service/migration_manager.hh" +#include "sstables/sstables.hh" +#include "db/config.hh" +#include "tmpdir.hh" SEASTAR_TEST_CASE(test_querying_with_limits) { return do_with_cql_env([](cql_test_env& e) { @@ -107,3 +111,56 @@ SEASTAR_THREAD_TEST_CASE(test_database_with_data_in_sstables_is_a_mutation_sourc return make_ready_future<>(); }).get(); } + +SEASTAR_THREAD_TEST_CASE(test_distributed_loader_with_incomplete_sstables) { + using sst = sstables::sstable; + + tmpdir data_dir; + db::config db_cfg; + + db_cfg.data_file_directories({data_dir.path}, db::config::config_source::CommandLine); + + // Create incomplete sstables in test data directory + sstring ks = "system"; + sstring cf = "local-7ad54392bcdd35a684174e047860b377"; + sstring sst_dir = data_dir.path + "/" + ks + "/" + cf; + + auto require_exist = [] (const sstring& name, bool should_exist) { + auto exists = file_exists(name).get0(); + BOOST_REQUIRE(exists == should_exist); + }; + + auto touch_dir = [&require_exist] (const sstring& dir_name) { + recursive_touch_directory(dir_name).get(); + require_exist(dir_name, true); + }; + + auto touch_file = [&require_exist] (const sstring& file_name) { + auto f = open_file_dma(file_name, open_flags::create).get0(); + f.close().get(); + require_exist(file_name, true); + }; + + auto temp_sst_dir = sst::temp_sst_dir(sst_dir, 2); + touch_dir(temp_sst_dir); + + temp_sst_dir = sst::temp_sst_dir(sst_dir, 3); + touch_dir(temp_sst_dir); + auto temp_file_name = sst::filename(temp_sst_dir, ks, cf, sst::version_types::mc, 3, sst::format_types::big, component_type::TemporaryTOC); + touch_file(temp_file_name); + + temp_file_name = sst::filename(sst_dir, ks, cf, sst::version_types::mc, 4, sst::format_types::big, component_type::TemporaryTOC); + touch_file(temp_file_name); + temp_file_name = sst::filename(sst_dir, ks, cf, sst::version_types::mc, 4, sst::format_types::big, component_type::Data); + touch_file(temp_file_name); + + do_with_cql_env([&sst_dir, &ks, &cf, &require_exist] (cql_test_env& e) { + require_exist(sst::temp_sst_dir(sst_dir, 2), false); + require_exist(sst::temp_sst_dir(sst_dir, 3), false); + + require_exist(sst::filename(sst_dir, ks, cf, sst::version_types::mc, 4, sst::format_types::big, component_type::TemporaryTOC), false); + require_exist(sst::filename(sst_dir, ks, cf, sst::version_types::mc, 4, sst::format_types::big, component_type::Data), false); + + return make_ready_future<>(); + }, db_cfg).get(); +}