From f323551ebb239f5c7b4b8be162da3922e027cac9 Mon Sep 17 00:00:00 2001 From: Glauber Costa Date: Thu, 23 Jul 2015 10:25:25 -0400 Subject: [PATCH 1/4] thrift_server: make destructor out of line The destructor depends on a lot of things (like the thrift lib classes), that are not visible from the .hh. It works fine so far because nobody is trying to destroy it explicitly either. But soon I will. Signed-off-by: Glauber Costa --- thrift/server.cc | 3 +++ thrift/server.hh | 1 + 2 files changed, 4 insertions(+) diff --git a/thrift/server.cc b/thrift/server.cc index 0188673c93..cf882c4113 100644 --- a/thrift/server.cc +++ b/thrift/server.cc @@ -43,6 +43,9 @@ thrift_server::thrift_server(distributed& db) , _processor_factory(new CassandraAsyncProcessorFactory(_handler_factory)) { } +thrift_server::~thrift_server() { +} + struct handler_deleter { CassandraCobSvIfFactory* hf; void operator()(CassandraCobSvIf* h) const { diff --git a/thrift/server.hh b/thrift/server.hh index 0a4c68da28..259acd8788 100644 --- a/thrift/server.hh +++ b/thrift/server.hh @@ -46,6 +46,7 @@ class thrift_server { uint64_t _requests_served = 0; public: thrift_server(distributed& db); + ~thrift_server(); future<> listen(ipv4_addr addr); void do_accepts(int which); class connection; From 60cdfa20c15beb4216171d3ab8ee5d7215eb3993 Mon Sep 17 00:00:00 2001 From: Glauber Costa Date: Thu, 23 Jul 2015 10:26:48 -0400 Subject: [PATCH 2/4] thrift_server: define and call a stop method All sharded services "should" define a stop method. Calling them is also a good practice. Blindly calling it at exit is wrong, but it is less wrong than not calling it at all, and makes it now equally as wrong as any of the other services. Signed-off-by: Glauber Costa --- main.cc | 3 +++ thrift/server.cc | 6 ++++++ thrift/server.hh | 1 + 3 files changed, 10 insertions(+) diff --git a/main.cc b/main.cc index 664b572c13..b709a824f1 100644 --- a/main.cc +++ b/main.cc @@ -146,6 +146,9 @@ int main(int ac, char** av) { }); auto tserver = new distributed; tserver->start(std::ref(db)).then([server = std::move(tserver), thrift_port, rpc_address] () mutable { + engine().at_exit([server] { + return server->stop(); + }); server->invoke_on_all(&thrift_server::listen, ipv4_addr{rpc_address, thrift_port}); }).then([thrift_port] { std::cout << "Thrift server listening on port " << thrift_port << " ...\n"; diff --git a/thrift/server.cc b/thrift/server.cc index cf882c4113..3d137aeb5f 100644 --- a/thrift/server.cc +++ b/thrift/server.cc @@ -46,6 +46,12 @@ thrift_server::thrift_server(distributed& db) thrift_server::~thrift_server() { } +// FIXME: this is here because we must have a stop function. But we should actually +// do something useful - or be sure it is not needed +future<> thrift_server::stop() { + return make_ready_future<>(); +} + struct handler_deleter { CassandraCobSvIfFactory* hf; void operator()(CassandraCobSvIf* h) const { diff --git a/thrift/server.hh b/thrift/server.hh index 259acd8788..2586017c96 100644 --- a/thrift/server.hh +++ b/thrift/server.hh @@ -48,6 +48,7 @@ public: thrift_server(distributed& db); ~thrift_server(); future<> listen(ipv4_addr addr); + future<> stop(); void do_accepts(int which); class connection; uint64_t total_connections() const; From b488b06fa4adca3bcf9a943541330e64c242684c Mon Sep 17 00:00:00 2001 From: Glauber Costa Date: Thu, 23 Jul 2015 10:28:17 -0400 Subject: [PATCH 3/4] cql_server: define and call a stop method All sharded services "should" define a stop method. Calling them is also a good practice. Blindly calling it at exit is wrong, but it is less wrong than not calling it at all, and makes it now equally as wrong as any of the other services. Signed-off-by: Glauber Costa --- main.cc | 3 +++ transport/server.cc | 6 ++++++ transport/server.hh | 1 + 3 files changed, 10 insertions(+) diff --git a/main.cc b/main.cc index b709a824f1..1c78434426 100644 --- a/main.cc +++ b/main.cc @@ -140,6 +140,9 @@ int main(int ac, char** av) { auto rpc_address = e.addresses[0].in.s_addr; auto cserver = new distributed; cserver->start(std::ref(proxy), std::ref(qp)).then([server = std::move(cserver), cql_port, rpc_address] () mutable { + engine().at_exit([server] { + return server->stop(); + }); server->invoke_on_all(&cql_server::listen, ipv4_addr{rpc_address, cql_port}); }).then([cql_port] { std::cout << "CQL server listening on port " << cql_port << " ...\n"; diff --git a/transport/server.cc b/transport/server.cc index 08d55d876a..069047f6cb 100644 --- a/transport/server.cc +++ b/transport/server.cc @@ -241,6 +241,12 @@ cql_server::cql_server(distributed& proxy, distributed cql_server::stop() { + return make_ready_future<>(); +} + future<> cql_server::listen(ipv4_addr addr) { listen_options lo; diff --git a/transport/server.hh b/transport/server.hh index da7255b962..a373bbf0dc 100644 --- a/transport/server.hh +++ b/transport/server.hh @@ -20,6 +20,7 @@ public: cql_server(distributed& proxy, distributed& qp); future<> listen(ipv4_addr addr); void do_accepts(int which); + future<> stop(); private: class fmt_visitor; class connection; From 4cd143de87f44a5be2436471cf03deeecafbad89 Mon Sep 17 00:00:00 2001 From: Glauber Costa Date: Thu, 23 Jul 2015 10:28:42 -0400 Subject: [PATCH 4/4] filter_tracker: define and call a stop method All sharded services "should" define a stop method. Calling them is also a good practice. For this one specifically, though, we will not call stop. We miss a good way to add a Deleter to a shared_ptr class, and that would be the only reliable way to tie into its lifetime. Signed-off-by: Glauber Costa --- sstables/filter.cc | 2 ++ sstables/filter.hh | 2 ++ 2 files changed, 4 insertions(+) diff --git a/sstables/filter.cc b/sstables/filter.cc index 182d12490f..e750ffca20 100644 --- a/sstables/filter.cc +++ b/sstables/filter.cc @@ -16,6 +16,8 @@ namespace sstables { future<> sstable::read_filter() { auto ft = _filter_tracker; return _filter_tracker->start(std::move(ft)).then([this] { + // FIXME: should stop this service. This one is definitely wrong to stop at_exit. + // We should use a Deleter class in lw_shared_ptr if (!has_component(sstable::component_type::Filter)) { _filter = std::make_unique(); return make_ready_future<>(); diff --git a/sstables/filter.hh b/sstables/filter.hh index 8b6c952d4e..5f4db86f7e 100644 --- a/sstables/filter.hh +++ b/sstables/filter.hh @@ -36,6 +36,8 @@ class filter_tracker { public: filter_tracker(lw_shared_ptr>&& ptr) : _ptr(std::move(ptr)) {} + future<> stop() { return make_ready_future<>(); } + void add_false_positive() { false_positive++; }