Hi Sven.
Thanks for your review comments, please see my answers below.
On Thursday 22 November 2007 19:50:48, Sven Sandberg wrote:
>Hi Alexey,
>
>I think the patch looks good. I have two suggestions: (1) about comments
>and (2) about test cases.
>
>(1) This is a very difficult piece of code, and I would suggest to add
>comment explaining under what conditions the error message is removed
>(this will make the code safer for the future).
>
>It discards errors if check_io_slave_killed returns true.
>check_io_slave_killed returns true if io_slave_killed() returns true.
>io_slave_killed returns true under this condition:
>
> DBUG_RETURN(mi->abort_slave || abort_loop || thd->killed);
>
>mi->abort_slave is in the following conditions:
> - by the slave when STOP SLAVE is issued (sql_repl.cc:stop_slave()
>calls slave.cc:terminate_slave_threads() which sets the flag
> - by the slave when the condition in "START SLAVE UNTIL condition" is
>met (slave.cc:exec_relay_log_event()). However, this cannot happen while
>starting the slave.
>
>abort_loop and thd->killed are not set by "STOP SLAVE" AFAICT; they are
>set when the slave server is shut down for various reasons. I think it
>is ok to hide these messages (doesn't matter if you get a few messages
>on shutdown or not), but the behavior should be documented so that the
>code can be edited in a safe way in the future.
>
thd->killed is set when a thread was killed with the KILL command. abort_loop
is set at the server shutdown.
While it would be good of course to have the above documented somewhere, I
don't see what should be documented with respect to this particular bug fix.
check_io_slave_killed() does just what the name suggests, i.e. it checks
whether the I/O thread was killed as a result of an explicit request from a
user. What the patch does is kind of self-explaining. It simply suppresses
the error when check_io_slave_killed() returns true, that is, when the I/O
thread was explicitly killed.
Do you agree?
>
>(2) Would it be difficult to create a test case? Just something that does
>
>START SLAVE;
>STOP SLAVE;
>
>should suffice, right? If it's not much more difficult than that, I
>think it should be included.
>
Note that what the patch tries to fix is a failure in the test suite, This
means that the fix itself is already covered. For example, rpl_ssl.test does
roughly the same (START SLAVE followed by a soon STOP SLAVE) many times in a
loop:
let $i= 100;
while ($i)
{
start slave;
connection master;
insert into t1 values (NULL);
select * from t1; # Some variance
connection slave;
select * from t1; # Some variance
stop slave;
dec $i;
}
Thus I don't see a need for a separate test case for this bug. If it's not
fixed, we will have rpl_ssl, rpl_server_id, etc. failing on some of the
PushBuild machines, so the bug will be reopened.
Best regards,
--
Alexey Kopytov, Software Developer
MySQL AB, www.mysql.com
Are you MySQL certified? www.mysql.com/certification
