MXS-1703 Cleanup do_show_slave_status()

Slave_IO_Running is now properly parsed. Renamed class to SlaveStatus.

server/modules/monitor/mariadbmon/cluster_manipulation.cc

@@ -413,7 +413,7 @@ bool MariaDBMonitor::server_is_rejoin_suspect(MariaDBServer* rejoin_cand, json_t
     bool is_suspect = false;
     if (rejoin_cand->is_running() && !rejoin_cand->is_master())
     {
-        SlaveStatusInfo* slave_status = &rejoin_cand->slave_status;
+        SlaveStatus* slave_status = &rejoin_cand->slave_status;
         // Has no slave connection, yet is not a master.
         if (rejoin_cand->n_slaves_configured == 0)
         {
@@ -423,12 +423,14 @@ bool MariaDBMonitor::server_is_rejoin_suspect(MariaDBServer* rejoin_cand, json_t
         else if (rejoin_cand->n_slaves_configured == 1)
         {
             // which is connected to master but it's the wrong one
-            if (slave_status->slave_io_running  && slave_status->master_server_id != m_master->server_id)
+            if (slave_status->slave_io_running == SlaveStatus::SLAVE_IO_YES  &&
+                slave_status->master_server_id != m_master->server_id)
             {
                 is_suspect = true;
             }
             // or is disconnected but master host or port is wrong.
-            else if (!slave_status->slave_io_running && slave_status->slave_sql_running &&
+            else if (slave_status->slave_io_running == SlaveStatus::SLAVE_IO_CONNECTING &&
+                     slave_status->slave_sql_running &&
                      (slave_status->master_host != m_master->server_base->server->name ||
                       slave_status->master_port != m_master->server_base->server->port))
             {
@@ -1436,7 +1438,7 @@ bool MariaDBMonitor::slave_receiving_events()
         MariaDBServer* info = get_server_info(server);
 
         if (info->slave_configured &&
-            info->slave_status.slave_io_running &&
+            info->slave_status.slave_io_running == SlaveStatus::SLAVE_IO_YES &&
             info->slave_status.master_server_id == master_id &&
             difftime(time(NULL), info->latest_event) < m_master_failure_timeout)
         {

server/modules/monitor/mariadbmon/mariadbserver.cc

@@ -21,10 +21,18 @@
 
 using std::string;
 
-SlaveStatusInfo::SlaveStatusInfo()
+namespace
+{
+    // Used for Slave_IO_Running
+    const char YES[] = "Yes";
+    const char CONNECTING[] = "Connecting";
+    const char NO[] = "No";
+}
+
+SlaveStatus::SlaveStatus()
     : master_server_id(SERVER_ID_UNKNOWN)
     , master_port(0)
-    , slave_io_running(false)
+    , slave_io_running(SLAVE_IO_NO)
     , slave_sql_running(false)
     , read_master_log_pos(0)
 {}
@@ -75,80 +83,93 @@ std::auto_ptr<QueryResult> MariaDBServer::execute_query(const string& query)
 
 bool MariaDBServer::do_show_slave_status()
 {
-    /** Column positions for SHOW SLAVE STATUS */
+    unsigned int columns = 0;
-    const size_t MYSQL55_STATUS_MASTER_LOG_POS = 5;
+    string query;
-    const size_t MYSQL55_STATUS_MASTER_LOG_FILE = 6;
+    switch (version)
-    const size_t MYSQL55_STATUS_IO_RUNNING = 10;
-    const size_t MYSQL55_STATUS_SQL_RUNNING = 11;
-    const size_t MYSQL55_STATUS_MASTER_ID = 39;
-
-    unsigned int columns;
-    int i_slave_io_running, i_slave_sql_running, i_read_master_log_pos, i_master_server_id, i_master_log_file;
-    int i_last_io_errno, i_last_io_error, i_last_sql_error, i_slave_rec_hbs, i_slave_hb_period;
-    int i_master_host, i_master_port, i_using_gtid, i_gtid_io_pos;
-    const char *query;
-    mysql_server_version server_version = version;
-
-    if (server_version == MYSQL_SERVER_VERSION_100)
     {
-        columns = 42;
+        case MYSQL_SERVER_VERSION_100:
-        query = "SHOW ALL SLAVES STATUS";
+            columns = 42;
+            query = "SHOW ALL SLAVES STATUS";
+            break;
+        case MYSQL_SERVER_VERSION_55:
+            columns = 40;
+            query = "SHOW SLAVE STATUS";
+            break;
+        case MYSQL_SERVER_VERSION_51: // TODO: Remove support?
+            columns = 38;
+            query = "SHOW SLAVE STATUS";
+            break;
+        default:
+            ss_dassert(!true);
     }
-    else
+
+    auto result = execute_query(query);
+    if (result.get() == NULL)
     {
-        columns = server_version == MYSQL_SERVER_VERSION_55 ? 40 : 38;
+        MXS_ERROR("'%s' did not return data.", query.c_str());
-        query = "SHOW SLAVE STATUS";
+        return false;
-        i_slave_io_running = MYSQL55_STATUS_IO_RUNNING;
+    }
-        i_slave_sql_running = MYSQL55_STATUS_SQL_RUNNING;
+    else if(result->get_column_count() < columns)
-        i_master_log_file = MYSQL55_STATUS_MASTER_LOG_FILE;
+    {
-        i_read_master_log_pos = MYSQL55_STATUS_MASTER_LOG_POS;
+        MXS_ERROR("'%s' returned less than the expected amount of columns. Expected %u columns, "
-        i_master_server_id = MYSQL55_STATUS_MASTER_ID;
+                  "got %" PRId64 ".", query.c_str(), columns, result->get_column_count());
+        return false;
+    }
+
+    // Fields common to all server versions
+    auto i_master_host = result->get_col_index("Master_Host");
+    auto i_master_port = result->get_col_index("Master_Port");
+    auto i_slave_io_running = result->get_col_index("Slave_IO_Running");
+    auto i_slave_sql_running = result->get_col_index("Slave_SQL_Running");
+    auto i_master_server_id = result->get_col_index("Master_Server_Id");
+    auto i_master_log_file = result->get_col_index("Master_Log_File");
+    auto i_read_master_log_pos = result->get_col_index("Read_Master_Log_Pos");
+    auto i_last_io_errno = result->get_col_index("Last_IO_Errno");
+    auto i_last_io_error = result->get_col_index("Last_IO_Error");
+    auto i_last_sql_error = result->get_col_index("Last_SQL_Error");
+
+    const char INVALID_DATA[] = "'%s' returned invalid data.";
+    if (i_master_host < 0 || i_master_port < 0 || i_slave_io_running < 0 || i_slave_sql_running < 0 ||
+        i_master_log_file < 0 || i_read_master_log_pos < 0 || i_master_server_id < 0 ||
+        i_last_io_errno < 0  || i_last_io_error < 0 || i_last_sql_error < 0)
+    {
+        MXS_ERROR(INVALID_DATA, query.c_str());
+        return false;
+    }
+
+    int64_t i_slave_rec_hbs = -1, i_slave_hb_period = -1;
+    int64_t i_using_gtid = -1, i_gtid_io_pos = -1;
+    if (version == MYSQL_SERVER_VERSION_100)
+    {
+        i_slave_rec_hbs = result->get_col_index("Slave_received_heartbeats");
+        i_slave_hb_period = result->get_col_index("Slave_heartbeat_period");
+        i_using_gtid = result->get_col_index("Using_Gtid");
+        i_gtid_io_pos = result->get_col_index("Gtid_IO_Pos");
+        if (i_slave_rec_hbs < 0 || i_slave_hb_period < 0 || i_using_gtid < 0 || i_gtid_io_pos < 0)
+        {
+            MXS_ERROR(INVALID_DATA, query.c_str());
+            return false;
+        }
     }
 
     int64_t master_server_id = SERVER_ID_UNKNOWN;
     int nconfigured = 0;
     int nrunning = 0;
-
-    auto result = execute_query(query);
-    if (result.get() == NULL)
-    {
-        return false;
-    }
-    else if(result->get_column_count() < columns)
-    {
-        MXS_ERROR("\"%s\" returned less than the expected amount of columns. "
-                  "Expected %u columns.", query, columns);
-        return false;
-    }
-
-    if (server_version == MYSQL_SERVER_VERSION_100)
-    {
-        i_slave_io_running = result->get_col_index("Slave_IO_Running");
-        i_slave_sql_running = result->get_col_index("Slave_SQL_Running");
-        i_master_log_file = result->get_col_index("Master_Log_File");
-        i_read_master_log_pos = result->get_col_index("Read_Master_Log_Pos");
-        i_master_server_id = result->get_col_index("Master_Server_Id");
-        i_slave_rec_hbs = result->get_col_index("Slave_received_heartbeats");
-        i_slave_hb_period = result->get_col_index("Slave_heartbeat_period");
-        i_master_host = result->get_col_index("Master_Host");
-        i_master_port = result->get_col_index("Master_Port");
-        i_using_gtid = result->get_col_index("Using_Gtid");
-        i_gtid_io_pos = result->get_col_index("Gtid_IO_Pos");
-        i_last_io_errno = result->get_col_index("Last_IO_Errno");
-        i_last_io_error = result->get_col_index("Last_IO_Error");
-        i_last_sql_error = result->get_col_index("Last_SQL_Error");
-    }
-    // TODO: Add other versions here once it's certain the column names are the same. Better yet, save the
-    // indexes to object data so they don't need to be updated every query.
-
     while (result->next_row())
     {
         nconfigured++;
+        slave_status.master_host = result->get_string(i_master_host);
+        slave_status.master_port = result->get_uint(i_master_port);
+        string last_io_error = result->get_string(i_last_io_error);
+        string last_sql_error = result->get_string(i_last_sql_error);
+        slave_status.last_error = !last_io_error.empty() ? last_io_error : last_sql_error;
+
         /* get Slave_IO_Running and Slave_SQL_Running values*/
-        slave_status.slave_io_running = (result->get_string(i_slave_io_running) == "Yes");
+        slave_status.slave_io_running =
+            SlaveStatus::slave_io_from_string(result->get_string(i_slave_io_running));
         slave_status.slave_sql_running = (result->get_string(i_slave_sql_running) == "Yes");
 
-        if (slave_status.slave_io_running && slave_status.slave_sql_running)
+        if (slave_status.slave_io_running == SlaveStatus::SLAVE_IO_YES && slave_status.slave_sql_running)
         {
             if (nrunning == 0)
             {
@@ -177,8 +198,7 @@ bool MariaDBServer::do_show_slave_status()
         int io_errno = last_io_errno;
         const int connection_errno = 2003;
 
-        if ((io_errno == 0 || io_errno == connection_errno) &&
+        if ((io_errno == 0 || io_errno == connection_errno) && version != MYSQL_SERVER_VERSION_51)
-            server_version != MYSQL_SERVER_VERSION_51)
         {
             /* Get Master_Server_Id */
             auto parsed = result->get_uint(i_master_server_id);
@@ -188,15 +208,8 @@ bool MariaDBServer::do_show_slave_status()
             }
         }
 
-        if (server_version == MYSQL_SERVER_VERSION_100)
+        if (version == MYSQL_SERVER_VERSION_100)
         {
-            slave_status.master_host = result->get_string(i_master_host);
-            slave_status.master_port = result->get_uint(i_master_port);
-
-            string last_io_error = result->get_string(i_last_io_error);
-            string last_sql_error = result->get_string(i_last_sql_error);
-            slave_status.last_error = !last_io_error.empty() ? last_io_error : last_sql_error;
-
             int heartbeats = result->get_uint(i_slave_rec_hbs);
             if (slave_heartbeats < heartbeats)
             {
@@ -227,7 +240,7 @@ bool MariaDBServer::do_show_slave_status()
         /** Query returned no rows, replication is not configured */
         slave_configured = false;
         slave_heartbeats = 0;
-        slave_status = SlaveStatusInfo();
+        slave_status = SlaveStatus();
     }
 
     slave_status.master_server_id = master_server_id;
@@ -441,7 +454,8 @@ string MariaDBServer::diagnostics(bool multimaster) const
     ss << "Slave configured:       " << (slave_configured ? "YES" : "NO") << "\n";
     if (slave_configured)
     {
-        ss << "Slave IO running:       " << (slave_status.slave_io_running ? "YES" : "NO") << "\n";
+        ss << "Slave IO running:       " <<
+            SlaveStatus::slave_io_to_string(slave_status.slave_io_running) << "\n";
         ss << "Slave SQL running:      " << (slave_status.slave_sql_running ? "YES" : "NO") << "\n";
         ss << "Master ID:              " << slave_status.master_server_id << "\n";
         ss << "Master binlog file:     " << slave_status.master_log_file << "\n";
@@ -475,7 +489,8 @@ json_t* MariaDBServer::diagnostics_json(bool multimaster) const
 
     json_object_set_new(srv, "read_only", json_boolean(read_only));
     json_object_set_new(srv, "slave_configured", json_boolean(slave_configured));
-    json_object_set_new(srv, "slave_io_running", json_boolean(slave_status.slave_io_running));
+    json_object_set_new(srv, "slave_io_running",
+        json_string(SlaveStatus::slave_io_to_string(slave_status.slave_io_running).c_str()));
     json_object_set_new(srv, "slave_sql_running", json_boolean(slave_status.slave_sql_running));
 
     json_object_set_new(srv, "master_binlog_file", json_string(slave_status.master_log_file.c_str()));
@@ -689,6 +704,44 @@ bool MariaDBServer::run_sql_from_file(const string& path, json_t** error_out)
     return !error;
 }
 
+SlaveStatus::slave_io_running_t SlaveStatus::slave_io_from_string(const std::string& str)
+{
+    slave_io_running_t rval = SLAVE_IO_NO;
+    if (str == YES)
+    {
+        rval = SLAVE_IO_YES;
+    }
+    else if (str == CONNECTING)
+    {
+        rval = SLAVE_IO_CONNECTING;
+    }
+    else if (str !=  NO)
+    {
+        MXS_ERROR("Unexpected value for Slave_IO_Running: '%s'.", str.c_str());
+    }
+    return rval;
+}
+
+string SlaveStatus::slave_io_to_string(SlaveStatus::slave_io_running_t slave_io)
+{
+    string rval;
+    switch (slave_io)
+    {
+        case SlaveStatus::SLAVE_IO_YES:
+            rval = YES;
+            break;
+        case SlaveStatus::SLAVE_IO_CONNECTING:
+            rval = CONNECTING;
+            break;
+        case SlaveStatus::SLAVE_IO_NO:
+            rval = NO;
+            break;
+        default:
+            ss_dassert(!false);
+    }
+    return rval;
+}
+
 QueryResult::QueryResult(MYSQL_RES* resultset)
     : m_resultset(resultset)
     , m_columns(-1)

server/modules/monitor/mariadbmon/mariadbserver.hh

@@ -34,15 +34,22 @@ enum print_repl_warnings_t
 class QueryResult;
 
 // Contains data returned by one row of SHOW ALL SLAVES STATUS
-class SlaveStatusInfo
+class SlaveStatus
 {
 public:
+    enum slave_io_running_t
+    {
+        SLAVE_IO_YES,
+        SLAVE_IO_CONNECTING,
+        SLAVE_IO_NO,
+    };
+
     int64_t master_server_id;       /**< The master's server_id value. Valid ids are 32bit unsigned. -1 is
                                      *   unread/error. */
     std::string master_host;        /**< Master server host name. */
     int master_port;                /**< Master server port. */
-    bool slave_io_running;          /**< Whether the slave I/O thread is running and connected. */
+    slave_io_running_t slave_io_running; /**< Slave I/O thread running state: "Yes", "Connecting" or "No" */
-    bool slave_sql_running;         /**< Whether or not the SQL thread is running. */
+    bool slave_sql_running;         /**< Slave SQL thread running state, true if "Yes" */
     std::string master_log_file;    /**< Name of the master binary log file that the I/O thread is currently
                                      *   reading from. */
     uint64_t read_master_log_pos;   /**< Position up to which the I/O thread has read in the current master
@@ -50,7 +57,9 @@ public:
     GtidList gtid_io_pos;           /**< Gtid I/O position of the slave thread. */
     std::string last_error;         /**< Last IO or SQL error encountered. */
 
-    SlaveStatusInfo();
+    SlaveStatus();
+    static slave_io_running_t slave_io_from_string(const std::string& str);
+    static std::string slave_io_to_string(slave_io_running_t slave_io);
 };
 
 // This class groups some miscellaneous replication related settings together.
@@ -93,7 +102,7 @@ public:
                                               *  new non-replicated events. */
     GtidList        gtid_current_pos;       /**< Gtid of latest event. */
     GtidList        gtid_binlog_pos;        /**< Gtid of latest event written to binlog. */
-    SlaveStatusInfo slave_status;           /**< Data returned from SHOW SLAVE STATUS */
+    SlaveStatus     slave_status;           /**< Data returned from SHOW SLAVE STATUS */
     ReplicationSettings rpl_settings;       /**< Miscellaneous replication related settings */
 
     MariaDBServer(MXS_MONITORED_SERVER* monitored_server);