List:Commits« Previous MessageNext Message »
From:Luís Soares Date:April 19 2010 10:23pm
Subject:Re: bzr commit into mysql-5.1-rep+2 branch (luis.soares:3183) Bug#44209
View as plain text  
Hi Li-Bing,

On Mon, 2010-04-12 at 17:05 +0800, Libing Song wrote:
> Hi Luis,
> 
> Great work.
> Please find my review comments below.
> 
>     I suppose you will fix both bug#44209 and bug#44486 together, 
>     right? I will not review bug#44486 if it is true.

No. I just use patch for bug#44486 for testing purposes in this 
patch.

> STATUS
> ------
>  
>   Not Approved.
> 
> REQUIRED CHANGES
> ----------------
> Some questions should be clarified.
>   RC1. Should mi->retry_count be stored into master.info ?
>        I think it should as mi->connect_retry is stored into master.info
>        If you agree, please load mi->connect_retry from master.info
>        when initializing master info.

Ok. Done.

>   RC3. Please check test cases again, there might be more result files 
>        related to SHOW SLAVE STATUS. 
>        The following tests' result have to be changed.
>        + rpl_loaddata_fatal.result
>        + rpl_known_bugs_detection.result
>        + rpl_rbr_to_sbr.result
>        + rpl_rotate_logs.result
>        + rpl_slave_skip.result

OK. Checked and Updated.
  
>   RC4. mi->retry_count has been changed after 'CHANGE MASTER TO', so 
>        you have not to call 'include/start-slave.inc'.

Hmm... Yes. However keeping it may provide a little more coverate so
lets keep it.
       
>  
> REQUESTS
> --------
>   Please add test to verify MASTER_RETRY_COUNT is valid.
>   for example: 
>   CHANGE MASTER TO MASTER_RETRY_COUNT=3 ...
>   I/O thread will stop after 3 times try and print an error with the
> correct try times.

Done (but I have butterflies regarding this test case).

> 
> SUGGESTIONS
> -----------
>   Shall we add some exceptional test cases ?
>   for example:
>   CHANGE MASTER TO MASTER_RETRY_COUNT='blabla'.
> 

Done.

> DETAILS 
> -------
>  n/a
> 
> 
> > 
> > === modified file 'sql/lex.h'
> > --- a/sql/lex.h	2009-12-03 08:59:58 +0000
> > +++ b/sql/lex.h	2010-04-08 15:24:22 +0000
> > @@ -315,6 +315,7 @@ static SYMBOL symbols[] = {
> >    { "MASTER_LOG_POS",           SYM(MASTER_LOG_POS_SYM)},
> >    { "MASTER_PASSWORD",           SYM(MASTER_PASSWORD_SYM)},
> >    { "MASTER_PORT",           SYM(MASTER_PORT_SYM)},
> > +  { "MASTER_RETRY_COUNT",           SYM(MASTER_RETRY_COUNT_SYM)},
> Could you please explain why add it?
> Or what are this symbols for ?

This is the definition of the MASTER_RETRY_COUNT token.

> >    { "MASTER_SERVER_ID",           SYM(MASTER_SERVER_ID_SYM)},
> >    { "MASTER_SSL",       SYM(MASTER_SSL_SYM)},
> >    { "MASTER_SSL_CA",    SYM(MASTER_SSL_CA_SYM)},
> > 
> 
> 

The new patch is available at: 
http://lists.mysql.com/commits/106058

Regards,
Luís


Thread
bzr commit into mysql-5.1-rep+2 branch (luis.soares:3183) Bug#44209Luis Soares8 Apr
  • Re: bzr commit into mysql-5.1-rep+2 branch (luis.soares:3183) Bug#44209Libing Song12 Apr
    • Re: bzr commit into mysql-5.1-rep+2 branch (luis.soares:3183) Bug#44209Luís Soares20 Apr
  • Re: bzr commit into mysql-5.1-rep+2 branch (luis.soares:3183) Bug#44209Libing Song28 Apr