From c81173e320b5ef78b9804d56640215b2440a5e59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20M=C3=A4kel=C3=A4?= Date: Sat, 8 Sep 2018 17:32:59 +0300 Subject: [PATCH] Move C++ code out of C headers The additions into the server.h header used C++ language which caused C programs to fail to compile. Moved the implementation of the EMAverage class into the private Server class in the server.hh header and exposed it via functions in the server.h header. Also temporarily moved almost_equal_server_scores into the public server.hh as there is no service.hh header. --- include/maxscale/server.h | 7 ++--- include/maxscale/server.hh | 14 ++++++++++ include/maxscale/service.h | 12 -------- server/core/config.cc | 6 ---- server/core/internal/server.hh | 25 ++++++++++++++++- server/core/server.cc | 28 ++++++++++++++----- .../routing/readconnroute/readconnroute.cc | 8 +++--- .../routing/readwritesplit/readwritesplit.hh | 8 +++--- .../readwritesplit/rwsplit_select_backends.cc | 27 +++++++++--------- .../routing/readwritesplit/rwsplitsession.cc | 2 +- 10 files changed, 85 insertions(+), 52 deletions(-) diff --git a/include/maxscale/server.h b/include/maxscale/server.h index 0a6b6ea65..4791ecaf6 100644 --- a/include/maxscale/server.h +++ b/include/maxscale/server.h @@ -20,7 +20,6 @@ #include #include -#include #include #include @@ -166,9 +165,6 @@ typedef struct server * by rwsplit. TODO: Move to rwsplit */ bool warn_ssl_not_enabled;/**< SSL not used for an SSL enabled server */ MxsDiskSpaceThreshold* disk_space_threshold;/**< Disk space thresholds */ - // TODO, this is a plain ptr to a C++ class. Soonish, when the server is new/deleted - // this will become a std::unique ptr. But not in this commit. - maxbase::EMAverage* response_time; /**< for calculating average response time */ } SERVER; /** @@ -528,4 +524,7 @@ extern void dprintServer(DCB*, const SERVER*); extern void dprintPersistentDCBs(DCB*, const SERVER*); extern void dListServers(DCB*); +int server_response_time_num_samples(const SERVER* server); +double server_response_time_average(const SERVER* server); + MXS_END_DECLS diff --git a/include/maxscale/server.hh b/include/maxscale/server.hh index 8b4483d63..0ab7807ae 100644 --- a/include/maxscale/server.hh +++ b/include/maxscale/server.hh @@ -15,10 +15,24 @@ #include #include +#include + #include namespace maxscale { bool server_set_status(SERVER* server, int bit, std::string* errmsg_out = NULL); bool server_clear_status(SERVER* server, int bit, std::string* errmsg_out = NULL); + +/** Returns true if the two server "scores" are within 1/(see code) of each other. + * The epsilon needs tweaking, and might even need to be in config. This + * function is important for some compares, where one server might be only + * marginally better than others, in which case historical data could determine + * the outcome. + */ +inline bool almost_equal_server_scores(double lhs, double rhs) +{ + constexpr double div = 100; // within 1% of each other. + return std::abs((long)(lhs - rhs)) < std::abs((long)std::max(lhs, rhs)) * (1 / div); +} } diff --git a/include/maxscale/service.h b/include/maxscale/service.h index 72b643bc6..1eaec5e90 100644 --- a/include/maxscale/service.h +++ b/include/maxscale/service.h @@ -68,18 +68,6 @@ typedef struct server_ref_t bool active; /**< Whether this reference is valid and in use*/ } SERVER_REF; -/** Returns true if the two server "scores" are within 1/(see code) of each other. - * The epsilon needs tweaking, and might even need to be in config. This - * function is important for some compares, where one server might be only - * marginally better than others, in which case historical data could determine - * the outcome. - */ -inline bool almost_equal_server_scores(double lhs, double rhs) -{ - constexpr double div = 100; // within 1% of each other. - return std::abs(lhs - rhs) < std::abs(std::max(lhs, rhs)) * (1 / div); -} - /** Macro to check whether a SERVER_REF is active */ #define SERVER_REF_IS_ACTIVE(ref) (ref->active && server_is_active(ref->server)) diff --git a/server/core/config.cc b/server/core/config.cc index 7af1d32c5..c3df8f642 100644 --- a/server/core/config.cc +++ b/server/core/config.cc @@ -3743,12 +3743,6 @@ int create_new_server(CONFIG_CONTEXT* obj) error = true; } } - - // nantti, TODO. Decide whether to expose some of this in config, or if the values - // can be calculated at runtime. The "500" or sample_max affects how often a - // session should updates this stat. sample_max should be slightly lower than max sample - // rate (which is less than qps due to the noise filter). - server->response_time = new( std::nothrow) maxbase::EMAverage(0.04, 0.35, 500); } else { diff --git a/server/core/internal/server.hh b/server/core/internal/server.hh index 0eacd3cb3..c0f98f3a8 100644 --- a/server/core/internal/server.hh +++ b/server/core/internal/server.hh @@ -16,6 +16,7 @@ * Internal header for the server type */ +#include #include #include @@ -24,7 +25,29 @@ std::unique_ptr serverGetList(); // Private server implementation class Server : public SERVER { - // TODO: Move everything here +public: + + int response_time_num_samples() const + { + return response_time.num_samples(); + } + + double response_time_average() const + { + return response_time.average(); + } + + void response_time_add(double ave, int num_samples) + { + response_time.add(ave, num_samples); + } + +private: + // nantti, TODO. Decide whether to expose some of this in config, or if the values + // can be calculated at runtime. The "500" or sample_max affects how often a + // session should updates this stat. sample_max should be slightly lower than max sample + // rate (which is less than qps due to the noise filter). + maxbase::EMAverage response_time = {0.04, 0.35, 500}; }; void server_free(Server* server); diff --git a/server/core/server.cc b/server/core/server.cc index 527fe374f..1701aab94 100644 --- a/server/core/server.cc +++ b/server/core/server.cc @@ -238,7 +238,6 @@ void server_free(Server* server) } delete server->disk_space_threshold; - delete server->response_time; delete server; } @@ -504,13 +503,15 @@ static void cleanup_persistent_connections(const SERVER* server) * Designed to be called within a debugger session in order * to display all active servers within the gateway */ -void dprintServer(DCB* dcb, const SERVER* server) +void dprintServer(DCB* dcb, const SERVER* srv) { - if (!server_is_active(server)) + if (!server_is_active(srv)) { return; } + const Server* server = static_cast(srv); + dcb_printf(dcb, "Server %p (%s)\n", server, server->name); dcb_printf(dcb, "\tServer: %s\n", server->address); char* stat = server_status(server); @@ -562,9 +563,9 @@ void dprintServer(DCB* dcb, const SERVER* server) dcb_printf(dcb, "\tCurrent no. of operations: %d\n", server->stats.n_current_ops); dcb_printf(dcb, "\tNumber of routed packets: %lu\n", server->stats.packets); std::ostringstream ave_os; - if (server->response_time->num_samples()) + if (server_response_time_num_samples(server)) { - maxbase::Duration dur(server->response_time->average()); + maxbase::Duration dur(server_response_time_average(server)); ave_os << dur; } else @@ -1541,8 +1542,21 @@ namespace std::mutex add_response_mutex; } -void server_add_response_average(SERVER* server, double ave, int num_samples) +void server_add_response_average(SERVER* srv, double ave, int num_samples) { std::lock_guard lock(add_response_mutex); - server->response_time->add(ave, num_samples); + Server* server = static_cast(srv); + server->response_time_add(ave, num_samples); +} + +int server_response_time_num_samples(const SERVER* srv) +{ + const Server* server = static_cast(srv); + return server->response_time_num_samples(); +} + +double server_response_time_average(const SERVER* srv) +{ + const Server* server = static_cast(srv); + return server->response_time_average(); } diff --git a/server/modules/routing/readconnroute/readconnroute.cc b/server/modules/routing/readconnroute/readconnroute.cc index ab5926533..98897fe72 100644 --- a/server/modules/routing/readconnroute/readconnroute.cc +++ b/server/modules/routing/readconnroute/readconnroute.cc @@ -79,7 +79,7 @@ #include #include #include -#include +#include #include #include #include @@ -378,9 +378,9 @@ static MXS_ROUTER_SESSION* newSession(MXS_ROUTER* instance, MXS_SESSION* session /* ref has a better score. */ candidate = ref; } - else if (almost_equal_server_scores(ref->inv_weight * ref->connections, - candidate->inv_weight * candidate->connections) - && ref->server->stats.n_connections < candidate->server->stats.n_connections) + else if (mxs::almost_equal_server_scores(ref->inv_weight * ref->connections, + candidate->inv_weight * candidate->connections) && + ref->server->stats.n_connections < candidate->server->stats.n_connections) { /* The servers are about equally good, but ref has had fewer connections over time. * TODO: On second thought, if the servers are currently about equally good, diff --git a/server/modules/routing/readwritesplit/readwritesplit.hh b/server/modules/routing/readwritesplit/readwritesplit.hh index f111d428f..f4788510e 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.hh +++ b/server/modules/routing/readwritesplit/readwritesplit.hh @@ -134,7 +134,7 @@ static const char gtid_wait_stmt[] */ using SRWBackendVector = std::vector; using BackendSelectFunction = std::function - ; + ; BackendSelectFunction get_backend_select_function(select_criteria_t); struct Config @@ -385,9 +385,9 @@ std::pair get_slave_counts(mxs::SRWBackendList& backends, mxs::SRWBack * * @return Valid iterator into argument backends, or end(backends) if empty */ -SRWBackendVector::const_iterator find_best_backend(const SRWBackendVector& backends, - BackendSelectFunction select, - bool masters_accepts_reads); +SRWBackendVector::iterator find_best_backend(SRWBackendVector& backends, + BackendSelectFunction select, + bool masters_accepts_reads); /* * The following are implemented in rwsplit_tmp_table_multi.c diff --git a/server/modules/routing/readwritesplit/rwsplit_select_backends.cc b/server/modules/routing/readwritesplit/rwsplit_select_backends.cc index 392145f35..a510a8bac 100644 --- a/server/modules/routing/readwritesplit/rwsplit_select_backends.cc +++ b/server/modules/routing/readwritesplit/rwsplit_select_backends.cc @@ -57,8 +57,8 @@ static bool valid_for_slave(const SRWBackend& backend, const SRWBackend& master) && (!master || backend != master); } -SRWBackendVector::const_iterator best_score(const SRWBackendVector& sBackends, - std::function server_score) +SRWBackendVector::iterator best_score(SRWBackendVector& sBackends, + std::function server_score) { double min {std::numeric_limits::max()}; auto best = sBackends.end(); @@ -76,7 +76,7 @@ SRWBackendVector::const_iterator best_score(const SRWBackendVector& sBackends, } /** Compare number of connections from this router in backend servers */ -SRWBackendVector::const_iterator backend_cmp_router_conn(const SRWBackendVector& sBackends) +SRWBackendVector::iterator backend_cmp_router_conn(SRWBackendVector& sBackends) { static auto server_score = [](SERVER_REF* server) { return server->inv_weight * server->connections; @@ -86,7 +86,7 @@ SRWBackendVector::const_iterator backend_cmp_router_conn(const SRWBackendVector& } /** Compare number of global connections in backend servers */ -SRWBackendVector::const_iterator backend_cmp_global_conn(const SRWBackendVector& sBackends) +SRWBackendVector::iterator backend_cmp_global_conn(SRWBackendVector& sBackends) { static auto server_score = [](SERVER_REF* server) { return server->inv_weight * server->server->stats.n_current; @@ -96,7 +96,7 @@ SRWBackendVector::const_iterator backend_cmp_global_conn(const SRWBackendVector& } /** Compare replication lag between backend servers */ -SRWBackendVector::const_iterator backend_cmp_behind_master(const SRWBackendVector& sBackends) +SRWBackendVector::iterator backend_cmp_behind_master(SRWBackendVector& sBackends) { static auto server_score = [](SERVER_REF* server) { return server->inv_weight * server->server->rlag; @@ -106,7 +106,7 @@ SRWBackendVector::const_iterator backend_cmp_behind_master(const SRWBackendVecto } /** Compare number of current operations in backend servers */ -SRWBackendVector::const_iterator backend_cmp_current_load(const SRWBackendVector& sBackends) +SRWBackendVector::iterator backend_cmp_current_load(SRWBackendVector& sBackends) { static auto server_score = [](SERVER_REF* server) { return server->inv_weight * server->server->stats.n_current_ops; @@ -115,7 +115,7 @@ SRWBackendVector::const_iterator backend_cmp_current_load(const SRWBackendVector return best_score(sBackends, server_score); } -SRWBackendVector::const_iterator backend_cmp_response_time(const SRWBackendVector& sBackends) +SRWBackendVector::iterator backend_cmp_response_time(SRWBackendVector& sBackends) { const int SZ = sBackends.size(); double slot[SZ]; @@ -125,8 +125,9 @@ SRWBackendVector::const_iterator backend_cmp_response_time(const SRWBackendVecto for (int i = 0; i < SZ; ++i) { SERVER_REF* server = (**sBackends[i]).backend(); - auto ave = server->server->response_time->average(); - if (ave == 0) + auto ave = server_response_time_average(server->server); + + if (ave==0) { constexpr double very_quick = 1.0 / 10000000; // arbitrary very short duration (0.1 // microseconds) @@ -197,9 +198,9 @@ BackendSelectFunction get_backend_select_function(select_criteria_t sc) * * @return iterator to the best slave or backends.end() if none found */ -SRWBackendVector::const_iterator find_best_backend(const SRWBackendVector& backends, - BackendSelectFunction select, - bool masters_accepts_reads) +SRWBackendVector::iterator find_best_backend(SRWBackendVector& backends, + BackendSelectFunction select, + bool masters_accepts_reads) { // Group backends by priority. The set of highest priority backends will then compete. std::map priority_map; @@ -288,7 +289,7 @@ static void log_server_connections(select_criteria_t criteria, const SRWBackendL case LOWEST_RESPONSE_TIME: { - maxbase::Duration response_ave(b->server->response_time->average()); + maxbase::Duration response_ave(server_response_time_average(b->server)); std::ostringstream os; os << response_ave; MXS_INFO("Average response time : %s from \t[%s]:%d %s", diff --git a/server/modules/routing/readwritesplit/rwsplitsession.cc b/server/modules/routing/readwritesplit/rwsplitsession.cc index 7e5591951..a0f1f5be4 100644 --- a/server/modules/routing/readwritesplit/rwsplitsession.cc +++ b/server/modules/routing/readwritesplit/rwsplitsession.cc @@ -648,7 +648,7 @@ void RWSplitSession::clientReply(GWBUF* writebuf, DCB* backend_dcb) ResponseStat& stat = backend->response_stat(); stat.query_ended(); if (stat.is_valid() && (stat.sync_time_reached() - || backend->server()->response_time->num_samples() == 0)) + || server_response_time_num_samples(backend->server()) == 0)) { server_add_response_average(backend->server(), stat.average().secs(),