Hi Daogang,
Nice work!
Please find my review comments below.
STATUS
------
Not Approved.
REQUIRED CHANGES
----------------
RC1. It will result another problem.
Because it just print the error if it is its first happening.
So retry times will always be 1 even through it retries so many times.
I think there are two solutions for this bug:
1. Print the error if it is its last happening.
and print err_count.
2. According to the bug report, It just print a wrong number at the
first connect. That is because the code that:
if ((int)mysql_errno(mysql) != last_errno).
when on its first connecting, last_errno is probably 0.
But fatal error never retry.
I prefer the first one.
REQUESTS
--------
SUGGESTIONS
-----------
DETAILS
-------
n/a
On Mon, 2010-09-20 at 06:36 +0000, Dao-Gang.Qu@stripped wrote:
> #At file:///home/daogangqu/mysql/bzrwork1/bugtest/mysql-next-mr-bugfixing/ based on
> revid:wlad@stripped
>
> 3273 Dao-Gang.Qu@stripped 2010-09-20
> Bug #56416 errors in first connection for IO thread are not retried
>
> SHOW SLAVE STATUS can not display the number of real retries
> for each connection attempt of I/O thread.
>
> To fix the problem to make SHOW SLAVE STATUS to display the
> number of retries which would be real retry counts instead of
> mi->retry_count(3600*24 by default) for each connection
> attempt of I/O thread.
> @ mysql-test/suite/rpl/r/rpl_change_master_dbug.result
> Updated for the patch of BUG #56416
> @ mysql-test/suite/rpl/t/rpl_change_master_dbug.test
> Updated test case to also verifies that the 'Last_IO_Error'
> entry of SHOW SLAVE STATUS will display the number of real
> retries.
> @ sql/rpl_slave.cc
> Added code to make SHOW SLAVE STATUS to display the number of
> real retries for each connection attempt of I/O thread.
>
> modified:
> mysql-test/suite/rpl/r/rpl_change_master_dbug.result
> mysql-test/suite/rpl/t/rpl_change_master_dbug.test
> sql/rpl_slave.cc
> === modified file 'mysql-test/suite/rpl/r/rpl_change_master_dbug.result'
> --- a/mysql-test/suite/rpl/r/rpl_change_master_dbug.result 2010-07-29 01:51:38 +0000
> +++ b/mysql-test/suite/rpl/r/rpl_change_master_dbug.result 2010-09-20 06:36:19 +0000
> @@ -5,11 +5,8 @@ reset slave;
> drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9;
> start slave;
> include/stop_slave.inc
> -SET @old_debug= @@global.debug;
> -SET GLOBAL debug="+d,connect_to_master_always_report_error";
> CHANGE MASTER TO master_retry_count=3, master_host='dummy', master_connect_retry=1;
> START SLAVE io_thread;
> -SET @@global.debug= @old_debug;
> stop slave;
> drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9;
> reset master;
>
> === modified file 'mysql-test/suite/rpl/t/rpl_change_master_dbug.test'
> --- a/mysql-test/suite/rpl/t/rpl_change_master_dbug.test 2010-07-29 01:51:38 +0000
> +++ b/mysql-test/suite/rpl/t/rpl_change_master_dbug.test 2010-09-20 06:36:19 +0000
> @@ -6,7 +6,9 @@
> #
> # BUG#44209: MASTER_CONNECT_RETRY and --master-retry-count disconnected from each
> other
> #
> -# Second part of the tests. Requires debug build.
> +# BUG#56416
> +# The test also verifies that the 'Last_IO_Error' entry of SHOW SLAVE STATUS
> +# will display the number of real retries
> #
>
> ## Checks that the slave actually gives up after retrying N times, where
> @@ -14,8 +16,6 @@
>
> -- connection slave
> -- source include/stop_slave.inc
> -SET @old_debug= @@global.debug;
> -SET GLOBAL debug="+d,connect_to_master_always_report_error";
> CHANGE MASTER TO master_retry_count=3, master_host='dummy', master_connect_retry=1;
> START SLAVE io_thread;
>
> @@ -32,7 +32,6 @@ if (!`SELECT "$error" LIKE "%retries: 3"
> -- echo Expected number of retries was: 3
> -- die
> }
> -SET @@global.debug= @old_debug;
>
> -- source include/master-slave-reset.inc
> -- source include/master-slave-end.inc
>
> === modified file 'sql/rpl_slave.cc'
> --- a/sql/rpl_slave.cc 2010-09-01 02:51:08 +0000
> +++ b/sql/rpl_slave.cc 2010-09-20 06:36:19 +0000
> @@ -4503,33 +4503,19 @@ static int connect_to_master(THD* thd, M
> mysql_real_connect(mysql, mi->host, mi->user, mi->password, 0,
> mi->port, 0, client_flag) == 0))
> {
> - /* Don't repeat last error */
> - if ((int)mysql_errno(mysql) != last_errno ||
> - DBUG_EVALUATE_IF("connect_to_master_always_report_error",
> - TRUE, FALSE))
> - {
> - /*
> - TODO: would be great that when issuing SHOW SLAVE STATUS
> - the number of retries would actually show err_count
> - instead of mi->retry_count.
> -
> - We can achieve that if we remove the 'if' above and
> - replace mi->retry_count with err_count. However, we
> - would be adding an entry in the error log for each
> - connection attempt (ie, for each retry).
> - */
> - last_errno=mysql_errno(mysql);
> - suppress_warnings= 0;
> - mi->report(ERROR_LEVEL, last_errno,
> - "error %s to master '%s@%s:%d'"
> - " - retry-time: %d retries: %lu",
> - (reconnect ? "reconnecting" : "connecting"),
> - mi->user, mi->host, mi->port,
> - mi->connect_retry,
> - DBUG_EVALUATE_IF("connect_to_master_always_report_error",
> - err_count+1,
> - mi->retry_count));
> - }
> + /*
> + SHOW SLAVE STATUS will display the number of retries which
> + would be real retry counts instead of mi->retry_count for
> + each connection attempt by 'Last_IO_Error' entry.
> + */
> + last_errno=mysql_errno(mysql);
> + suppress_warnings= 0;
> + mi->report(ERROR_LEVEL, last_errno,
> + "error %s to master '%s@%s:%d'"
> + " - retry-time: %d retries: %lu",
> + (reconnect ? "reconnecting" : "connecting"),
> + mi->user, mi->host, mi->port,
> + mi->connect_retry, err_count+1);
> /*
> By default we try forever. The reason is that failure will trigger
> master election, so if the user did not set mi->retry_count we
>
--
Your Sincerely,
Libing Song
==================================
MySQL Replication Team
Software Engineer
Email : Li-Bing.Song@stripped
Skype : libing.song
MSN : slb_database@stripped
Phone : +86 010-6505-4020 ext. 319
Mobile: +86 138-1144-2038
==================================
