From bd1d65ec3898a75042ecf3357d78e3d2542d42cb Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Tue, 16 Jan 2024 18:15:59 +0200 Subject: [PATCH 1/5] compaction_manager: perform_cleanup: use compaction_manager::eligible_for_compaction 3b424e391b6ccebd19cd392bef999e2aa52c0242 introduced a loop in `perform_cleanup` that waits until all sstables that require cleanup are cleaned up. However, with f1bbf705f9b6faa406baac04f418a93b09a53c19, an sstable that is_eligible_for_compaction (i.e. it is not in staging, awaiting view update generation), may already be compacted by e.g. regular compaction. And so perform_cleanup should interrupt that by calling try_perform_cleanup, since the latter reevaluates `update_sstable_cleanup_state` with compaction disabled - that stops ongoing compactions. Refs scylladb/scylladb#15673 Signed-off-by: Benny Halevy --- compaction/compaction_manager.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compaction/compaction_manager.cc b/compaction/compaction_manager.cc index 22d4cf060a..a055a9284c 100644 --- a/compaction/compaction_manager.cc +++ b/compaction/compaction_manager.cc @@ -1892,7 +1892,7 @@ future<> compaction_manager::perform_cleanup(owned_ranges_ptr sorted_owned_range auto has_sstables_eligible_for_compaction = [&] { for (auto& sst : cs.sstables_requiring_cleanup) { - if (sstables::is_eligible_for_compaction(sst)) { + if (eligible_for_compaction(sst)) { return true; } } From 51a46aa83bf57133b4e18d25885a215dd486d709 Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Tue, 16 Jan 2024 18:33:55 +0200 Subject: [PATCH 2/5] compaction_manager: perform_task_on_all_files: return early when there are no sstables to compact Prevent the creation of a compaction task when the list of sstables is known to be empty ahead of time. Refs scylladb/scylladb#16694 Fixes scylladb/scylladb#16803 Signed-off-by: Benny Halevy --- compaction/compaction_manager.cc | 3 +++ test/rest_api/task_manager_utils.py | 2 +- test/rest_api/test_compaction_task.py | 2 +- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/compaction/compaction_manager.cc b/compaction/compaction_manager.cc index a055a9284c..6b3e7920b7 100644 --- a/compaction/compaction_manager.cc +++ b/compaction/compaction_manager.cc @@ -1627,6 +1627,9 @@ future compaction_manager::perform_tas return a->data_size() > b->data_size(); }); }); + if (sstables.empty()) { + co_return std::nullopt; + } co_return co_await perform_compaction(throw_if_stopping::no, info, &t, info.value_or(tasks::task_info{}).id, std::move(options), std::move(owned_ranges_ptr), std::move(sstables), std::move(compacting), std::forward(args)...); } diff --git a/test/rest_api/task_manager_utils.py b/test/rest_api/task_manager_utils.py index 63f3c4d121..def0a4ef6e 100644 --- a/test/rest_api/task_manager_utils.py +++ b/test/rest_api/task_manager_utils.py @@ -56,7 +56,7 @@ def assert_task_does_not_exist(rest_api, task_id): assert resp.status_code == requests.codes.bad_request, f"Task {task_id} is kept in memory" def check_child_parent_relationship(rest_api, parent, tree_depth, allow_no_children, depth=0): - assert allow_no_children or parent.get("children_ids", []), "Child tasks were not created" + assert allow_no_children or parent.get("children_ids", []), f"Child tasks were not created for {parent}" for child_id in parent.get("children_ids", []): child = wait_for_task(rest_api, child_id) diff --git a/test/rest_api/test_compaction_task.py b/test/rest_api/test_compaction_task.py index 806cbda0a7..e853180063 100644 --- a/test/rest_api/test_compaction_task.py +++ b/test/rest_api/test_compaction_task.py @@ -91,7 +91,7 @@ def test_offstrategy_keyspace_compaction_task(cql, this_dc, rest_api): check_compaction_task(cql, this_dc, rest_api, lambda keyspace, _: rest_api.send("POST", f"storage_service/keyspace_offstrategy_compaction/{keyspace}"), "offstrategy compaction", task_tree_depth, True) def test_rewrite_sstables_keyspace_compaction_task(cql, this_dc, rest_api): - task_tree_depth = 3 + task_tree_depth = 2 # upgrade sstables compaction check_compaction_task(cql, this_dc, rest_api, lambda keyspace, _: rest_api.send("GET", f"storage_service/keyspace_upgrade_sstables/{keyspace}"), "upgrade sstables compaction", task_tree_depth) # scrub sstables compaction From 0d937f39746dc8da223bc5fcc6e2a4d418c8d1d6 Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Wed, 17 Jan 2024 12:01:51 +0200 Subject: [PATCH 3/5] table: add_sstable_and_update_cache: trigger compaction only in compaction group There is no need to trigger compaction in all compaction groups when an sstable is added to only one of them. And with that level of control, if the caller passes sstables::offstrategy::yes, we know it will trigger offstrategy compaction later on so there is no need to trigger compaction at all for this sstable at this time. Signed-off-by: Benny Halevy --- replica/database.hh | 2 +- replica/table.cc | 14 +++++++++----- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/replica/database.hh b/replica/database.hh index f1f3aa6247..126819f399 100644 --- a/replica/database.hh +++ b/replica/database.hh @@ -604,7 +604,7 @@ private: return _config.enable_cache && _schema->caching_options().enabled(); } void update_stats_for_new_sstable(const sstables::shared_sstable& sst) noexcept; - future<> do_add_sstable_and_update_cache(sstables::shared_sstable sst, sstables::offstrategy offstrategy); + future<> do_add_sstable_and_update_cache(sstables::shared_sstable sst, sstables::offstrategy offstrategy, bool trigger_compaction); // Helpers which add sstable on behalf of a compaction group and refreshes compound set. void add_sstable(compaction_group& cg, sstables::shared_sstable sstable); void add_maintenance_sstable(compaction_group& cg, sstables::shared_sstable sst); diff --git a/replica/table.cc b/replica/table.cc index d60590b7cf..289c0d38bd 100644 --- a/replica/table.cc +++ b/replica/table.cc @@ -814,9 +814,9 @@ void table::update_stats_for_new_sstable(const sstables::shared_sstable& sst) no } future<> -table::do_add_sstable_and_update_cache(sstables::shared_sstable sst, sstables::offstrategy offstrategy) { +table::do_add_sstable_and_update_cache(sstables::shared_sstable sst, sstables::offstrategy offstrategy, bool trigger_compaction) { auto permit = co_await seastar::get_units(_sstable_set_mutation_sem, 1); - co_return co_await get_row_cache().invalidate(row_cache::external_updater([this, sst, offstrategy] () noexcept { + co_return co_await get_row_cache().invalidate(row_cache::external_updater([&] () noexcept { // FIXME: this is not really noexcept, but we need to provide strong exception guarantees. // atomically load all opened sstables into column family. compaction_group& cg = compaction_group_for_sstable(sst); @@ -826,20 +826,24 @@ table::do_add_sstable_and_update_cache(sstables::shared_sstable sst, sstables::o add_maintenance_sstable(cg, sst); } update_stats_for_new_sstable(sst); + if (trigger_compaction) { + try_trigger_compaction(cg); + } }), dht::partition_range::make({sst->get_first_decorated_key(), true}, {sst->get_last_decorated_key(), true})); } future<> table::add_sstable_and_update_cache(sstables::shared_sstable sst, sstables::offstrategy offstrategy) { - co_await do_add_sstable_and_update_cache(std::move(sst), offstrategy); - trigger_compaction(); + bool do_trigger_compaction = offstrategy == sstables::offstrategy::no; + co_await do_add_sstable_and_update_cache(std::move(sst), offstrategy, do_trigger_compaction); } future<> table::add_sstables_and_update_cache(const std::vector& ssts) { + constexpr bool do_not_trigger_compaction = false; for (auto& sst : ssts) { try { - co_await do_add_sstable_and_update_cache(sst, sstables::offstrategy::no); + co_await do_add_sstable_and_update_cache(sst, sstables::offstrategy::no, do_not_trigger_compaction); } catch (...) { tlogger.error("Failed to load SSTable {}: {}", sst->toc_filename(), std::current_exception()); throw; From 8595d64d01890d3d9680ecea745a3f744bf42d34 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov Date: Thu, 18 Jan 2024 18:57:21 +0300 Subject: [PATCH 4/5] locator: Handle replication factor of 0 for initial_tablets calculations When calculating per-DC tablets the formula is shards_in_dc / rf_in_dc, but the denominator in it can be configured to be literally zero and the division doesn't work. Fix by assuming zero tablets for dcs with zero rf fixes: #16844 Signed-off-by: Pavel Emelyanov Closes scylladb/scylladb#16861 --- locator/network_topology_strategy.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/locator/network_topology_strategy.cc b/locator/network_topology_strategy.cc index 798b7ca185..80c929a780 100644 --- a/locator/network_topology_strategy.cc +++ b/locator/network_topology_strategy.cc @@ -305,7 +305,7 @@ static unsigned calculate_initial_tablets_from_topology(const schema& s, const t rf_in_dc = it->second; } - unsigned tablets_in_dc = (shards_in_dc + rf_in_dc - 1) / rf_in_dc; + unsigned tablets_in_dc = rf_in_dc > 0 ? (shards_in_dc + rf_in_dc - 1) / rf_in_dc : 0; initial_tablets = std::max(initial_tablets, tablets_in_dc); } rslogger.debug("Estimated {} initial tablets for table {}.{}", initial_tablets, s.ks_name(), s.cf_name()); From 263e2fabae4021dcd1022bc3ba409c3fe57f1923 Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Fri, 19 Jan 2024 10:31:48 +0800 Subject: [PATCH 5/5] auth: do not include unused headers these unused includes were identified by clangd. see https://clangd.llvm.org/guides/include-cleaner#unused-include-warning for more details on the "Unused include" warning. Signed-off-by: Kefu Chai --- auth/allow_all_authorizer.hh | 1 - auth/authentication_options.hh | 1 - auth/authenticator.cc | 4 ---- auth/authenticator.hh | 5 ----- auth/authorizer.hh | 2 -- auth/common.cc | 4 ++-- auth/common.hh | 3 --- auth/default_authorizer.cc | 6 +----- auth/default_authorizer.hh | 2 -- auth/maintenance_socket_role_manager.cc | 1 - auth/maintenance_socket_role_manager.hh | 2 -- auth/password_authenticator.cc | 3 +-- auth/password_authenticator.hh | 1 + auth/passwords.cc | 1 - auth/permissions_cache.cc | 1 - auth/permissions_cache.hh | 5 ----- auth/resource.cc | 2 -- auth/role_manager.hh | 1 - auth/service.cc | 4 ---- auth/standard_role_manager.cc | 1 - auth/standard_role_manager.hh | 1 - 21 files changed, 5 insertions(+), 46 deletions(-) diff --git a/auth/allow_all_authorizer.hh b/auth/allow_all_authorizer.hh index 22a57d1272..0b64165874 100644 --- a/auth/allow_all_authorizer.hh +++ b/auth/allow_all_authorizer.hh @@ -9,7 +9,6 @@ #pragma once #include "auth/authorizer.hh" -#include "exceptions/exceptions.hh" namespace cql3 { class query_processor; diff --git a/auth/authentication_options.hh b/auth/authentication_options.hh index 2d6ff10a6d..06b5b2d15e 100644 --- a/auth/authentication_options.hh +++ b/auth/authentication_options.hh @@ -8,7 +8,6 @@ #pragma once -#include #include #include #include diff --git a/auth/authenticator.cc b/auth/authenticator.cc index 890afbb97d..38ab93a37a 100644 --- a/auth/authenticator.cc +++ b/auth/authenticator.cc @@ -11,10 +11,6 @@ #include "auth/authenticator.hh" #include "auth/authenticated_user.hh" -#include "auth/common.hh" -#include "auth/password_authenticator.hh" -#include "cql3/query_processor.hh" -#include "utils/class_registrator.hh" const sstring auth::authenticator::USERNAME_KEY("username"); const sstring auth::authenticator::PASSWORD_KEY("password"); diff --git a/auth/authenticator.hh b/auth/authenticator.hh index ce945291b4..d76cdd5a47 100644 --- a/auth/authenticator.hh +++ b/auth/authenticator.hh @@ -12,8 +12,6 @@ #include #include -#include -#include #include #include #include @@ -26,9 +24,6 @@ #include "auth/authentication_options.hh" #include "auth/resource.hh" #include "auth/sasl_challenge.hh" -#include "bytes.hh" -#include "enum_set.hh" -#include "exceptions/exceptions.hh" namespace db { class config; diff --git a/auth/authorizer.hh b/auth/authorizer.hh index 1488418084..38a58f0c7e 100644 --- a/auth/authorizer.hh +++ b/auth/authorizer.hh @@ -11,8 +11,6 @@ #pragma once #include -#include -#include #include #include #include diff --git a/auth/common.cc b/auth/common.cc index 731bf580f4..7533f94665 100644 --- a/auth/common.cc +++ b/auth/common.cc @@ -6,14 +6,14 @@ * SPDX-License-Identifier: AGPL-3.0-or-later */ -#include #include "auth/common.hh" +#include #include +#include "utils/exponential_backoff_retry.hh" #include "cql3/query_processor.hh" #include "cql3/statements/create_table_statement.hh" -#include "replica/database.hh" #include "schema/schema_builder.hh" #include "service/migration_manager.hh" #include "timeout_config.hh" diff --git a/auth/common.hh b/auth/common.hh index a74b29d67a..83c76f9820 100644 --- a/auth/common.hh +++ b/auth/common.hh @@ -8,7 +8,6 @@ #pragma once -#include #include #include @@ -19,9 +18,7 @@ #include #include -#include "log.hh" #include "seastarx.hh" -#include "utils/exponential_backoff_retry.hh" using namespace std::chrono_literals; diff --git a/auth/default_authorizer.cc b/auth/default_authorizer.cc index 6c71b1772b..f978b85cab 100644 --- a/auth/default_authorizer.cc +++ b/auth/default_authorizer.cc @@ -15,14 +15,11 @@ extern "C" { #include } -#include -#include - #include #include #include +#include -#include "auth/authenticated_user.hh" #include "auth/common.hh" #include "auth/permission.hh" #include "auth/role_or_anonymous.hh" @@ -30,7 +27,6 @@ extern "C" { #include "cql3/untyped_result_set.hh" #include "exceptions/exceptions.hh" #include "log.hh" -#include "replica/database.hh" #include "utils/class_registrator.hh" namespace auth { diff --git a/auth/default_authorizer.hh b/auth/default_authorizer.hh index 61d456a4c6..5367962cfa 100644 --- a/auth/default_authorizer.hh +++ b/auth/default_authorizer.hh @@ -10,8 +10,6 @@ #pragma once -#include - #include #include "auth/authorizer.hh" diff --git a/auth/maintenance_socket_role_manager.cc b/auth/maintenance_socket_role_manager.cc index 4c946eecb1..eeb5cee79e 100644 --- a/auth/maintenance_socket_role_manager.cc +++ b/auth/maintenance_socket_role_manager.cc @@ -11,7 +11,6 @@ #include #include #include -#include "log.hh" #include "utils/class_registrator.hh" namespace auth { diff --git a/auth/maintenance_socket_role_manager.hh b/auth/maintenance_socket_role_manager.hh index 8db277f4b1..61f05059b8 100644 --- a/auth/maintenance_socket_role_manager.hh +++ b/auth/maintenance_socket_role_manager.hh @@ -10,9 +10,7 @@ #include "auth/resource.hh" #include "auth/role_manager.hh" -#include "authorizer.hh" #include "seastar/core/future.hh" -#include namespace cql3 { class query_processor; diff --git a/auth/password_authenticator.cc b/auth/password_authenticator.cc index ce7c938964..684b54cd2a 100644 --- a/auth/password_authenticator.cc +++ b/auth/password_authenticator.cc @@ -10,14 +10,13 @@ #include "auth/password_authenticator.hh" -#include -#include #include #include #include #include #include +#include #include "auth/authenticated_user.hh" #include "auth/common.hh" diff --git a/auth/password_authenticator.hh b/auth/password_authenticator.hh index 4193149fab..e4b9c7f4ba 100644 --- a/auth/password_authenticator.hh +++ b/auth/password_authenticator.hh @@ -12,6 +12,7 @@ #include +#include "db/consistency_level_type.hh" #include "auth/authenticator.hh" namespace db { diff --git a/auth/passwords.cc b/auth/passwords.cc index f345f3484c..1843b0139f 100644 --- a/auth/passwords.cc +++ b/auth/passwords.cc @@ -9,7 +9,6 @@ #include "auth/passwords.hh" #include -#include extern "C" { #include diff --git a/auth/permissions_cache.cc b/auth/permissions_cache.cc index 5f8322d399..ff09c84621 100644 --- a/auth/permissions_cache.cc +++ b/auth/permissions_cache.cc @@ -9,7 +9,6 @@ #include "auth/permissions_cache.hh" #include "auth/authorizer.hh" -#include "auth/common.hh" #include "auth/service.hh" namespace auth { diff --git a/auth/permissions_cache.hh b/auth/permissions_cache.hh index 2c55a9c8dc..8dc94e22cc 100644 --- a/auth/permissions_cache.hh +++ b/auth/permissions_cache.hh @@ -8,11 +8,7 @@ #pragma once -#include -#include -#include #include -#include #include #include @@ -20,7 +16,6 @@ #include #include -#include "auth/authenticated_user.hh" #include "auth/permission.hh" #include "auth/resource.hh" #include "auth/role_or_anonymous.hh" diff --git a/auth/resource.cc b/auth/resource.cc index 5de272b4e8..aee3a465ee 100644 --- a/auth/resource.cc +++ b/auth/resource.cc @@ -19,8 +19,6 @@ #include #include -#include "service/storage_proxy.hh" -#include "data_dictionary/user_types_metadata.hh" #include "cql3/util.hh" #include "db/marshal/type_parser.hh" diff --git a/auth/role_manager.hh b/auth/role_manager.hh index 6ed5ce1c6b..6d89dfba76 100644 --- a/auth/role_manager.hh +++ b/auth/role_manager.hh @@ -11,7 +11,6 @@ #include #include #include -#include #include #include diff --git a/auth/service.cc b/auth/service.cc index 64bcf272c0..42c1cc08ea 100644 --- a/auth/service.cc +++ b/auth/service.cc @@ -11,7 +11,6 @@ #include "auth/service.hh" #include -#include #include #include @@ -21,20 +20,17 @@ #include "auth/allow_all_authorizer.hh" #include "auth/common.hh" #include "auth/role_or_anonymous.hh" -#include "cql3/functions/function_name.hh" #include "cql3/functions/functions.hh" #include "cql3/query_processor.hh" #include "cql3/untyped_result_set.hh" #include "db/config.hh" #include "db/consistency_level_type.hh" #include "db/functions/function_name.hh" -#include "exceptions/exceptions.hh" #include "log.hh" #include "service/migration_manager.hh" #include "utils/class_registrator.hh" #include "locator/abstract_replication_strategy.hh" #include "data_dictionary/keyspace_metadata.hh" -#include "mutation/mutation.hh" namespace auth { diff --git a/auth/standard_role_manager.cc b/auth/standard_role_manager.cc index e8a089abd6..bbe0d290da 100644 --- a/auth/standard_role_manager.cc +++ b/auth/standard_role_manager.cc @@ -27,7 +27,6 @@ #include "exceptions/exceptions.hh" #include "log.hh" #include "utils/class_registrator.hh" -#include "replica/database.hh" #include "service/migration_manager.hh" #include "password_authenticator.hh" diff --git a/auth/standard_role_manager.hh b/auth/standard_role_manager.hh index 2090deb6ec..6e3b67876d 100644 --- a/auth/standard_role_manager.hh +++ b/auth/standard_role_manager.hh @@ -11,7 +11,6 @@ #include "auth/role_manager.hh" #include -#include #include #include