Hi, Andrei!
Thanks for your review!
Andrei Elkin wrote:
> Hello, Ramil.
>
> The patch is like "clean the Augean stables" :)
indeed :)
> @@ -1975,79 +2068,36 @@ connected:
> Register ourselves with the master.
> */
> thd->proc_info = "Registering slave on master";
> - if (register_slave_on_master(mysql, mi))
> + if (register_slave_on_master(mysql, mi, &suppress_warnings))
> {
> sql_print_error("Slave I/O thread couldn't register on master");
> - goto err;
> + if (check_io_slave_killed(thd, mi, "Slave I/O thread killed while \
> +registering slave on master") ||
> + try_to_reconnect(thd, mysql, mi, &retry_count, suppress_warnings,
> + reconnect_messages[0]))
> + goto err;
> + goto connected;
> }
> }
>
> Appeared to be a tricky part, but thanks to your explanation there is
> the try_to_reconnect-goto-connected loop which forces
> re-connecting,re-registerering if the failure is
> ER_NET_READ_INTERRUPTED.
Actually, no.
We always force reconnecting if the register_slave_on_master() failed.
ER_NET_READ_INTERRUPTED check is used only to suppress reconnect warnings.
> @@ -2056,12 +2106,9 @@ after reconnect");
> */
> thd->proc_info= "Waiting for master to send event";
> event_len= read_event(mysql, mi, &suppress_warnings);
> - if (io_slave_killed(thd,mi))
> - {
> - if (global_system_variables.log_warnings)
> - sql_print_information("Slave I/O thread killed while reading event");
> + if (check_io_slave_killed(thd, mi, "Slave I/O thread killed while \
> +reading event"))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> goto err;
> - }
>
> if (event_len == packet_error)
> {
> @@ -2081,39 +2128,9 @@ max_allowed_packet",
> mysql_error(mysql));
> goto err;
> }
> - mi->slave_running= MYSQL_SLAVE_RUN_NOT_CONNECT;
> - thd->proc_info = "Waiting to reconnect after a failed master event
> read";
> -#ifdef SIGNAL_WITH_VIO_CLOSE
> - thd->clear_active_vio();
> -#endif
> - end_server(mysql);
> - if (retry_count++)
> - {
> - if (retry_count > master_retry_count)
> - goto err; // Don't retry forever
> -
> safe_sleep(thd,mi->connect_retry,(CHECK_KILLED_FUNC)io_slave_killed,
> - (void*) mi);
> - }
> - if (io_slave_killed(thd,mi))
> - {
> - if (global_system_variables.log_warnings)
> - sql_print_information("Slave I/O thread killed while waiting to \
> -reconnect after a failed read");
> - goto err;
> - }
> - thd->proc_info = "Reconnecting after a failed master event read";
> - if (!suppress_warnings)
> - sql_print_information("Slave I/O thread: Failed reading log event, \
> -reconnecting to retry, log '%s' position %s", IO_RPL_LOG_NAME,
> - llstr(mi->master_log_pos, llbuff));
> - if (safe_reconnect(thd, mysql, mi, suppress_warnings) ||
> - io_slave_killed(thd,mi))
> - {
> - if (global_system_variables.log_warnings)
> - sql_print_information("Slave I/O thread killed during or after a \
> -reconnect done to recover from failed read");
>
> The check "if (io_slave_killed...)" is removed by the previous
> hunk. I'd rather to preserve it to make the following "if" clause
>
> + if (try_to_reconnect(thd, mysql, mi, &retry_count,
> suppress_warnings,
> + reconnect_messages[2]))
> goto err;
>
> compatible with the two other checks where
>
> + if (check_io_slave_killed(thd, mi, "Slave I/O thread killed while \
> +requesting master dump") || try_to_reconnect
>
> I know there is checking on killed inside of try_to_reconnect, still
> it's better to keep the logic precisely and be compatible .
We have the check right after the read_event() call. It is not removed but
replaced with the check_io_slave_killed() call (see underscored by ^^^^^^^^^^ line).
Regards,
Ramil.
