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.

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},
+    {"named", HINT_ROUTE_TO_NAMED_SERVER},
-    {"route_to_all", HINT_ROUTE_TO_ALL},
+    {"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<HintRouter, HintRouterSession>(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()

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&);

server/modules/routing/hintrouter/hintrouterdefs.hh

@@ -17,8 +17,12 @@
 #include <maxscale/dcb.h>
 #include <maxscale/log_manager.h>
 
+#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__)

server/modules/routing/hintrouter/hintroutersession.cc

@@ -20,6 +20,39 @@
 #include <maxscale/alloc.h>
 #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<HintRouterSession::MapElement, bool>
+{
+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<HintRouterSession::MapElement, bool>
-{
-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<const Dcb, bool>
-{
-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<const char*>(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);
+            success = route_by_hint(pPacket, current_hint, false);
-            current_hint = current_hint->next;
+            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 = {};
+        HINT default_hint = {};
-        fake_hint.type = m_router->get_default_action();
+        default_hint.type = m_router->get_default_action();
-        if (fake_hint.type == HINT_ROUTE_TO_NAMED_SERVER)
+        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);
+        success = route_by_hint(pPacket, &default_hint, true);
-        if (fake_hint.type == HINT_ROUTE_TO_NAMED_SERVER)
+        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

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
-
 };