From 8a061bd862dfabd0c70dfd15bb3f4aa1a28a4102 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Wed, 1 Feb 2023 16:24:39 +0300 Subject: [PATCH] sstables, code: Introduce and use change_state() call The call moves the sstable to the specified state. The change state is translated into the storage driver state change which is for todays filesystem storage means moving between directories. The "normal" state maps to the base dir of the table, there's no dedicated subdir for this state and this brings some trouble into the play. The thing is that in order to check if an sstable is in "normal" state already its impossible to compare filename of its path to any pre-defined values, as tables' basdirs are dynamic. To overcome this, the change-state call checks that the sstable is in one of "known" sub-states, and assumes that it's in normal state otherwise. Signed-off-by: Pavel Emelyanov --- compaction/compaction.cc | 2 +- replica/table.cc | 2 +- sstables/sstables.cc | 33 +++++++++++++++++++++++++++++++++ sstables/sstables.hh | 14 ++++++++++++++ test/boost/database_test.cc | 4 ++-- 5 files changed, 51 insertions(+), 4 deletions(-) diff --git a/compaction/compaction.cc b/compaction/compaction.cc index 4ad38b2e03..bac75f9441 100644 --- a/compaction/compaction.cc +++ b/compaction/compaction.cc @@ -1799,7 +1799,7 @@ static future scrub_sstables_validate_mode(sstables::compacti if (validation_errors != 0) { for (auto& sst : *sstables->all()) { - co_await sst->move_to_quarantine(); + co_await sst->change_state(sstables::quarantine_dir); } } diff --git a/replica/table.cc b/replica/table.cc index d111d739c7..f127cd796c 100644 --- a/replica/table.cc +++ b/replica/table.cc @@ -2543,7 +2543,7 @@ future<> table::move_sstables_from_staging(std::vector // completed first. // The _sstable_deletion_sem prevents list update on off-strategy completion and move_sstables_from_staging() // from stepping on each other's toe. - co_await sst->move_to_new_dir(dir(), sst->generation(), &delay_commit); + co_await sst->change_state(sstables::normal_dir, &delay_commit); // If view building finished faster, SSTable with repair origin still exists. // It can also happen the SSTable is not going through reshape, so it doesn't have a repair origin. // That being said, we'll only add this SSTable to tracker if its origin is other than repair. diff --git a/sstables/sstables.cc b/sstables/sstables.cc index cb61eadb13..f5bca4650d 100644 --- a/sstables/sstables.cc +++ b/sstables/sstables.cc @@ -2241,6 +2241,35 @@ future<> sstable::filesystem_storage::quarantine(const sstable& sst, delayed_com co_await move(sst, std::move(new_dir), sst.generation(), delay_commit); } +future<> sstable::filesystem_storage::change_state(const sstable& sst, sstring to, generation_type new_generation, delayed_commit_changes* delay_commit) { + auto path = fs::path(dir); + auto current = path.filename().native(); + + // Moving between states means moving between basedir/state subdirectories. + // However, normal state maps to the basedir itself and thus there's no way + // to check if current is normal_dir. The best that can be done here is to + // check that it's not anything else + if (current == staging_dir || current == upload_dir || current == quarantine_dir) { + if (to == quarantine_dir && current != staging_dir) { + // Legacy exception -- quarantine from anything but staging + // moves to the current directory quarantine subdir + path = path / to; + } else { + path = path.parent_path() / to; + } + } else { + current = normal_dir; + path = path / to; + } + + if (current == to) { + co_return; // Already there + } + + sstlog.info("Moving sstable {} to {} in {}", sst.get_filename(), to, path); + co_await move(sst, path.native(), std::move(new_generation), delay_commit); +} + future<> sstable::move_to_quarantine(delayed_commit_changes* delay_commit) { if (is_quarantined()) { return make_ready_future<>(); @@ -2249,6 +2278,10 @@ future<> sstable::move_to_quarantine(delayed_commit_changes* delay_commit) { return _storage.quarantine(*this, delay_commit); } +future<> sstable::change_state(sstring to, delayed_commit_changes* delay_commit) { + co_await _storage.change_state(*this, to, _generation, delay_commit); +} + future<> sstable::delayed_commit_changes::commit() { return parallel_for_each(_dirs, [] (sstring dir) { return sync_directory(dir); diff --git a/sstables/sstables.hh b/sstables/sstables.hh index 50e76fbed2..ac0593c821 100644 --- a/sstables/sstables.hh +++ b/sstables/sstables.hh @@ -113,6 +113,7 @@ private: friend class sstables_manager; }; +constexpr const char* normal_dir = ""; constexpr const char* staging_dir = "staging"; constexpr const char* upload_dir = "upload"; constexpr const char* snapshots_dir = "snapshots"; @@ -220,6 +221,12 @@ public: // will move it into a quarantine_dir subdirectory of its current directory. future<> move_to_quarantine(delayed_commit_changes* delay = nullptr); + // Move the sstable between states + // + // Known states are normal, staging, upload and quarantine. + // It's up to the storage driver how to implement this. + future<> change_state(sstring to, delayed_commit_changes* delay = nullptr); + generation_type generation() const { return _generation; } @@ -491,6 +498,13 @@ public: future<> seal(const sstable& sst); future<> snapshot(const sstable& sst, sstring dir, absolute_path abs) const; future<> quarantine(const sstable& sst, delayed_commit_changes* delay); + + // Moves the files around with .move() method. States are basedir subdirectories + // with the exception that normal state maps to the basedir itself. If the sstable + // is already in the target state, this is a noop. + // Moving in a snapshot or upload will move to a subdirectory of the current directory. + future<> change_state(const sstable& sst, sstring to, generation_type generation, delayed_commit_changes* delay); + future<> move(const sstable& sst, sstring new_dir, generation_type generation, delayed_commit_changes* delay); // runs in async context void open(sstable& sst, const io_priority_class& pc); diff --git a/test/boost/database_test.cc b/test/boost/database_test.cc index a8b9e82190..1f7fc69b09 100644 --- a/test/boost/database_test.cc +++ b/test/boost/database_test.cc @@ -1137,7 +1137,7 @@ SEASTAR_TEST_CASE(populate_from_quarantine_works) { auto idx = tests::random::get_int(0, sstables.size() - 1); testlog.debug("Moving sstable #{} out of {} to quarantine", idx, sstables.size()); auto sst = sstables[idx]; - co_await sst->move_to_quarantine(); + co_await sst->change_state(sstables::quarantine_dir); found |= true; }); co_return found; @@ -1191,7 +1191,7 @@ SEASTAR_TEST_CASE(snapshot_with_quarantine_works) { } auto idx = tests::random::get_int(0, sstables.size() - 1); auto sst = sstables[idx]; - co_await sst->move_to_quarantine(); + co_await sst->change_state(sstables::quarantine_dir); }); }); }