From 526ba7d706e722478ffb6e9b0a92c109fa04fb1b Mon Sep 17 00:00:00 2001 From: Esa Korhonen Date: Mon, 27 Mar 2017 17:16:09 +0300 Subject: [PATCH] HintRouter: cleanup + statistics for diagnosis entrypoint General code cleanup. Routing error detection now more robust. Remove some unused code. Debug messages now use "unique_name" when referring to servers. --- .../modules/routing/hintrouter/hintrouter.cc | 29 +- .../modules/routing/hintrouter/hintrouter.hh | 6 +- .../routing/hintrouter/hintrouterdefs.hh | 4 + .../routing/hintrouter/hintroutersession.cc | 476 +++++++++--------- .../routing/hintrouter/hintroutersession.hh | 13 +- 5 files changed, 267 insertions(+), 261 deletions(-) diff --git a/server/modules/routing/hintrouter/hintrouter.cc b/server/modules/routing/hintrouter/hintrouter.cc index 8cd4f6fe2..265d57cba 100644 --- a/server/modules/routing/hintrouter/hintrouter.cc +++ b/server/modules/routing/hintrouter/hintrouter.cc @@ -24,8 +24,8 @@ static const MXS_ENUM_VALUE default_action_values[] = { {"master", HINT_ROUTE_TO_MASTER}, {"slave", HINT_ROUTE_TO_SLAVE}, - {"named_server", HINT_ROUTE_TO_NAMED_SERVER}, - {"route_to_all", HINT_ROUTE_TO_ALL}, + {"named", HINT_ROUTE_TO_NAMED_SERVER}, + {"all", HINT_ROUTE_TO_ALL}, {NULL} /* Last must be NULL */ }; static const char DEFAULT_ACTION[] = "default_action"; @@ -35,6 +35,10 @@ static const char MAX_SLAVES[] = "max_slaves"; HintRouter::HintRouter(SERVICE* pService, HINT_TYPE default_action, string& default_server, int max_slaves) : maxscale::Router(pService), + m_routed_to_master(0), + m_routed_to_slave(0), + m_routed_to_named(0), + m_routed_to_all(0), m_default_action(default_action), m_default_server(default_server), m_max_slaves(max_slaves), @@ -42,7 +46,8 @@ HintRouter::HintRouter(SERVICE* pService, HINT_TYPE default_action, string& defa { HR_ENTRY(); if (m_max_slaves < 0) - { // set a reasonable default value + { + // set a reasonable default value m_max_slaves = pService->n_dbref - 1; } MXS_NOTICE("Hint router [%s] created.", pService->name); @@ -101,7 +106,7 @@ HintRouterSession* HintRouter::newSession(MXS_SESSION *pSession) if (master_ref) { // Connect to master - HR_DEBUG("Connecting to %s.", master_ref->server->name); + HR_DEBUG("Connecting to %s.", master_ref->server->unique_name); DCB* master_conn = dcb_connect(master_ref->server, pSession, master_ref->server->protocol); if (master_conn) @@ -138,7 +143,7 @@ HintRouterSession* HintRouter::newSession(MXS_SESSION *pSession) { SERVER_REF* slave_ref = slave_refs.at(current % size); // Connect to a slave - HR_DEBUG("Connecting to %s.", slave_ref->server->name); + HR_DEBUG("Connecting to %s.", slave_ref->server->unique_name); DCB* slave_conn = dcb_connect(slave_ref->server, pSession, slave_ref->server->protocol); if (slave_conn) @@ -170,6 +175,20 @@ HintRouterSession* HintRouter::newSession(MXS_SESSION *pSession) void HintRouter::diagnostics(DCB* pOut) { HR_ENTRY(); + for (int i = 0; default_action_values[i].name; i++) + { + if (default_action_values[i].enum_value == m_default_action) + { + dcb_printf(pOut, "\tDefault action: route to %s\n", default_action_values[i].name); + } + } + dcb_printf(pOut, "\tDefault server: %s\n", m_default_server.c_str()); + dcb_printf(pOut, "\tMaximum slave connections/session: %d\n", m_max_slaves); + dcb_printf(pOut, "\tTotal cumulative slave connections: %d\n", m_total_slave_conns); + dcb_printf(pOut, "\tQueries routed to master: %d\n", m_routed_to_master); + dcb_printf(pOut, "\tQueries routed to single slave: %d\n", m_routed_to_slave); + dcb_printf(pOut, "\tQueries routed to named server: %d\n", m_routed_to_named); + dcb_printf(pOut, "\tQueries routed to all servers: %d\n", m_routed_to_all); } uint64_t HintRouter::getCapabilities() diff --git a/server/modules/routing/hintrouter/hintrouter.hh b/server/modules/routing/hintrouter/hintrouter.hh index fe8f3c1e1..2048113ce 100644 --- a/server/modules/routing/hintrouter/hintrouter.hh +++ b/server/modules/routing/hintrouter/hintrouter.hh @@ -32,6 +32,11 @@ public: { return m_default_server; }; + /* Simple, approximate statistics */ + volatile unsigned int m_routed_to_master; + volatile unsigned int m_routed_to_slave; + volatile unsigned int m_routed_to_named; + volatile unsigned int m_routed_to_all; private: HintRouter(SERVICE* pService, HINT_TYPE default_action, string& default_server, int max_slaves); @@ -40,7 +45,6 @@ private: string m_default_server; int m_max_slaves; volatile int m_total_slave_conns; - private: HintRouter(const HintRouter&); HintRouter& operator = (const HintRouter&); diff --git a/server/modules/routing/hintrouter/hintrouterdefs.hh b/server/modules/routing/hintrouter/hintrouterdefs.hh index cdbef06ab..b435ad799 100644 --- a/server/modules/routing/hintrouter/hintrouterdefs.hh +++ b/server/modules/routing/hintrouter/hintrouterdefs.hh @@ -17,8 +17,12 @@ #include #include +#if defined(SS_DEBUG) #define DEBUG_HINTROUTER //#undef DEBUG_HINTROUTER +#else +#undef DEBUG_HINTROUTER +#endif #ifdef DEBUG_HINTROUTER #define HR_DEBUG(msg, ...) MXS_NOTICE(msg, ##__VA_ARGS__) diff --git a/server/modules/routing/hintrouter/hintroutersession.cc b/server/modules/routing/hintrouter/hintroutersession.cc index 72f9e28f9..18ad85af1 100644 --- a/server/modules/routing/hintrouter/hintroutersession.cc +++ b/server/modules/routing/hintrouter/hintroutersession.cc @@ -20,6 +20,39 @@ #include #include "hintrouter.hh" +namespace +{ +/** + * Writer is a function object that writes a clone of a provided GWBUF, + * to each dcb it is called with. + */ +class Writer : std::unary_function +{ +public: + Writer(GWBUF* pPacket) + : m_pPacket(pPacket) + {} + + bool operator()(HintRouterSession::MapElement& elem) + { + bool rv = false; + Dcb& dcb = elem.second; + GWBUF* pPacket = gwbuf_clone(m_pPacket); + + if (pPacket) + { + SERVER* pServer = dcb.server(); + HR_DEBUG("Writing packet to %p %s.", dcb.get(), pServer ? pServer->unique_name : "(null)"); + rv = dcb.write(pPacket); + } + return rv; + } + +private: + GWBUF* m_pPacket; +}; +} + HintRouterSession::HintRouterSession(MXS_SESSION* pSession, HintRouter* pRouter, const BackendMap& backends) @@ -49,243 +82,6 @@ void HintRouterSession::close() m_backends.clear(); } -namespace -{ - -/** - * Writer is a function object that writes a clone of a provided GWBUF, - * to each dcb it is called with. - */ -class Writer : std::unary_function -{ -public: - Writer(GWBUF* pPacket) - : m_pPacket(pPacket) - {} - - bool operator()(HintRouterSession::MapElement& elem) - { - bool rv = false; - Dcb& dcb = elem.second; - GWBUF* pPacket = gwbuf_clone(m_pPacket); - - if (pPacket) - { - SERVER* pServer = dcb.server(); - HR_DEBUG("Writing packet to %p %s.", dcb.get(), pServer ? pServer->name : "(null)"); - - rv = dcb.write(pPacket); - } - - return rv; - } - -private: - GWBUF* m_pPacket; -}; - -/** - * HintMatcher is a function object that when invoked with a dcb, checks - * whether the dcb matches the hint(s) that was given when the HintMatcher - * was created. - */ -class HintMatcher : std::unary_function -{ -public: - HintMatcher(HINT* pHint) - : m_pHint(pHint) - {} - - bool operator()(const Dcb& dcb) - { - bool match = false; - - SERVER* pServer = dcb.server(); - - if (pServer) - { - HINT* pHint = m_pHint; - - while (!match && pHint) - { - switch (pHint->type) - { - case HINT_ROUTE_TO_MASTER: - if (SERVER_IS_MASTER(pServer)) - { - match = true; - } - break; - - case HINT_ROUTE_TO_SLAVE: - if (SERVER_IS_SLAVE(pServer)) - { - match = true; - } - break; - - case HINT_ROUTE_TO_NAMED_SERVER: - { - const char* zName = static_cast(pHint->data); - - if (strcmp(zName, pServer->name) == 0) - { - match = true; - } - } - break; - - case HINT_ROUTE_TO_UPTODATE_SERVER: - case HINT_ROUTE_TO_ALL: - case HINT_PARAMETER: - MXS_ERROR("HINT not handled."); - ss_dassert(false); - break; - } - - pHint = pHint->next; - } - } - - return match; - } - -private: - HINT* m_pHint; -}; - -} - -bool HintRouterSession::route_to_slave(GWBUF* pPacket) -{ - bool success = false; - // Find a valid slave - size_type size = m_slaves.size(); - size_type begin = m_n_routed_to_slave % size; - size_type limit = begin + size; - for (size_type curr = begin; curr != limit; curr++) - { - Dcb& candidate = m_slaves.at(curr % size); - if (SERVER_IS_SLAVE(candidate.server())) - { - success = candidate.write(pPacket); - if (success) - { - break; - } - } - } - /* It is (in theory) possible, that none of the slaves in the slave-array are - * working (or have been promoted to master) and the previous master is now - * a slave. In this situation, re-arranging the dcb:s will help. */ - if (!success) - { - update_connections(); - for (size_type curr = begin; curr != limit; curr++) - { - Dcb& candidate = m_slaves.at(curr % size); - success = candidate.write(pPacket); - if (success) - { - break; - } - } - } - - if (success) - { - m_n_routed_to_slave++; - } - return success; -} - -bool HintRouterSession::route_by_hint(GWBUF* pPacket, HINT* hint, bool ignore_errors) -{ - bool success = false; - switch (hint->type) - { - case HINT_ROUTE_TO_MASTER: - if (m_master.get()) - { - // The master server should be already known, but may have changed - if (SERVER_IS_MASTER(m_master.server())) - { - success = m_master.write(pPacket); - } - else - { - update_connections(); - if (m_master.get()) - { - HR_DEBUG("Writing packet to %s.", m_master.server()->name); - success = m_master.write(pPacket); - } - } - } - else if (!ignore_errors) - { - MXS_ERROR("Hint suggests routing to master when no master connected."); - } - break; - - case HINT_ROUTE_TO_SLAVE: - if (m_slaves.size()) - { - success = route_to_slave(pPacket); - } - else if (!ignore_errors) - { - MXS_ERROR("Hint suggests routing to slave when no slave connected."); - } - break; - - case HINT_ROUTE_TO_NAMED_SERVER: - { - string backend_name((hint->data) ? (const char*)(hint->data) : ""); - BackendMap::const_iterator iter = m_backends.find(backend_name); - if (iter != m_backends.end()) - { - HR_DEBUG("Writing packet to %s.", iter->second.server()->unique_name); - success = iter->second.write(pPacket); - } - else if (!ignore_errors) - { - MXS_ERROR("Hint suggests routing to backend '%s' when no such backend connected.", - backend_name.c_str()); - } - } - break; - - case HINT_ROUTE_TO_ALL: - { - HR_DEBUG("Writing packet to %lu backends.", m_backends.size()); - size_type n_writes = - std::count_if(m_backends.begin(), m_backends.end(), Writer(pPacket)); - if (n_writes != 0) - { - m_surplus_replies = n_writes - 1; - } - else if (!ignore_errors) - { - MXS_ERROR("Nothing could be written, terminating session."); - } - - success = (n_writes == m_backends.size()); // Is this too strict? - if (success) - { - gwbuf_free(pPacket); - } - } - break; - - default: - MXS_ERROR("Unsupported hint type '%d'", hint->type); - break; - } - - return success; -} - int32_t HintRouterSession::routeQuery(GWBUF* pPacket) { HR_ENTRY(); @@ -300,26 +96,28 @@ int32_t HintRouterSession::routeQuery(GWBUF* pPacket) HR_DEBUG("Hint, looking for match."); while (!success && current_hint) { - success = route_by_hint(pPacket, current_hint, true); - current_hint = current_hint->next; + success = route_by_hint(pPacket, current_hint, false); + if (!success) + { + current_hint = current_hint->next; + } } } if (!success) { - // No hint => default action. HR_DEBUG("No hints or hint-based routing failed, falling back to default action."); - HINT fake_hint = {}; - fake_hint.type = m_router->get_default_action(); - if (fake_hint.type == HINT_ROUTE_TO_NAMED_SERVER) + HINT default_hint = {}; + default_hint.type = m_router->get_default_action(); + if (default_hint.type == HINT_ROUTE_TO_NAMED_SERVER) { - fake_hint.data = MXS_STRDUP(m_router->get_default_server().c_str()); + default_hint.data = MXS_STRDUP(m_router->get_default_server().c_str()); // Ignore allocation error, it will just result in an error later on } - success = route_by_hint(pPacket, &fake_hint, false); - if (fake_hint.type == HINT_ROUTE_TO_NAMED_SERVER) + success = route_by_hint(pPacket, &default_hint, true); + if (default_hint.type == HINT_ROUTE_TO_NAMED_SERVER) { - MXS_FREE(fake_hint.data); + MXS_FREE(default_hint.data); } } @@ -396,6 +194,190 @@ void HintRouterSession::handleError(GWBUF* pMessage, } } +bool HintRouterSession::route_by_hint(GWBUF* pPacket, HINT* hint, bool print_errors) +{ + bool success = false; + switch (hint->type) + { + case HINT_ROUTE_TO_MASTER: + { + bool master_ok = false; + // The master server should be already known, but may have changed + if (m_master.get() && SERVER_IS_MASTER(m_master.server())) + { + master_ok = true; + } + else + { + update_connections(); + if (m_master.get()) + { + master_ok = true; + } + } + + if (master_ok) + { + HR_DEBUG("Writing packet to master: '%s'.", m_master.server()->unique_name); + success = m_master.write(pPacket); + if (success) + { + m_router->m_routed_to_master++; + } + else + { + HR_DEBUG("Write to master failed."); + } + } + else if (print_errors) + { + MXS_ERROR("Hint suggests routing to master when no master connected."); + } + } + break; + + case HINT_ROUTE_TO_SLAVE: + success = route_to_slave(pPacket, print_errors); + break; + + case HINT_ROUTE_TO_NAMED_SERVER: + { + string backend_name((hint->data) ? (const char*)(hint->data) : ""); + BackendMap::const_iterator iter = m_backends.find(backend_name); + if (iter != m_backends.end()) + { + HR_DEBUG("Writing packet to %s.", iter->second.server()->unique_name); + success = iter->second.write(pPacket); + if (success) + { + m_router->m_routed_to_named++; + } + else + { + HR_DEBUG("Write failed."); + } + } + else if (print_errors) + { + /* This shouldn't be possible with current setup as server names are + * checked on startup. With a different filter and the 'print_errors' + * on for the first call this is possible. */ + MXS_ERROR("Hint suggests routing to backend '%s' when no such backend connected.", + backend_name.c_str()); + } + } + break; + + case HINT_ROUTE_TO_ALL: + { + HR_DEBUG("Writing packet to %lu backends.", m_backends.size()); + BackendMap::size_type n_writes = + std::count_if(m_backends.begin(), m_backends.end(), Writer(pPacket)); + if (n_writes != 0) + { + m_surplus_replies = n_writes - 1; + } + BackendMap::size_type size = m_backends.size(); + success = (n_writes == size); + if (success) + { + gwbuf_free(pPacket); + m_router->m_routed_to_all++; + } + else + { + HR_DEBUG("Write to all failed."); + if (print_errors) + { + MXS_ERROR("Write failed for '%lu' out of '%lu' backends.", + (size - n_writes), size); + } + } + } + break; + + default: + MXS_ERROR("Unsupported hint type '%d'", hint->type); + break; + } + return success; +} + +bool HintRouterSession::route_to_slave(GWBUF* pPacket, bool print_errors) +{ + bool success = false; + // Find a valid slave + size_type size = m_slaves.size(); + if (size) + { + size_type begin = m_n_routed_to_slave % size; + size_type limit = begin + size; + for (size_type curr = begin; curr != limit; curr++) + { + Dcb& candidate = m_slaves.at(curr % size); + if (SERVER_IS_SLAVE(candidate.server())) + { + HR_DEBUG("Writing packet to slave: '%s'.", candidate.server()->unique_name); + success = candidate.write(pPacket); + if (success) + { + break; + } + else + { + HR_DEBUG("Write to slave failed."); + } + } + } + } + + /* It is (in theory) possible, that none of the slaves in the slave-array are + * working (or have been promoted to master) and the previous master is now + * a slave. In this situation, re-arranging the dcb:s will help. */ + if (!success) + { + update_connections(); + size = m_slaves.size(); + if (size) + { + size_type begin = m_n_routed_to_slave % size; + size_type limit = begin + size; + for (size_type curr = begin; curr != limit; curr++) + { + Dcb& candidate = m_slaves.at(curr % size); + HR_DEBUG("Writing packet to slave: '%s'.", candidate.server()->unique_name); + success = candidate.write(pPacket); + if (success) + { + break; + } + else + { + HR_DEBUG("Write to slave failed."); + } + } + } + } + + if (success) + { + m_router->m_routed_to_slave++; + m_n_routed_to_slave++; + } + else if (print_errors) + { + if (!size) + { + MXS_ERROR("Hint suggests routing to slave when no slaves found."); + } + else + { + MXS_ERROR("Could not write to any of '%lu' slaves.", size); + } + } + return success; +} + void HintRouterSession::update_connections() { /* Attempt to rearrange the dcb:s in the session such that the master and diff --git a/server/modules/routing/hintrouter/hintroutersession.hh b/server/modules/routing/hintrouter/hintroutersession.hh index 674210f5a..003890470 100644 --- a/server/modules/routing/hintrouter/hintroutersession.hh +++ b/server/modules/routing/hintrouter/hintroutersession.hh @@ -45,10 +45,6 @@ public: void close(); int32_t routeQuery(GWBUF* pPacket); - bool route_by_hint(GWBUF* pPacket, HINT* current_hint, bool ignore_errors); - - bool route_to_slave(GWBUF* pPacket); - void clientReply(GWBUF* pPacket, DCB* pBackend); @@ -57,17 +53,18 @@ public: mxs_error_action_t action, bool* pSuccess); - void update_connections(); - private: HintRouterSession(const HintRouterSession&); // denied HintRouterSession& operator = (const HintRouterSession&); // denied private: + bool route_by_hint(GWBUF* pPacket, HINT* current_hint, bool ignore_errors); + bool route_to_slave(GWBUF* pPacket, bool print_errors); + void update_connections(); + HintRouter* m_router; BackendMap m_backends; // all connections Dcb m_master; // connection to master BackendArray m_slaves; // connections to slaves size_type m_n_routed_to_slave; // packets routed to a single slave, used for rr - size_t m_surplus_replies; // how many replies should be ignored - + size_type m_surplus_replies; // how many replies should be ignored };