Hi Sven,
That's a good reason, patch approved!
Sven Sandberg wrote:
> He Zhenxing wrote:
> > Hi Sven,
> >
> > Great work! Thanks for the improvement of the test code!
> >
> > Patch approved, only one minor comment.
> >
> > Sven Sandberg wrote:
> >> #At file:///home/sven/bzr/b37718-rpl_stm_mystery22/5.1-rpl/ based on
> revid:zhenxing.he@stripped
> >>
> >> 2709 Sven Sandberg 2009-01-07
> >> BUG#37718: rpl.rpl_stm_mystery22 fails sporadically on pushbuild
> >> Problem 1: The test waits for an error in the slave sql thread,
> >> then resolves the error and issues 'start slave'. However, there
> >> is a gap between when the error is reported and the slave sql
> >> thread stops. If this gap was long, the slave would still be
> >> running when 'start slave' happened, so 'start slave' would fail
> >> and cause a test failure.
> >> Fix 1: Made wait_for_slave_sql_error wait for the slave to stop
> >> instead of wait for error in the IO thread. After stopping, the
> >> error code is verified. If the error code is wrong, debug info
> >> is printed. To print debug info, the debug printing code in
> >> wait_for_slave_param.inc was moved out to a new file,
> >> show_rpl_debug_info.inc.
> >> Problem 2: rpl_stm_mystery22 is a horrible name, the comments in
> >> the file didn't explain anything useful, the test was generally
> >> hard to follow, and the test was essentially duplicated between
> >> rpl_stm_mystery22 and rpl_row_mystery22.
> >> Fix 2: The test is about conflicts in the slave SQL thread,
> >> hence I renamed the tests to rpl_{stm,row}_conflicts. Refactored
> >> the test so that the work is done in
> >> extra/rpl_tests/rpl_conflicts.inc, and
> >> rpl.rpl_{row,stm}_conflicts merely sets some variables and then
> >> sourced extra/rpl_tests/rpl_conflicts.inc.
> >> The tests have been rewritten and comments added.
> >> Problem 3: When calling wait_for_slave_sql_error.inc, you always
> >> want to verify that the sql thread stops because of the expected
> >> error and not because of some other error. Currently,
> >> wait_for_slave_sql_error.inc allows the caller to omit the error
> >> code, in which case all error codes are accepted.
> >> Fix 3: Made wait_for_slave_sql_error.inc fail if no error code
> >> is given. Updated rpl_filter_tables_not_exist accordingly.
> >> Problem 4: rpl_filter_tables_not_exist had a typo, the dollar
> >> sign was missing in a 'let' statement.
> >> Fix 4: Added dollar sign.
> >> Problem 5: When replicating from other servers than the one named
> >> 'master', the wait_for_slave_* macros were unable to print debug
> >> info on the master.
> >> Fix 5: Remove parameter $slave_keep_connection by
> >> $master_connection.
> >
> > [snip]
> >
> >> === added file 'mysql-test/include/show_rpl_debug_info.inc'
> >> --- a/mysql-test/include/show_rpl_debug_info.inc 1970-01-01 00:00:00 +0000
> >> +++ b/mysql-test/include/show_rpl_debug_info.inc 2009-01-07 08:07:07 +0000
> >> @@ -0,0 +1,75 @@
> >> +# ==== Purpose ====
> >> +#
> >> +# Print status information for replication, typically used to debug
> >> +# test failures.
> >> +#
> >> +# First, the following is printed on slave:
> >> +#
> >> +# SHOW SLAVE STATUS
> >> +# SHOW PROCESSLIST
> >> +# SHOW BINLOG EVENTS IN <binlog_name>
> >> +#
> >> +# Where <binlog_name> is the currently active binlog.
> >> +#
> >> +# Then, the following is printed on master:
> >> +#
> >> +# SHOW MASTER STATUS
> >> +# SHOW PROCESSLIST
> >> +# SHOW BINLOG EVENTS IN <sql_binlog_name>
> >> +# SHOW BINLOG EVENTS IN <io_binlog_name>
> >> +#
> >> +# Where <sql_binlog_name> is the binlog name that the slave sql
> thread
> >> +# is currently reading from and <io_binlog_name> is the binlog that
> >> +# the slave IO thread is currently reading from.
> >> +#
> >> +# ==== Usage ====
> >> +#
> >> +# [let $master_connection= <connection>;]
> >> +# source include/show_rpl_debug_info.inc;
> >> +#
> >> +# If $master_connection is set, debug info will be retrieved from the
> >> +# connection named $master_connection. Otherwise, it will be
> >> +# retrieved from the 'master' connection if the current connection is
> >> +# 'slave'.
> >> +
> >> +let $_con= $CURRENT_CONNECTION;
> >> +--echo [on $_con]
> >> +--echo **** SHOW SLAVE STATUS on $_con ****
> >> +query_vertical SHOW SLAVE STATUS;
> >> +--echo **** SHOW PROCESSLIST on $_con ****
> >> +SHOW PROCESSLIST;
> >> +--echo **** SHOW BINLOG EVENTS on $_con ****
> >> +let $binlog_name= query_get_value("SHOW MASTER STATUS", File, 1);
> >> +eval SHOW BINLOG EVENTS IN '$binlog_name';
> >> +
> >> +let $_master_con= $master_connection;
> >> +if (!$_master_con) {
> >> + if (`SELECT '$_master_con' = 'slave') {
> >> + let $_master_con= master;
> >> + }
> >> + if (!$_master_con) {
> >> + --echo Unable to determine master connection. No debug info printed for
> master.
> >> + --echo Please fix the test case by setting $master_connection before
> sourcing
> >> + --echo show_rpl_debug_info.inc.
> >
> > I think an 'exit' should be added here to quit the test case.
>
> The purpose of show_rpl_debug_info.inc is just to print debug info. In
> the two use cases we have currently, there is an 'exit' right after
> 'source show_rpl_debug_info.inc', but I think it is possible to think of
> situations when 'exit' is not desired. E.g., a test case may want to
> print other debug info after 'source show_rpl_debug_info.inc'. So I
> would prefer to not add exit. OK?
>
> /Sven
>
>
