From f80f638bb93206cce8ace93b92e4aef35e358a14 Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Sat, 29 Apr 2023 15:48:01 +0800 Subject: [PATCH 1/6] raft: disambiguate promise name in raft::awaited_conf_changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit otherwise GCC 13 complains that ``` /home/kefu/dev/scylladb/raft/server.cc:42:15: error: declaration of ‘seastar::promise raft::awaited_index::promise’ changes meaning of ‘promise’ [-Wchanges-meaning] 42 | promise<> promise; | ^~~~~~~ /home/kefu/dev/scylladb/raft/server.cc:42:5: note: used here to mean ‘class seastar::promise’ 42 | promise<> promise; | ^~~~~~~~~ ``` see also cd4af0c722ce82ef944ae982147e52cf21c30265 Signed-off-by: Kefu Chai --- raft/server.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/raft/server.cc b/raft/server.cc index 9430a51e4d..d29f47b01c 100644 --- a/raft/server.cc +++ b/raft/server.cc @@ -39,12 +39,12 @@ struct active_read { }; struct awaited_index { - promise<> promise; + seastar::promise<> promise; optimized_optional abort; }; struct awaited_conf_change { - promise<> promise; + seastar::promise<> promise; optimized_optional abort; }; From 6d8188ad70395823fcce77c222bfca78d9db154f Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Sat, 29 Apr 2023 15:51:05 +0800 Subject: [PATCH 2/6] locator: topology: disambiguate type names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit otherwise GCC-13 complains: ``` /home/kefu/dev/scylladb/locator/topology.hh:70:21: error: declaration of ‘const locator::topology* locator::node::topology() const’ changes meaning of ‘topology’ [-Wchanges-meaning] 70 | const topology* topology() const noexcept { | ^~~~~~~~ ``` Signed-off-by: Kefu Chai --- locator/topology.hh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/locator/topology.hh b/locator/topology.hh index ad826d5840..2701828fbe 100644 --- a/locator/topology.hh +++ b/locator/topology.hh @@ -51,8 +51,8 @@ public: }; private: - const topology* _topology; - host_id _host_id; + const locator::topology* _topology; + locator::host_id _host_id; inet_address _endpoint; endpoint_dc_rack _dc_rack; state _state; @@ -67,11 +67,11 @@ public: node(const node&) = delete; node(node&&) = delete; - const topology* topology() const noexcept { + const locator::topology* topology() const noexcept { return _topology; } - const host_id& host_id() const noexcept { + const locator::host_id& host_id() const noexcept { return _host_id; } From 48387a5a9a2b4506a68a07f1031efdce9247cb3c Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Sat, 29 Apr 2023 16:30:55 +0800 Subject: [PATCH 3/6] reader_concurrency_semaphore: fix signed/unsigned comparision MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit a signed/unsigned comparsion can overflow. and GCC-13 rightly points this out. so let's use `std::cmp_greater_equal()` when comparing unsigned and signed for greater-or-equal. ``` /home/kefu/dev/scylladb/reader_concurrency_semaphore.cc:931:76: error: comparison of integer expressions of different signedness: ‘long int’ and ‘uint64_t’ {aka ‘long unsigned int’} [-Werror=sign-compare] 931 | if (_resources.memory <= 0 && (consumed_resources().memory + r.memory) >= get_kill_limit()) [[unlikely]] { | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~ ``` Signed-off-by: Kefu Chai --- reader_concurrency_semaphore.cc | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/reader_concurrency_semaphore.cc b/reader_concurrency_semaphore.cc index f53c0f08fb..9ab2d130a4 100644 --- a/reader_concurrency_semaphore.cc +++ b/reader_concurrency_semaphore.cc @@ -13,6 +13,7 @@ #include #include #include +#include #include "reader_concurrency_semaphore.hh" #include "readers/flat_mutation_reader_v2.hh" @@ -928,7 +929,7 @@ void reader_concurrency_semaphore::consume(reader_permit::impl& permit, resource // We check whether we even reached the memory limit first. // This is a cheap check and should be false most of the time, providing a // cheap short-circuit. - if (_resources.memory <= 0 && (consumed_resources().memory + r.memory) >= get_kill_limit()) [[unlikely]] { + if (_resources.memory <= 0 && std::cmp_greater_equal(consumed_resources().memory + r.memory, get_kill_limit())) [[unlikely]] { if (permit.on_oom_kill()) { ++_stats.total_reads_killed_due_to_kill_limit; } @@ -1220,10 +1221,10 @@ reader_concurrency_semaphore::admit_result reader_concurrency_semaphore::can_admit_read(const reader_permit::impl& permit) const noexcept { if (_resources.memory < 0) [[unlikely]] { const auto consumed_memory = consumed_resources().memory; - if (consumed_memory >= get_kill_limit()) { + if (std::cmp_greater_equal(consumed_memory, get_kill_limit())) { return {can_admit::no, reason::memory_resources}; } - if (consumed_memory >= get_serialize_limit()) { + if (std::cmp_greater_equal(consumed_memory, get_serialize_limit())) { if (_blessed_permit) { // blessed permit is never in the wait list return {can_admit::no, reason::memory_resources}; From 56511a42d028a822c285f035c4c2e1db68fec535 Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Sat, 29 Apr 2023 16:34:27 +0800 Subject: [PATCH 4/6] db: schema_tables: drop unused variable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit this also silence the warning from GCC-13: ``` /home/kefu/dev/scylladb/db/schema_tables.cc:1489:10: error: variable ‘ts’ set but not used [-Werror=unused-but-set-variable] 1489 | auto ts = db_clock::now(); | ^~ ``` Signed-off-by: Kefu Chai --- db/schema_tables.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/db/schema_tables.cc b/db/schema_tables.cc index 0110966743..c67683c44a 100644 --- a/db/schema_tables.cc +++ b/db/schema_tables.cc @@ -1486,7 +1486,6 @@ static future<> merge_tables_and_views(distributed& prox // to a mv not finding its schema when snapshoting since the main table // was already dropped (see https://github.com/scylladb/scylla/issues/5614) auto& db = proxy.local().get_db(); - auto ts = db_clock::now(); co_await max_concurrent_for_each(views_diff.dropped, max_concurrent, [&db] (schema_diff::dropped_schema& dt) { auto& s = *dt.schema.get(); return replica::database::drop_table_on_all_shards(db, s.ks_name(), s.cf_name()); From 0232115eaab6cd09d90b49da84c70fe484b238aa Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Sat, 29 Apr 2023 16:38:28 +0800 Subject: [PATCH 5/6] compaction: disambiguate type name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit otherwise GCC-13 complains: ``` /home/kefu/dev/scylladb/compaction/compaction_state.hh:38:22: error: declaration of ‘compaction::owned_ranges_ptr compaction::compaction_state::owned_ranges_ptr’ changes meaning of ‘owned_ranges_ptr’ [-Wchanges-meaning] 38 | owned_ranges_ptr owned_ranges_ptr; | ^~~~~~~~~~~~~~~~ ``` Signed-off-by: Kefu Chai --- compaction/compaction_state.hh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compaction/compaction_state.hh b/compaction/compaction_state.hh index 729992e9b1..4588aab146 100644 --- a/compaction/compaction_state.hh +++ b/compaction/compaction_state.hh @@ -35,7 +35,7 @@ struct compaction_state { compaction_backlog_tracker backlog_tracker; std::unordered_set sstables_requiring_cleanup; - owned_ranges_ptr owned_ranges_ptr; + compaction::owned_ranges_ptr owned_ranges_ptr; explicit compaction_state(table_state& t); compaction_state(compaction_state&&) = delete; From e333bcc2da926ca99846c30571997e626d6a0d10 Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Sat, 29 Apr 2023 16:52:12 +0800 Subject: [PATCH 6/6] cdc: initialize an optional using its value type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit as this syntax is not supported by the standard, it seems clang just silently construct the value with the initializer list and calls the operator=, but GCC complains: ``` /home/kefu/dev/scylladb/cdc/split.cc:392:54: error: converting to ‘std::optional’ from initializer list would use explicit constructor ‘constexpr std::optional<_Tp>::optional(_Up&&) [with _Up = const tombstone&; typename std::enable_if<__and_v, typename std::remove_cv::type>::type> >, std::__not_::type>::type> >, std::is_constructible<_Tp, _Up>, std::__not_ > >, bool>::type = false; _Tp = partition_deletion]’ 392 | _result[t.timestamp].partition_deletions = {t}; | ^ ``` to silences the error, and to be more standard compliant, let's use emplace() instead. Signed-off-by: Kefu Chai --- cdc/split.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cdc/split.cc b/cdc/split.cc index cabb6903b9..5840dd76ff 100644 --- a/cdc/split.cc +++ b/cdc/split.cc @@ -389,7 +389,7 @@ struct extract_changes_visitor { } void partition_delete(const tombstone& t) { - _result[t.timestamp].partition_deletions = {t}; + _result[t.timestamp].partition_deletions = partition_deletion{t}; } constexpr bool finished() const { return false; }