From 926ce145dbc0a715c47461ad9dae3c321dcafc6f Mon Sep 17 00:00:00 2001 From: Vlad Zolotarov Date: Mon, 2 Nov 2015 13:25:44 +0200 Subject: [PATCH 1/5] locator::i_endpoint_snitch_base: move _gossip_started to the base class Move the member and add an access method. This is needed in order to be able to access this state using snitch_ptr handle. This also allows to get rid of ec2_multi_region_snitch::_helper_added member since it duplicates _gossip_started semantics. Signed-off-by: Vlad Zolotarov --- locator/ec2_multi_region_snitch.cc | 4 ++-- locator/ec2_multi_region_snitch.hh | 1 - locator/gossiping_property_file_snitch.hh | 1 - locator/snitch_base.hh | 13 +++++++++---- 4 files changed, 11 insertions(+), 8 deletions(-) diff --git a/locator/ec2_multi_region_snitch.cc b/locator/ec2_multi_region_snitch.cc index 8db909520b..53db54db78 100644 --- a/locator/ec2_multi_region_snitch.cc +++ b/locator/ec2_multi_region_snitch.cc @@ -109,9 +109,9 @@ future<> ec2_multi_region_snitch::gossiper_starting() { return g.add_local_application_state(application_state::INTERNAL_IP, ss.value_factory.internal_ip(_local_private_address)).then([this] { - if (!_helper_added) { + if (!_gossip_started) { gms::get_local_gossiper().register_(make_shared(_my_dc)); - _helper_added = true; + _gossip_started = true; } }); diff --git a/locator/ec2_multi_region_snitch.hh b/locator/ec2_multi_region_snitch.hh index 180d9a3c59..ab48c2f38a 100644 --- a/locator/ec2_multi_region_snitch.hh +++ b/locator/ec2_multi_region_snitch.hh @@ -51,6 +51,5 @@ private: static constexpr const char* PUBLIC_IP_QUERY_REQ = "/latest/meta-data/public-ipv4"; static constexpr const char* PRIVATE_IP_QUERY_REQ = "/latest/meta-data/local-ipv4"; sstring _local_private_address; - bool _helper_added = false; }; } // namespace locator diff --git a/locator/gossiping_property_file_snitch.hh b/locator/gossiping_property_file_snitch.hh index ba34246313..15e273eaf8 100644 --- a/locator/gossiping_property_file_snitch.hh +++ b/locator/gossiping_property_file_snitch.hh @@ -124,7 +124,6 @@ private: timer<> _file_reader; std::experimental::optional _last_file_mod; std::istringstream _istrm; - bool _gossip_started = false; bool _prefer_local = false; bool _file_reader_runs = false; unsigned _file_reader_cpu_id; diff --git a/locator/snitch_base.hh b/locator/snitch_base.hh index 1a59bebf70..661872749a 100644 --- a/locator/snitch_base.hh +++ b/locator/snitch_base.hh @@ -102,7 +102,10 @@ public: * called after Gossiper instance exists immediately before it starts * gossiping */ - virtual future<> gossiper_starting() = 0; + virtual future<> gossiper_starting() { + _gossip_started = true; + return make_ready_future<>(); + } /** * Returns whether for a range query doing a query against merged is likely @@ -165,6 +168,10 @@ public: //noop by default } + bool local_gossiper_started() { + return _gossip_started; + } + protected: static logging::logger& logger() { static logging::logger snitch_logger("snitch_logger"); @@ -185,6 +192,7 @@ protected: stopping, stopped } _state = snitch_state::initializing; + bool _gossip_started = false; }; struct snitch_ptr { @@ -406,9 +414,6 @@ public: virtual int compare_endpoints( inet_address& address, inet_address& a1, inet_address& a2) override; - // noop by default - virtual future<> gossiper_starting() override { return make_ready_future<>(); } - virtual bool is_worth_merging_for_range_query( std::vector& merged, std::vector& l1, From 5042f3c9520400e78da24221605983b3e368cbf1 Mon Sep 17 00:00:00 2001 From: Vlad Zolotarov Date: Mon, 2 Nov 2015 10:06:10 +0200 Subject: [PATCH 2/5] locator::i_endpoint_snitch_base: make reload_gossiper_state() a virtual function Make reload_gossiper_state() be a virtual method of a base class in order to allow calling it using a snitch_ptr handle. A base class already has a ton of virtual methods so no harm is done performance-wise. Using virtual methods instead of doing dynamic_cast results in a much cleaner code however. Signed-off-by: Vlad Zolotarov --- locator/gossiping_property_file_snitch.hh | 30 ++++++++++++++++------- locator/snitch_base.hh | 4 +++ 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/locator/gossiping_property_file_snitch.hh b/locator/gossiping_property_file_snitch.hh index 15e273eaf8..4848a7e7da 100644 --- a/locator/gossiping_property_file_snitch.hh +++ b/locator/gossiping_property_file_snitch.hh @@ -79,6 +79,27 @@ public: const sstring& fname = "", unsigned io_cpuid = 0); + /** + * This function register a Gossiper subscriber to reconnect according to + * the new "prefer_local" value, namely use either an internal or extenal IP + * address. + * + * @note Currently in order to be backward compatible we are mimicking the C* + * behavior, which is a bit strange: while allowing the change of + * prefer_local value during the same run it won't actually trigger + * disconnect from all remote nodes as would be logical (in order to + * connect using a new configuration). On the contrary, if the new + * prefer_local value is TRUE, it will trigger the reconnect only when + * there is a corresponding gossip event (e.g. on_change()) from the + * corresponding node has been accepted. If the new value is FALSE + * then it won't trigger disconnect at all! And in any case a remote + * node will be reconnected using the PREFERED_IP value stored in the + * system_table.peer. + * + * This is currently relevant to EC2/GCE(?) only. + */ + virtual void reload_gossiper_state() override; + private: void periodic_reader_callback(); @@ -103,15 +124,6 @@ private: */ future<> read_property_file(); - /** - * TODO: this function is expected to trigger a Gossiper to reconnect - * according to the new "prefer_local" value, namely use either an internal - * or extenal IP address. - * - * This is currently relevant to EC2/GCE(?) only. - */ - void reload_gossiper_state(); - /** * Indicate that the snitch has stopped its I/O. */ diff --git a/locator/snitch_base.hh b/locator/snitch_base.hh index 661872749a..19f577b73f 100644 --- a/locator/snitch_base.hh +++ b/locator/snitch_base.hh @@ -172,6 +172,10 @@ public: return _gossip_started; } + virtual void reload_gossiper_state() { + // noop by default + } + protected: static logging::logger& logger() { static logging::logger snitch_logger("snitch_logger"); From 5da4e62a591e3e20a24d436e75a24f9768cee54a Mon Sep 17 00:00:00 2001 From: Vlad Zolotarov Date: Mon, 2 Nov 2015 11:09:32 +0200 Subject: [PATCH 3/5] locator::i_endpoint_snitch: align the _prefer_local parameter with _my_dc and _my_rack Adjust the interface and distribution of prefer_local parameter read from a snitch property file with the rest of similar parameters (e.g. dc and rack): they are read and their values are distributed (copied) across all shards' instances. Signed-off-by: Vlad Zolotarov --- locator/gossiping_property_file_snitch.cc | 1 + locator/gossiping_property_file_snitch.hh | 1 - locator/production_snitch_base.hh | 4 ++++ locator/snitch_base.hh | 2 ++ 4 files changed, 7 insertions(+), 1 deletion(-) diff --git a/locator/gossiping_property_file_snitch.cc b/locator/gossiping_property_file_snitch.cc index 12cd27f0c7..0e4a46a817 100644 --- a/locator/gossiping_property_file_snitch.cc +++ b/locator/gossiping_property_file_snitch.cc @@ -214,6 +214,7 @@ future<> gossiping_property_file_snitch::reload_configuration() { if (engine().cpu_id() != _file_reader_cpu_id) { local_s->set_my_dc(_my_dc); local_s->set_my_rack(_my_rack); + local_s->set_prefer_local(_prefer_local); } }).then([this] { return seastar::async([this] { diff --git a/locator/gossiping_property_file_snitch.hh b/locator/gossiping_property_file_snitch.hh index 4848a7e7da..27bab69970 100644 --- a/locator/gossiping_property_file_snitch.hh +++ b/locator/gossiping_property_file_snitch.hh @@ -136,7 +136,6 @@ private: timer<> _file_reader; std::experimental::optional _last_file_mod; std::istringstream _istrm; - bool _prefer_local = false; bool _file_reader_runs = false; unsigned _file_reader_cpu_id; shared_ptr _reconnectable_helper; diff --git a/locator/production_snitch_base.hh b/locator/production_snitch_base.hh index a9d02700c1..ded70dfd38 100644 --- a/locator/production_snitch_base.hh +++ b/locator/production_snitch_base.hh @@ -163,6 +163,10 @@ private: _my_rack = new_rack; } + virtual void set_prefer_local(bool prefer_local) override { + _prefer_local = prefer_local; + } + void parse_property_file(); protected: diff --git a/locator/snitch_base.hh b/locator/snitch_base.hh index 19f577b73f..35145ea453 100644 --- a/locator/snitch_base.hh +++ b/locator/snitch_base.hh @@ -145,6 +145,7 @@ public: // noop by default virtual void set_my_dc(const sstring& new_dc) {}; virtual void set_my_rack(const sstring& new_rack) {}; + virtual void set_prefer_local(bool prefer_local) {}; virtual void set_local_private_addr(const sstring& addr_str) {}; static distributed& snitch_instance() { @@ -429,6 +430,7 @@ private: protected: sstring _my_dc; sstring _my_rack; + bool _prefer_local = false; }; } // namespace locator From 689d5fb000c3f444afbd64d78ba6a82f8a57bf1f Mon Sep 17 00:00:00 2001 From: Vlad Zolotarov Date: Mon, 2 Nov 2015 10:13:59 +0200 Subject: [PATCH 4/5] locator::gossiping_property_file_snitch: fix in reload_configuration() When we access a gossiper instance we use a _gossip_started state of a snitch, which is set in a gossiper_starting() method. gossiper_starting() method however is invoked by a gossiper on CPU0 only therefore the _gossip_started snitch state will be set for an instance on CPU0 only. Therefore instead of synchronizing the _gossip_started state between all shards we just have to make sure we check it on the right CPU, which is CPU0. This patch fixes this issue. Signed-off-by: Vlad Zolotarov --- locator/gossiping_property_file_snitch.cc | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/locator/gossiping_property_file_snitch.cc b/locator/gossiping_property_file_snitch.cc index 0e4a46a817..c9ffe8d814 100644 --- a/locator/gossiping_property_file_snitch.cc +++ b/locator/gossiping_property_file_snitch.cc @@ -219,8 +219,9 @@ future<> gossiping_property_file_snitch::reload_configuration() { }).then([this] { return seastar::async([this] { // reload Gossiper state (executed on CPU0 only) - smp::submit_to(0, [this] { - this->reload_gossiper_state(); + smp::submit_to(0, [] { + auto& local_snitch_ptr = get_local_snitch_ptr(); + local_snitch_ptr->reload_gossiper_state(); }).get(); // update Storage Service on each shard @@ -240,8 +241,9 @@ future<> gossiping_property_file_snitch::reload_configuration() { // spread the word... - smp::submit_to(0, [this] { - if (this->_gossip_started && service::get_storage_service().local_is_initialized()) { + smp::submit_to(0, [] { + auto& local_snitch_ptr = get_local_snitch_ptr(); + if (local_snitch_ptr->local_gossiper_started() && service::get_storage_service().local_is_initialized()) { service::get_local_storage_service().gossip_snitch_info(); } }).get(); From b654d942b4a30a496bbfa88e876975cdfa65308d Mon Sep 17 00:00:00 2001 From: Vlad Zolotarov Date: Mon, 2 Nov 2015 13:37:51 +0200 Subject: [PATCH 5/5] locator::gossiping_property_file_snitch: don't ignore a returned future Don't ignore yet another returned future in reload_configuration(). Since commit 5e8037b50a91f3134bfef5aee126705cf29a6928 storage_service::gossip_snitch_info() returns a future. This patch takes this into an account. Signed-off-by: Vlad Zolotarov --- locator/gossiping_property_file_snitch.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/locator/gossiping_property_file_snitch.cc b/locator/gossiping_property_file_snitch.cc index c9ffe8d814..45a37e080a 100644 --- a/locator/gossiping_property_file_snitch.cc +++ b/locator/gossiping_property_file_snitch.cc @@ -244,8 +244,10 @@ future<> gossiping_property_file_snitch::reload_configuration() { smp::submit_to(0, [] { auto& local_snitch_ptr = get_local_snitch_ptr(); if (local_snitch_ptr->local_gossiper_started() && service::get_storage_service().local_is_initialized()) { - service::get_local_storage_service().gossip_snitch_info(); + return service::get_local_storage_service().gossip_snitch_info(); } + + return make_ready_future<>(); }).get(); }); });