From 3777da96bd36ea7352eca489754b9db396bc8c1f Mon Sep 17 00:00:00 2001 From: Esa Korhonen Date: Mon, 20 Aug 2018 18:41:34 +0300 Subject: [PATCH] Miscellaneous cleanup Removes needless status assignments and unused code. Moves and modifies some comments. --- .../mariadbmon/cluster_manipulation.cc | 20 ------ .../modules/monitor/mariadbmon/mariadbmon.cc | 41 ++++++------ .../modules/monitor/mariadbmon/mariadbmon.hh | 3 +- .../monitor/mariadbmon/mariadbserver.cc | 64 +++++-------------- .../monitor/mariadbmon/mariadbserver.hh | 20 +++--- 5 files changed, 48 insertions(+), 100 deletions(-) diff --git a/server/modules/monitor/mariadbmon/cluster_manipulation.cc b/server/modules/monitor/mariadbmon/cluster_manipulation.cc index 9ff0f4219..4b917d028 100644 --- a/server/modules/monitor/mariadbmon/cluster_manipulation.cc +++ b/server/modules/monitor/mariadbmon/cluster_manipulation.cc @@ -896,26 +896,6 @@ bool MariaDBMonitor::wait_cluster_stabilization(MariaDBServer* new_master, const return rval; } -/** - * Check that the given slave is a valid promotion candidate. - * - * @param preferred Preferred new master - * @param err_out Json object for error printing. Can be NULL. - * @return True, if given slave is a valid promotion candidate. - */ -bool MariaDBMonitor::switchover_check_preferred_master(MariaDBServer* preferred, json_t** err_out) -{ - mxb_assert(preferred); - bool rval = true; - if (!preferred->update_slave_info() || !preferred->check_replication_settings()) - { - PRINT_MXS_JSON_ERROR(err_out, "The requested server '%s' is not a valid promotion candidate.", - preferred->name()); - rval = false; - } - return rval; -} - /** * Prepares a server for the replication master role. * diff --git a/server/modules/monitor/mariadbmon/mariadbmon.cc b/server/modules/monitor/mariadbmon/mariadbmon.cc index 3958cbcc7..7a97612c4 100644 --- a/server/modules/monitor/mariadbmon/mariadbmon.cc +++ b/server/modules/monitor/mariadbmon/mariadbmon.cc @@ -334,33 +334,34 @@ json_t* MariaDBMonitor::diagnostics_to_json() const * * @param server The server to update */ -void MariaDBMonitor::update_server(MariaDBServer& server) +void MariaDBMonitor::update_server(MariaDBServer* server) { - MXS_MONITORED_SERVER* mon_srv = server.m_server_base; + MXS_MONITORED_SERVER* mon_srv = server->m_server_base; mxs_connect_result_t conn_status = mon_ping_or_connect_to_db(m_monitor, mon_srv); MYSQL* conn = mon_srv->con; // mon_ping_or_connect_to_db() may have reallocated the MYSQL struct. if (mon_connection_is_ok(conn_status)) { - server.set_status(SERVER_RUNNING); + server->set_status(SERVER_RUNNING); if (conn_status == MONITOR_CONN_NEWCONN_OK) { // Is a new connection or a reconnection. Check server version. - server.update_server_version(); + server->update_server_version(); } - if (server.m_version == MariaDBServer::version::MARIADB_MYSQL_55 || - server.m_version == MariaDBServer::version::MARIADB_100 || - server.m_version == MariaDBServer::version::BINLOG_ROUTER) + auto server_vrs = server->m_version; + if (server_vrs == MariaDBServer::version::MARIADB_MYSQL_55 || + server_vrs == MariaDBServer::version::MARIADB_100 || + server_vrs == MariaDBServer::version::BINLOG_ROUTER) { // Check permissions if permissions failed last time or if this is a new connection. - if (server.had_status(SERVER_AUTH_ERROR) || conn_status == MONITOR_CONN_NEWCONN_OK) + if (server->had_status(SERVER_AUTH_ERROR) || conn_status == MONITOR_CONN_NEWCONN_OK) { - server.check_permissions(); + server->check_permissions(); } // If permissions are ok, continue. - if (!server.has_status(SERVER_AUTH_ERROR)) + if (!server->has_status(SERVER_AUTH_ERROR)) { if (should_update_disk_space_status(mon_srv)) { @@ -368,7 +369,7 @@ void MariaDBMonitor::update_server(MariaDBServer& server) } // Query MariaDBServer specific data - server.monitor_server(); + server->monitor_server(); } } } @@ -376,24 +377,24 @@ void MariaDBMonitor::update_server(MariaDBServer& server) { /* The current server is not running. Clear all but the stale master bit as it is used to detect * masters that went down but came up. */ - server.clear_status(~SERVER_WAS_MASTER); + server->clear_status(~SERVER_WAS_MASTER); auto conn_errno = mysql_errno(conn); if (conn_errno == ER_ACCESS_DENIED_ERROR || conn_errno == ER_ACCESS_DENIED_NO_PASSWORD_ERROR) { - server.set_status(SERVER_AUTH_ERROR); + server->set_status(SERVER_AUTH_ERROR); } /* Log connect failure only once, that is, if server was RUNNING or MAINTENANCE during last * iteration. */ - if (mon_srv->mon_prev_status & (SERVER_RUNNING | SERVER_MAINT)) + if (server->had_status(SERVER_RUNNING) || server->had_status(SERVER_MAINT)) { mon_log_connect_error(mon_srv, conn_status); } } /** Increase or reset the error count of the server. */ - bool is_running = server.is_running(); - bool in_maintenance = server.is_in_maintenance(); + bool is_running = server->is_running(); + bool in_maintenance = server->is_in_maintenance(); mon_srv->mon_err_count = (is_running || in_maintenance) ? 0 : mon_srv->mon_err_count + 1; } @@ -434,10 +435,9 @@ void MariaDBMonitor::tick() } // Query all servers for their status. - for (auto iter = m_servers.begin(); iter != m_servers.end(); iter++) + for (MariaDBServer* server : m_servers) { - MariaDBServer* server = *iter; - update_server(*server); + update_server(server); if (server->m_topology_changed) { m_cluster_topology_changed = true; @@ -466,8 +466,7 @@ void MariaDBMonitor::tick() // Sanity check. Master may not be both slave and master. mxb_assert(m_master == NULL || !m_master->has_status(SERVER_SLAVE | SERVER_MASTER)); - // Update shared status. The next functions read the shared status. TODO: change the following - // functions to read "pending_status" instead. + // Update shared status. for (auto server : m_servers) { SERVER* srv = server->m_server_base->server; diff --git a/server/modules/monitor/mariadbmon/mariadbmon.hh b/server/modules/monitor/mariadbmon/mariadbmon.hh index 01f9b5f84..b88abf40d 100644 --- a/server/modules/monitor/mariadbmon/mariadbmon.hh +++ b/server/modules/monitor/mariadbmon/mariadbmon.hh @@ -192,7 +192,7 @@ private: json_t* diagnostics_to_json() const; // Cluster discovery and status assignment methods - void update_server(MariaDBServer& server); + void update_server(MariaDBServer* server); void find_graph_cycles(); void update_topology(); void log_master_changes(); @@ -218,7 +218,6 @@ private: MariaDBServer** new_master_out, MariaDBServer** current_master_out, json_t** error_out); bool do_switchover(MariaDBServer* demotion_target, MariaDBServer* promotion_target, json_t** error_out); - bool switchover_check_preferred_master(MariaDBServer* preferred, json_t** err_out); bool switchover_demote_master(MariaDBServer* current_master, json_t** err_out); bool switchover_wait_slaves_catchup(const ServerArray& slaves, const GtidList& gtid, int total_timeout, diff --git a/server/modules/monitor/mariadbmon/mariadbserver.cc b/server/modules/monitor/mariadbmon/mariadbserver.cc index c180d973b..e08dcd6b0 100644 --- a/server/modules/monitor/mariadbmon/mariadbserver.cc +++ b/server/modules/monitor/mariadbmon/mariadbserver.cc @@ -279,22 +279,18 @@ bool MariaDBServer::update_gtids(string* errmsg_out) return rval; } -bool MariaDBServer::update_replication_settings(std::string* error_out) +bool MariaDBServer::update_replication_settings(std::string* errmsg_out) { - static const string query = "SELECT @@gtid_strict_mode, @@log_bin, @@log_slave_updates;"; - string query_error; + const string query = "SELECT @@gtid_strict_mode, @@log_bin, @@log_slave_updates;"; bool rval = false; - auto result = execute_query(query, &query_error); + + auto result = execute_query(query, errmsg_out); if (result.get() != NULL && result->next_row()) { + rval = true; m_rpl_settings.gtid_strict_mode = result->get_bool(0); m_rpl_settings.log_bin = result->get_bool(1); m_rpl_settings.log_slave_updates = result->get_bool(2); - rval = true; - } - else if (error_out) - { - *error_out = query_error; } return rval; } @@ -582,13 +578,6 @@ bool MariaDBServer::uses_gtid(std::string* error_out) return using_gtid; } -bool MariaDBServer::update_slave_info() -{ - // TODO: fix for multisource repl - return (!m_slave_status.empty() && m_slave_status[0].slave_sql_running && update_replication_settings() && - update_gtids() && do_show_slave_status()); -} - bool MariaDBServer::can_replicate_from(MariaDBServer* master, string* error_out) { bool rval = false; @@ -782,9 +771,6 @@ bool MariaDBServer::run_sql_from_file(const string& path, json_t** error_out) return !error; } -/** - * Query this server. - */ void MariaDBServer::monitor_server() { string errmsg; @@ -818,7 +804,7 @@ void MariaDBServer::monitor_server() * ways. */ else if (!errmsg.empty() && m_print_update_errormsg) { - MXS_WARNING("Error during monitor update of server '%s'. %s.", name(), errmsg.c_str()); + MXS_WARNING("Error during monitor update of server '%s': %s", name(), errmsg.c_str()); m_print_update_errormsg = false; } return; @@ -832,19 +818,9 @@ void MariaDBServer::monitor_server() */ bool MariaDBServer::update_slave_status(string* errmsg_out) { - /** Clear old states */ - clear_status(SERVER_SLAVE | SERVER_MASTER | SERVER_RELAY | SERVER_SLAVE_OF_EXT_MASTER); - - bool rval = false; - if (do_show_slave_status(errmsg_out)) + bool rval = do_show_slave_status(errmsg_out); + if (rval) { - rval = true; - /* If all configured slaves are running set this node as slave */ - if (m_n_slaves_running > 0 && m_n_slaves_running == m_slave_status.size()) - { - set_status(SERVER_SLAVE); - } - /** Store master_id of current node. */ m_server_base->server->master_id = !m_slave_status.empty() ? m_slave_status[0].master_server_id : SERVER_ID_UNKNOWN; @@ -852,10 +828,6 @@ bool MariaDBServer::update_slave_status(string* errmsg_out) return rval; } -/** - * Update information which changes rarely. This method should be called after (re)connecting to a backend. - * Calling this every monitoring loop is overkill. - */ void MariaDBServer::update_server_version() { m_version = version::UNKNOWN; @@ -896,15 +868,14 @@ void MariaDBServer::update_server_version() } } -/** - * Checks monitor permissions on the server. Sets/clears the SERVER_AUTH_ERROR bit. - */ void MariaDBServer::check_permissions() { - MYSQL* conn = m_server_base->con; // Test with a typical query to make sure the monitor has sufficient permissions. - const char* query = "SHOW SLAVE STATUS;"; - if (mxs_mysql_query(conn, query) != 0) + const string query = "SHOW SLAVE STATUS;"; + string err_msg; + auto result = execute_query(query, &err_msg); + + if (result.get() == NULL) { /* In theory, this could be due to other errors as well, but that is quite unlikely since the * connection was just checked. The end result is in any case that the server is not updated, @@ -913,18 +884,13 @@ void MariaDBServer::check_permissions() // Only print error if last round was ok. if (!had_status(SERVER_AUTH_ERROR)) { - MXS_WARNING("Query '%s' to server '%s' failed when checking monitor permissions: '%s'. ", - query, name(), mysql_error(conn)); + MXS_WARNING("Error during monitor permissions test for server '%s': %s", + name(), err_msg.c_str()); } } else { clear_status(SERVER_AUTH_ERROR); - MYSQL_RES* result = mysql_store_result(conn); - if (result) - { - mysql_free_result(result); - } } } diff --git a/server/modules/monitor/mariadbmon/mariadbserver.hh b/server/modules/monitor/mariadbmon/mariadbserver.hh index b508bc8b4..511da7854 100644 --- a/server/modules/monitor/mariadbmon/mariadbserver.hh +++ b/server/modules/monitor/mariadbmon/mariadbserver.hh @@ -162,8 +162,20 @@ public: MariaDBServer(MXS_MONITORED_SERVER* monitored_server, int config_index); + /** + * Query this server. + */ void monitor_server(); + + /** + * Update information which changes rarely. This method should be called after (re)connecting to a backend. + * Calling this every monitoring loop is overkill. + */ void update_server_version(); + + /** + * Checks monitor permissions on the server. Sets/clears the SERVER_AUTH_ERROR bit. + */ void check_permissions(); /** @@ -356,14 +368,6 @@ public: */ bool uses_gtid(std::string* error_out = NULL); - /** - * Update replication settings, gtid:s and slave status of the server. - * - * @param server Slave to update - * @return True on success. False on error, or if server is not a slave (slave SQL not running). - */ - bool update_slave_info(); - /** * Checks if this server can replicate from master. Only considers gtid:s and only detects obvious errors. * The non-detected errors will mostly be detected once the slave tries to start replicating.