List:Commits« Previous MessageNext Message »
From:Libing Song Date:September 21 2010 10:26am
Subject:Re: bzr commit into mysql-next-mr-bugfixing branch (Dao-Gang.Qu:3273)
Bug#56416
View as plain text  
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
==================================

Thread
bzr commit into mysql-next-mr-bugfixing branch (Dao-Gang.Qu:3273) Bug#56416Dao-Gang.Qu20 Sep
  • Re: bzr commit into mysql-next-mr-bugfixing branch (Dao-Gang.Qu:3273)Bug#56416Libing Song21 Sep
    • Re: bzr commit into mysql-next-mr-bugfixing branch (Dao-Gang.Qu:3273)Bug#56416Alfranio Correia24 Sep
      • Re: bzr commit into mysql-next-mr-bugfixing branch (Dao-Gang.Qu:3273)Bug#56416Daogang Qu25 Sep
      • Re: bzr commit into mysql-next-mr-bugfixing branch (Dao-Gang.Qu:3273)Bug#56416Libing Song25 Sep
        • Re: bzr commit into mysql-next-mr-bugfixing branch (Dao-Gang.Qu:3273)Bug#56416Libing Song25 Sep
          • Re: bzr commit into mysql-next-mr-bugfixing branch (Dao-Gang.Qu:3273)Bug#56416Daogang Qu25 Sep