Merge 'gms: define and use generation and version types' from Benny H…

…alevy This series cleans up the generation and value types used in gms / gossiper. Currently we use a blend of int, int32_t, and int64_t around messaging. This change defines gms::generation_type and gms::version_type as int32_t and add check in non-release modes that the respective int64 value passed over messaging do not overflow 32 bits. Closes scylladb#12966 * github.com:scylladb/scylladb: gossiper: version_generator: add {debug_,}validate_gossip_generation gms: gossip_digest: use generation_type and version_type gms: heart_beat_state: use generation_type and version_type gms: versioned_value: use version_type gms: version_generator: define version_type and generation_type strong types utils: move generation-number to gms utils: add tagged_integer gms: versioned_value: make members private scylla-gdb: add get_gms_versioned_value gms: versioned_value: delete unused compare_to function gms: gossip_digest: delete unused compare_to function
sarvex · Apr 24, 2023 · 7f04d82 · 7f04d82
2 parents 002bdd7 + 5520d3a
commit 7f04d82
Show file tree

Hide file tree

Showing 41 changed files with 416 additions and 306 deletions.
diff --git a/api/failure_detector.cc b/api/failure_detector.cc
@@ -23,16 +23,16 @@ void set_failure_detector(http_context& ctx, routes& r, gms::gossiper& g) {
             fd::endpoint_state val;
             val.addrs = fmt::to_string(i.first);
             val.is_alive = i.second.is_alive();
-            val.generation = i.second.get_heart_beat_state().get_generation();
-            val.version = i.second.get_heart_beat_state().get_heart_beat_version();
+            val.generation = i.second.get_heart_beat_state().get_generation().value();
+            val.version = i.second.get_heart_beat_state().get_heart_beat_version().value();
             val.update_time = i.second.get_update_timestamp().time_since_epoch().count();
             for (auto a : i.second.get_application_state_map()) {
                 fd::version_value version_val;
                 // We return the enum index and not it's name to stay compatible to origin
                 // method that the state index are static but the name can be changed.
                 version_val.application_state = static_cast<std::underlying_type<gms::application_state>::type>(a.first);
-                version_val.value = a.second.value;
-                version_val.version = a.second.version;
+                version_val.value = a.second.value();
+                version_val.version = a.second.version().value();
                 val.application_state.push(version_val);
             }
             res.push_back(val);

diff --git a/api/gossiper.cc b/api/gossiper.cc
@@ -34,15 +34,15 @@ void set_gossiper(http_context& ctx, routes& r, gms::gossiper& g) {
 
     httpd::gossiper_json::get_current_generation_number.set(r, [&g] (std::unique_ptr<http::request> req) {
         gms::inet_address ep(req->param["addr"]);
-        return g.get_current_generation_number(ep).then([] (int res) {
-            return make_ready_future<json::json_return_type>(res);
+        return g.get_current_generation_number(ep).then([] (gms::generation_type res) {
+            return make_ready_future<json::json_return_type>(res.value());
         });
     });
 
     httpd::gossiper_json::get_current_heart_beat_version.set(r, [&g] (std::unique_ptr<http::request> req) {
         gms::inet_address ep(req->param["addr"]);
-        return g.get_current_heart_beat_version(ep).then([] (int res) {
-            return make_ready_future<json::json_return_type>(res);
+        return g.get_current_heart_beat_version(ep).then([] (gms::version_type res) {
+            return make_ready_future<json::json_return_type>(res.value());
         });
     });
 

diff --git a/api/storage_service.cc b/api/storage_service.cc
@@ -655,8 +655,8 @@ void set_storage_service(http_context& ctx, routes& r, sharded<service::storage_
 
     ss::get_current_generation_number.set(r, [&g](std::unique_ptr<http::request> req) {
         gms::inet_address ep(utils::fb_utilities::get_broadcast_address());
-        return g.get_current_generation_number(ep).then([](int res) {
-            return make_ready_future<json::json_return_type>(res);
+        return g.get_current_generation_number(ep).then([](gms::generation_type res) {
+            return make_ready_future<json::json_return_type>(res.value());
         });
     });
 

diff --git a/cdc/generation.cc b/cdc/generation.cc
@@ -35,12 +35,12 @@ extern logging::logger cdc_log;
 
 static int get_shard_count(const gms::inet_address& endpoint, const gms::gossiper& g) {
     auto ep_state = g.get_application_state_ptr(endpoint, gms::application_state::SHARD_COUNT);
-    return ep_state ? std::stoi(ep_state->value) : -1;
+    return ep_state ? std::stoi(ep_state->value()) : -1;
 }
 
 static unsigned get_sharding_ignore_msb(const gms::inet_address& endpoint, const gms::gossiper& g) {
     auto ep_state = g.get_application_state_ptr(endpoint, gms::application_state::IGNORE_MSB_BITS);
-    return ep_state ? std::stoi(ep_state->value) : 0;
+    return ep_state ? std::stoi(ep_state->value()) : 0;
 }
 
 namespace db {
@@ -775,7 +775,7 @@ future<> generation_service::on_change(gms::inet_address ep, gms::application_st
         return make_ready_future();
     }
 
-    auto gen_id = gms::versioned_value::cdc_generation_id_from_string(v.value);
+    auto gen_id = gms::versioned_value::cdc_generation_id_from_string(v.value());
     cdc_log.debug("Endpoint: {}, CDC generation ID change: {}", ep, gen_id);
 
     return legacy_handle_cdc_generation(gen_id);

diff --git a/configure.py b/configure.py
@@ -700,7 +700,7 @@ def find_headers(repodir, excluded_dirs):
                 'utils/limiting_data_source.cc',
                 'utils/updateable_value.cc',
                 'utils/directories.cc',
-                'utils/generation-number.cc',
+                'gms/generation-number.cc',
                 'utils/rjson.cc',
                 'utils/human_readable.cc',
                 'utils/histogram_metrics_helper.cc',
@@ -1155,6 +1155,7 @@ def find_headers(repodir, excluded_dirs):
         'idl/position_in_partition.idl.hh',
         'idl/experimental/broadcast_tables_lang.idl.hh',
         'idl/storage_service.idl.hh',
+        'idl/utils.idl.hh',
         ]
 
 headers = find_headers('.', excluded_dirs=['idl', 'build', 'seastar', '.git'])

diff --git a/db/system_keyspace.cc b/db/system_keyspace.cc
@@ -51,7 +51,7 @@
 #include "db/view/build_progress_virtual_reader.hh"
 #include "db/schema_tables.hh"
 #include "index/built_indexes_virtual_reader.hh"
-#include "utils/generation-number.hh"
+#include "gms/generation-number.hh"
 #include "db/virtual_table.hh"
 #include "service/storage_service.hh"
 #include "protocol_server.hh"
@@ -3120,16 +3120,16 @@ future<> system_keyspace::get_repair_history(::table_id table_id, repair_history
 future<int> system_keyspace::increment_and_get_generation() {
     auto req = format("SELECT gossip_generation FROM system.{} WHERE key='{}'", LOCAL, LOCAL);
     auto rs = co_await _qp.local().execute_internal(req, cql3::query_processor::cache_internal::yes);
-    int generation;
+    gms::generation_type generation;
     if (rs->empty() || !rs->one().has("gossip_generation")) {
         // seconds-since-epoch isn't a foolproof new generation
         // (where foolproof is "guaranteed to be larger than the last one seen at this ip address"),
         // but it's as close as sanely possible
-        generation = utils::get_generation_number();
+        generation = gms::get_generation_number();
     } else {
         // Other nodes will ignore gossip messages about a node that have a lower generation than previously seen.
-        int stored_generation = rs->one().template get_as<int>("gossip_generation") + 1;
-        int now = utils::get_generation_number();
+        auto stored_generation = gms::generation_type(rs->one().template get_as<int>("gossip_generation") + 1);
+        auto now = gms::get_generation_number();
         if (stored_generation >= now) {
             slogger.warn("Using stored Gossip Generation {} as it is greater than current system time {}."
                         "See CASSANDRA-3654 if you experience problems", stored_generation, now);
@@ -3139,7 +3139,7 @@ future<int> system_keyspace::increment_and_get_generation() {
         }
     }
     req = format("INSERT INTO system.{} (key, gossip_generation) VALUES ('{}', ?)", LOCAL, LOCAL);
-    co_await _qp.local().execute_internal(req, {generation}, cql3::query_processor::cache_internal::yes);
+    co_await _qp.local().execute_internal(req, {generation.value()}, cql3::query_processor::cache_internal::yes);
     co_await force_blocking_flush(LOCAL);
     co_return generation;
 }

diff --git a/gms/endpoint_state.cc b/gms/endpoint_state.cc
@@ -14,7 +14,7 @@
 
 namespace gms {
 
-static_assert(!std::is_default_constructible_v<heart_beat_state>);
+static_assert(std::is_default_constructible_v<heart_beat_state>);
 static_assert(std::is_nothrow_copy_constructible_v<heart_beat_state>);
 static_assert(std::is_nothrow_move_constructible_v<heart_beat_state>);
 
@@ -48,7 +48,7 @@ bool endpoint_state::is_cql_ready() const noexcept {
         return false;
     }
     try {
-        return boost::lexical_cast<int>(app_state->value);
+        return boost::lexical_cast<int>(app_state->value());
     } catch (...) {
         return false;
     }

diff --git a/gms/endpoint_state.hh b/gms/endpoint_state.hh
@@ -43,7 +43,7 @@ public:
     }
 
     endpoint_state() noexcept
-        : _heart_beat_state(0)
+        : _heart_beat_state()
         , _update_timestamp(clk::now())
         , _is_alive(true) {
         update_is_normal();
@@ -140,7 +140,7 @@ public:
         if (!app_state) {
             return empty;
         }
-        const auto& value = app_state->value;
+        const auto& value = app_state->value();
         if (value.empty()) {
             return empty;
         }

diff --git a/gms/generation-number.cc b/gms/generation-number.cc
@@ -0,0 +1,36 @@
+/*
+ * Copyright 2020-present ScyllaDB
+ */
+
+/*
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+#include <cassert>
+#include <chrono>
+#include <utility>
+#include <exception>
+
+#include <fmt/format.h>
+
+#include "generation-number.hh"
+
+namespace gms {
+
+generation_type get_generation_number() {
+    using namespace std::chrono;
+    auto now = high_resolution_clock::now().time_since_epoch();
+    int generation_number = duration_cast<seconds>(now).count();
+    auto ret = generation_type(generation_number);
+    // Make sure the clock didn't overflow the 32 bits value
+    assert(ret.value() == generation_number);
+    return ret;
+}
+
+void validate_gossip_generation(int64_t generation_number) {
+    if (!std::in_range<gms::generation_type::value_type>(generation_number)) {
+        throw std::out_of_range(fmt::format("gossip generation {} is out of range", generation_number));
+    }
+}
+
+}
diff --git a/gms/generation-number.hh b/gms/generation-number.hh
@@ -0,0 +1,26 @@
+/*
+ * Copyright 2020-present ScyllaDB
+ */
+
+/*
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+#pragma once
+
+#include "utils/tagged_integer.hh"
+
+namespace gms {
+
+using generation_type = utils::tagged_integer<struct generation_type_tag, int32_t>;
+
+generation_type get_generation_number();
+
+void validate_gossip_generation(int64_t generation_number);
+inline void debug_validate_gossip_generation([[maybe_unused]] int64_t generation_number) {
+#ifndef SCYLLA_BUILD_MODE_RELEASE
+    validate_gossip_generation(generation_number);
+#endif
+}
+
+}
diff --git a/gms/gossip_digest.hh b/gms/gossip_digest.hh
@@ -13,6 +13,8 @@
 #include <seastar/core/sstring.hh>
 #include "utils/serialization.hh"
 #include "gms/inet_address.hh"
+#include "gms/generation-number.hh"
+#include "gms/version_generator.hh"
 
 namespace gms {
 
@@ -24,16 +26,12 @@ class gossip_digest { // implements Comparable<GossipDigest>
 private:
     using inet_address = gms::inet_address;
     inet_address _endpoint;
-    int32_t _generation;
-    int32_t _max_version;
+    generation_type _generation;
+    version_type _max_version;
 public:
-    gossip_digest()
-        : _endpoint(0)
-        , _generation(0)
-        , _max_version(0) {
-    }
+    gossip_digest() = default;
 
-    gossip_digest(inet_address ep, int32_t gen, int32_t version)
+    explicit gossip_digest(inet_address ep, generation_type gen = {}, version_type version = {}) noexcept
         : _endpoint(ep)
         , _generation(gen)
         , _max_version(version) {
@@ -43,21 +41,14 @@ public:
         return _endpoint;
     }
 
-    int32_t get_generation() const {
+    generation_type get_generation() const {
         return _generation;
     }
 
-    int32_t get_max_version() const {
+    version_type get_max_version() const {
         return _max_version;
     }
 
-    int32_t compare_to(gossip_digest d) const {
-        if (_generation != d.get_generation()) {
-            return (_generation - d.get_generation());
-        }
-        return (_max_version - d.get_max_version());
-    }
-
     friend bool operator<(const gossip_digest& x, const gossip_digest& y) {
         if (x._generation != y._generation) {
             return x._generation < y._generation;