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
