From 93df88aac48909720eb02cc545bfde586db4855a Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Fri, 22 Apr 2022 13:00:25 +0300 Subject: [PATCH 1/3] format-selector: Coroutinize simple methods These all are just straightfowrard usage of co_await's around the code. Signed-off-by: Pavel Emelyanov --- db/sstables-format-selector.cc | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/db/sstables-format-selector.cc b/db/sstables-format-selector.cc index f77313cf13..1260a8345f 100644 --- a/db/sstables-format-selector.cc +++ b/db/sstables-format-selector.cc @@ -7,6 +7,7 @@ * SPDX-License-Identifier: AGPL-3.0-or-later */ +#include #include "sstables-format-selector.hh" #include "log.hh" #include "replica/database.hh" @@ -61,31 +62,27 @@ future<> sstables_format_selector::do_maybe_select_format(sstables::sstable_vers future<> sstables_format_selector::start() { assert(this_shard_id() == 0); - return read_sstables_format().then([this] { - _features.local().me_sstable.when_enabled(_me_feature_listener); - _features.local().md_sstable.when_enabled(_md_feature_listener); - return make_ready_future<>(); - }); + co_await read_sstables_format(); + _features.local().me_sstable.when_enabled(_me_feature_listener); + _features.local().md_sstable.when_enabled(_md_feature_listener); } future<> sstables_format_selector::stop() { - return _sel.close(); + co_await _sel.close(); } future<> sstables_format_selector::read_sstables_format() { - return db::system_keyspace::get_scylla_local_param(SSTABLE_FORMAT_PARAM_NAME).then([this] (std::optional format_opt) { - if (format_opt) { - sstables::sstable_version_types format = sstables::from_string(*format_opt); - return select_format(format); - } - return make_ready_future<>(); - }); + std::optional format_opt = co_await db::system_keyspace::get_scylla_local_param(SSTABLE_FORMAT_PARAM_NAME); + if (format_opt) { + sstables::sstable_version_types format = sstables::from_string(*format_opt); + co_await select_format(format); + } } future<> sstables_format_selector::select_format(sstables::sstable_version_types format) { logger.info("Selected {} sstables format", to_string(format)); _selected_format = format; - return _db.invoke_on_all([this] (replica::database& db) { + co_await _db.invoke_on_all([this] (replica::database& db) { db.set_format(_selected_format); }); } From 7fee50f1e383437b64a6a7e3ec401e8e5e0ca5ed Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Fri, 22 Apr 2022 13:08:08 +0300 Subject: [PATCH 2/3] format-selector: Coroutinize maybe_select_format() This method is run when a feature is enabled. It's a bit trickier than the others, also there are two methods actually, that are merged into one by this patch. By and large most of the care is about the _sel gate and _sem semaphore. The gate protects the whole selection code from the selector being freed from underneath it on stop. The semaphore is only needed to keep two different format selections from each other -- each update the system keyspace, local variable and replica::database instance on all shards. In the end there's a gossiper update, but it happens outside of the semaphore. Signed-off-by: Pavel Emelyanov --- db/sstables-format-selector.cc | 26 ++++++++------------------ db/sstables-format-selector.hh | 2 -- 2 files changed, 8 insertions(+), 20 deletions(-) diff --git a/db/sstables-format-selector.cc b/db/sstables-format-selector.cc index 1260a8345f..3aa2391e59 100644 --- a/db/sstables-format-selector.cc +++ b/db/sstables-format-selector.cc @@ -38,26 +38,16 @@ sstables_format_selector::sstables_format_selector(gms::gossiper& g, sharded sstables_format_selector::maybe_select_format(sstables::sstable_version_types new_format) { - return with_gate(_sel, [this, new_format] { - return do_maybe_select_format(new_format); - }); -} + auto hg = _sel.hold(); + auto units = co_await get_units(_sem, 1); -future<> sstables_format_selector::do_maybe_select_format(sstables::sstable_version_types new_format) { - return with_semaphore(_sem, 1, [this, new_format] { - if (new_format <= _selected_format) { - return make_ready_future(false); - } - return db::system_keyspace::set_scylla_local_param(SSTABLE_FORMAT_PARAM_NAME, to_string(new_format)).then([this, new_format] { - return select_format(new_format); - }).then([] { return true; }); - }).then([this] (bool update_features) { - if (!update_features) { - return make_ready_future<>(); - } - return _gossiper.add_local_application_state(gms::application_state::SUPPORTED_FEATURES, + if (new_format > _selected_format) { + co_await db::system_keyspace::set_scylla_local_param(SSTABLE_FORMAT_PARAM_NAME, to_string(new_format)); + co_await select_format(new_format); + units.return_all(); + co_await _gossiper.add_local_application_state(gms::application_state::SUPPORTED_FEATURES, gms::versioned_value::supported_features(_features.local().supported_feature_set())); - }); + } } future<> sstables_format_selector::start() { diff --git a/db/sstables-format-selector.hh b/db/sstables-format-selector.hh index 3ea4a0c433..defb77b27a 100644 --- a/db/sstables-format-selector.hh +++ b/db/sstables-format-selector.hh @@ -57,8 +57,6 @@ class sstables_format_selector { future<> select_format(sstables::sstable_version_types new_format); future<> read_sstables_format(); - future<> do_maybe_select_format(sstables::sstable_version_types new_format); - public: sstables_format_selector(gms::gossiper& g, sharded& f, sharded& db); From f81f1c7ef78f81f2fe8d43024980ff1457ab3ebf Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Fri, 22 Apr 2022 13:29:43 +0300 Subject: [PATCH 3/3] format-selector: Remove .sync() point The feature listener callbacks are waited upon to finish in the middle of the cluster joining process. I particular -- before actually joining the cluster the format should have being selected. For that there's a .sync() method that locks the semaphore thus making sure that any update is finished and it's called right after the wait_for_gossip_to_settle() finishes. However, features are enabled inside the wait_for_gossip_to_settle() in a seastar::async() context that's also waited upon to finish. This waiting makes it possible for any feature listener to .get() any of its futures that should be resolved until gossip is settled. Said that, the format selection barrier can be moved -- instead of waiting on the semaphore, the respective part of the selection code can be .get()-ed (it all runs in async context). One thing to care about -- the remainder should continue running with the gate held. Signed-off-by: Pavel Emelyanov --- db/sstables-format-selector.cc | 9 ++++----- db/sstables-format-selector.hh | 4 ---- main.cc | 1 - 3 files changed, 4 insertions(+), 10 deletions(-) diff --git a/db/sstables-format-selector.cc b/db/sstables-format-selector.cc index 3aa2391e59..2822f9c6d1 100644 --- a/db/sstables-format-selector.cc +++ b/db/sstables-format-selector.cc @@ -24,8 +24,7 @@ static const sstring SSTABLE_FORMAT_PARAM_NAME = "sstable_format"; void feature_enabled_listener::on_enabled() { if (!_started) { _started = true; - // FIXME -- discarded future - (void)_selector.maybe_select_format(_format); + _selector.maybe_select_format(_format).get(); } } @@ -44,9 +43,9 @@ future<> sstables_format_selector::maybe_select_format(sstables::sstable_version if (new_format > _selected_format) { co_await db::system_keyspace::set_scylla_local_param(SSTABLE_FORMAT_PARAM_NAME, to_string(new_format)); co_await select_format(new_format); - units.return_all(); - co_await _gossiper.add_local_application_state(gms::application_state::SUPPORTED_FEATURES, - gms::versioned_value::supported_features(_features.local().supported_feature_set())); + // FIXME discarded future + (void)_gossiper.add_local_application_state(gms::application_state::SUPPORTED_FEATURES, + gms::versioned_value::supported_features(_features.local().supported_feature_set())).finally([h = std::move(hg)] {}); } } diff --git a/db/sstables-format-selector.hh b/db/sstables-format-selector.hh index defb77b27a..9a59a9889f 100644 --- a/db/sstables-format-selector.hh +++ b/db/sstables-format-selector.hh @@ -64,10 +64,6 @@ public: future<> stop(); future<> maybe_select_format(sstables::sstable_version_types new_format); - - void sync() { - get_units(_sem, 1).get0(); - } }; } // namespace sstables diff --git a/main.cc b/main.cc index ac6d6e8347..e52aa30418 100644 --- a/main.cc +++ b/main.cc @@ -1289,7 +1289,6 @@ To start the scylla server proper, simply invoke as: scylla server (or just scyl mm.local().passive_announce(std::move(schema_version)); }); gossiper.local().wait_for_gossip_to_settle().get(); - sst_format_selector.sync(); with_scheduling_group(maintenance_scheduling_group, [&] { return ss.local().join_cluster();