Re: bk commit into 5.1 tree (ramil:1.2524) BUG#19328

[Ramil Kalimullin]

Hi Rafal!

Thanks for your review!

Rafal Somla wrote:
>> +#define DBUG_FORCE_SLAVE_TO_RECONNECT(var, count, msg) \
> 
> I suggest to change parameter name "msg" to "act".
ok

>> +  DBUG_EXECUTE_IF(var, \
>> +      if (!count) \
>> +      { \
>> +        count++; \
>> +        sql_print_information("Forcing to reconnect slave I/O 
>> thread"); \
>> +        if (try_to_reconnect(thd, mysql, mi, &retry_count, 
>> suppress_warnings, \
>> +                             reconnect_messages[msg])) \
> 
> Are retry_count and suppress_warning global variables? If not, then it 
> is not so good.
No.
There are too many parameters to pass (also thd, mysql and mi) :(
I'll add a function instead of the macro.

>> +  if (!suppress_warnings) +  {
>> +    char buf[256], llbuff[22];
>> +    my_snprintf(buf, sizeof(buf), messages[SLAVE_RECON_MSG_FAILED], 
>> +                " log '%s' at postion %s", IO_RPL_LOG_NAME,
>> +                llstr(mi->master_log_pos, llbuff));
> 
> This my_snprintf() invocation is not correct. I mean, it will work but 
> the log positions will be ignored. This is because 
> messages[SLAVE_RECON_MSG_FAILED] will be used as format string and it 
> contains no %... fields.
Nice catch, thanks!

>> +    if (messages[SLAVE_RECON_MSG_COMMAND][0])
> 
> Why above if condition? Could you add explaining comment?
For event reading we should not raise an error (I don't know why, but it was so),
and there is no COM_XXXXXXX command for that.
Will add a comment.

 >> +  if (safe_reconnect(thd, mysql, mi, 1) || io_slave_killed(thd, mi))
 > Why not check_io_slave_killed() above?
We have it a bit earlier, right after the safe_sleep().

Regards,
Ramil.