topology: add_or_update_endpoint: make host_id non-optional

Move the argument to front, since inet_address is optional. token_metadata::update_topology with inet_address argument is removed since it's not used anymore.
scylladb · Nov 7, 2023 · ec2358a · ec2358a
1 parent 9dabee5
commit ec2358a
Show file tree

Hide file tree

Showing 8 changed files with 20 additions and 27 deletions.
diff --git a/locator/token_metadata.cc b/locator/token_metadata.cc
@@ -116,12 +116,8 @@ class token_metadata_impl final {
         return _bootstrap_tokens;
     }
 
-    void update_topology(inet_address ep, std::optional<endpoint_dc_rack> opt_dr, std::optional<node::state> opt_st, std::optional<shard_id> shard_count = std::nullopt) {
-        _topology.add_or_update_endpoint(ep, std::nullopt, std::move(opt_dr), std::move(opt_st), std::move(shard_count));
-    }
-
-    void update_topology(host_id ep, std::optional<endpoint_dc_rack> opt_dr, std::optional<node::state> opt_st, std::optional<shard_id> shard_count = std::nullopt) {
-        _topology.add_or_update_endpoint(std::nullopt, ep, std::move(opt_dr), std::move(opt_st), std::move(shard_count));
+    void update_topology(host_id id, std::optional<endpoint_dc_rack> opt_dr, std::optional<node::state> opt_st, std::optional<shard_id> shard_count = std::nullopt) {
+        _topology.add_or_update_endpoint(id, std::nullopt, std::move(opt_dr), std::move(opt_st), std::move(shard_count));
     }
 
     /**
@@ -529,7 +525,7 @@ void token_metadata_impl::debug_show() const {
 }
 
 void token_metadata_impl::update_host_id(const host_id& host_id, inet_address endpoint) {
-    _topology.add_or_update_endpoint(endpoint, host_id);
+    _topology.add_or_update_endpoint(host_id, endpoint);
 }
 
 host_id token_metadata_impl::get_host_id(inet_address endpoint) const {

diff --git a/locator/topology.cc b/locator/topology.cc
@@ -438,25 +438,22 @@ const node* topology::find_node(node::idx_type idx) const noexcept {
     return _nodes.at(idx).get();
 }
 
-const node* topology::add_or_update_endpoint(std::optional<inet_address> opt_ep, std::optional<host_id> opt_id, std::optional<endpoint_dc_rack> opt_dr, std::optional<node::state> opt_st, std::optional<shard_id> shard_count)
+const node* topology::add_or_update_endpoint(host_id id, std::optional<inet_address> opt_ep, std::optional<endpoint_dc_rack> opt_dr, std::optional<node::state> opt_st, std::optional<shard_id> shard_count)
 {
     if (tlogger.is_enabled(log_level::trace)) {
-        tlogger.trace("topology[{}]: add_or_update_endpoint: ep={} host_id={} dc={} rack={} state={} shards={}, at {}", fmt::ptr(this),
-            opt_ep, opt_id.value_or(host_id::create_null_id()), opt_dr.value_or(endpoint_dc_rack{}).dc, opt_dr.value_or(endpoint_dc_rack{}).rack, opt_st.value_or(node::state::none), shard_count,
+        tlogger.trace("topology[{}]: add_or_update_endpoint: host_id={} ep={} dc={} rack={} state={} shards={}, at {}", fmt::ptr(this),
+            id, opt_ep, opt_dr.value_or(endpoint_dc_rack{}).dc, opt_dr.value_or(endpoint_dc_rack{}).rack, opt_st.value_or(node::state::none), shard_count,
             current_backtrace());
     }
 
-    if (!opt_id) {
-        on_internal_error(tlogger, format("topology: host_id is not set, ep={}", opt_ep));
-    }
-    const auto* n = find_node(*opt_id);
+    const auto* n = find_node(id);
     if (n) {
         return update_node(make_mutable(n), std::nullopt, opt_ep, std::move(opt_dr), std::move(opt_st), std::move(shard_count));
     } else if (opt_ep && (n = find_node(*opt_ep))) {
-        return update_node(make_mutable(n), opt_id, std::nullopt, std::move(opt_dr), std::move(opt_st), std::move(shard_count));
+        return update_node(make_mutable(n), id, std::nullopt, std::move(opt_dr), std::move(opt_st), std::move(shard_count));
     }
 
-    return add_node(opt_id.value_or(host_id::create_null_id()),
+    return add_node(id,
                     opt_ep.value_or(inet_address{}),
                     opt_dr.value_or(endpoint_dc_rack::default_location),
                     opt_st.value_or(node::state::normal),

diff --git a/locator/topology.hh b/locator/topology.hh
@@ -233,17 +233,17 @@ public:
      *
      * Adds or updates a node with given endpoint
      */
-    const node* add_or_update_endpoint(std::optional<inet_address> ep, std::optional<host_id> opt_id,
+    const node* add_or_update_endpoint(host_id id, std::optional<inet_address> opt_ep,
                                        std::optional<endpoint_dc_rack> opt_dr,
                                        std::optional<node::state> opt_st,
                                        std::optional<shard_id> shard_count = std::nullopt);
 
     // Legacy entry point from token_metadata::update_topology
-    const node* add_or_update_endpoint(inet_address ep, endpoint_dc_rack dr, std::optional<node::state> opt_st) {
-        return add_or_update_endpoint(ep, std::nullopt, std::move(dr), std::move(opt_st), std::nullopt);
+    const node* add_or_update_endpoint(host_id id, endpoint_dc_rack dr, std::optional<node::state> opt_st) {
+        return add_or_update_endpoint(id, std::nullopt, std::move(dr), std::move(opt_st), std::nullopt);
     }
-    const node* add_or_update_endpoint(inet_address ep, host_id id) {
-        return add_or_update_endpoint(ep, id, std::nullopt, std::nullopt, std::nullopt);
+    const node* add_or_update_endpoint(host_id id, inet_address ep) {
+        return add_or_update_endpoint(id, ep, std::nullopt, std::nullopt, std::nullopt);
     }
 
     /**

diff --git a/main.cc b/main.cc
@@ -1198,7 +1198,7 @@ To start the scylla server proper, simply invoke as: scylla server (or just scyl
               // Raft topology discard the endpoint-to-id map, so the local id can
               // still be found in the config.
               tm.get_topology().set_host_id_cfg(host_id);
-              tm.get_topology().add_or_update_endpoint(endpoint, host_id);
+              tm.get_topology().add_or_update_endpoint(host_id, endpoint);
               return make_ready_future<>();
           }).get();
 

diff --git a/test/boost/locator_topology_test.cc b/test/boost/locator_topology_test.cc
@@ -114,7 +114,7 @@ SEASTAR_THREAD_TEST_CASE(test_update_node) {
         set_abort_on_internal_error(true);
     });
 
-    topo.add_or_update_endpoint(std::nullopt, id1, endpoint_dc_rack::default_location, node::state::normal);
+    topo.add_or_update_endpoint(id1, std::nullopt, endpoint_dc_rack::default_location, node::state::normal);
 
     auto node = topo.this_node();
     auto mutable_node = const_cast<locator::node*>(node);

diff --git a/test/boost/network_topology_strategy_test.cc b/test/boost/network_topology_strategy_test.cc
@@ -949,7 +949,7 @@ SEASTAR_THREAD_TEST_CASE(test_topology_tracks_local_node) {
 
     stm.mutate_token_metadata([&] (token_metadata& tm) -> future<> {
         co_await tm.clear_gently();
-        tm.get_topology().add_or_update_endpoint(ip1, host1, ip1_dc_rack_v2, node::state::being_decommissioned);
+        tm.get_topology().add_or_update_endpoint(host1, ip1, ip1_dc_rack_v2, node::state::being_decommissioned);
     }).get();
 
     n1 = stm.get()->get_topology().find_node(host1);

diff --git a/test/boost/tablets_test.cc b/test/boost/tablets_test.cc
@@ -434,7 +434,7 @@ SEASTAR_TEST_CASE(test_sharder) {
         auto table1 = table_id(utils::UUID_gen::get_time_UUID());
 
         token_metadata tokm(token_metadata::config{ .topo_cfg{ .this_host_id = h1 } });
-        tokm.get_topology().add_or_update_endpoint(utils::fb_utilities::get_broadcast_address(), h1);
+        tokm.get_topology().add_or_update_endpoint(h1, utils::fb_utilities::get_broadcast_address());
 
         std::vector<tablet_id> tablet_ids;
         {

diff --git a/test/lib/cql_test_env.cc b/test/lib/cql_test_env.cc
@@ -634,8 +634,8 @@ class single_node_cql_env : public cql_test_env {
             locator::shared_token_metadata::mutate_on_all_shards(_token_metadata, [hostid = host_id] (locator::token_metadata& tm) {
                 auto& topo = tm.get_topology();
                 topo.set_host_id_cfg(hostid);
-                topo.add_or_update_endpoint(utils::fb_utilities::get_broadcast_address(),
-                                                hostid,
+                topo.add_or_update_endpoint(hostid,
+                                                utils::fb_utilities::get_broadcast_address(),
                                                 std::nullopt,
                                                 locator::node::state::normal,
                                                 smp::count);