List:Commits« Previous MessageNext Message »
From:Libing Song Date:July 30 2010 9:50am
Subject:Re: bzr commit into mysql-5.1-bugteam branch (sven.sandberg:3497)
Bug#49978
View as plain text  
Hi Sven,
Here are some review comments.
Please check it.


> 
[snip] 
> 
> 
> === modified file 'mysql-test/extra/rpl_tests/rpl_loaddata.test'
> --- a/mysql-test/extra/rpl_tests/rpl_loaddata.test	2010-02-04 11:26:36 +0000
> +++ b/mysql-test/extra/rpl_tests/rpl_loaddata.test	2010-07-26 13:31:58 +0000
>  
> @@ -165,6 +152,8 @@ terminated by ',' optionally enclosed by
>  connection slave;
>  --source include/wait_for_slave_sql_to_stop.inc
>  drop table t1, t2;
> +--source include/stop_slave_io.inc
> +RESET SLAVE;
We don't need to reset slave here, as the following test already adds
'source include/rpl_reset.inc'. 
> connection master;
>  drop table t1, t2;
>  
> @@ -178,7 +167,8 @@ DROP TABLE IF EXISTS t1;
>  
>  # BUG#48297: Schema name is ignored when LOAD DATA is written into binlog,
>  # replication aborts
> --- source include/master-slave-reset.inc
> +-- let $rpl_only_running_threads= 1
> +-- source include/rpl_reset.inc
see above. 
> 
>  -- let $db1= b48297_db1
>  -- let $db2= b42897_db2
> @@ -251,7 +241,7 @@ source include/diff_tables.inc;
>  -- sync_slave_with_master
>  
>  # BUG#49479: LOAD DATA INFILE is binlogged without escaping field names
> --- source include/master-slave-reset.inc
> +-- source include/rpl_reset.inc
>  -- connection master
>  use test;
>  CREATE TABLE t1 (`key` TEXT, `text` TEXT);
> 
[snip] 
> 
> === modified file 'mysql-test/extra/rpl_tests/rpl_reset_slave.test'
> --- a/mysql-test/extra/rpl_tests/rpl_reset_slave.test	2010-05-24 13:54:08 +0000
> +++ b/mysql-test/extra/rpl_tests/rpl_reset_slave.test	2010-07-26 13:31:58 +0000
> @@ -54,9 +54,9 @@ source include/check_slave_no_error.inc;
>  change master to master_user='impossible_user_name';
>  start slave;
>  let $slave_io_errno= 1045;
> -source include/wait_for_slave_io_error.inc;
> +--source include/wait_for_slave_io_error_and_stop.inc
> +--source include/stop_slave_sql.inc
>  
> -source include/stop_slave.inc;
>  change master to master_user='root';
>  source include/start_slave.inc;
>  source include/check_slave_no_error.inc;
> @@ -71,6 +71,19 @@ start slave;
>  let $slave_io_errno= 1045;
>  source include/wait_for_slave_io_error.inc;
>  
> -source include/stop_slave.inc;
> +# Now, Slave_IO_Error is set and Slave_IO_Running=NO, but the thread
> +# is still trying to connect, i.e.,
> +# Master_info::slave_running==MYSQL_RUN_NOT_CONNECT.  In order to run
> +# CHANGE MASTER, we must stop the IO thread completely.  Hence we do
> +# STOP SLAVE, and then wait for the Slave_IO_State to become empty.
> +STOP SLAVE;
It should stop completely, as 1045 is a fatal error.
so using stop_slave_sql.inc is enough. 
> +--source include/wait_for_slave_sql_to_stop.inc
> +--let $slave_param= Slave_IO_State
> +--let $slave_param_value=
> +--source include/wait_for_slave_param.inc
> +
>  reset slave;
>  source include/check_slave_no_error.inc;
> +
> +--let $rpl_only_running_threads= 1
> +--source include/rpl_end.inc
> 
[snip] 
> === modified file 'mysql-test/extra/rpl_tests/rpl_stm_EE_err2.test'
> --- a/mysql-test/extra/rpl_tests/rpl_stm_EE_err2.test	2008-11-13 19:19:00 +0000
> +++ b/mysql-test/extra/rpl_tests/rpl_stm_EE_err2.test	2010-07-26 13:31:58 +0000
> @@ -30,6 +30,10 @@ let $errno= query_get_value(SHOW SLAVE S
>  --echo Error: "$error" (expected different error codes on master and slave)
>  --echo Errno: "$errno" (expected 0)
>  drop table t1;
> +--source include/stop_slave.inc
> +RESET SLAVE;
It aims to clear the error message, right?
Please add a comment to explain it. 
> 
>  # End of 4.1 tests
>  
> +--let $rpl_only_running_threads= 1
> +--source include/rpl_end.inc
> 
> === added file 'mysql-test/extra/rpl_tests/rpl_test_framework.inc'
> --- a/mysql-test/extra/rpl_tests/rpl_test_framework.inc	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/extra/rpl_tests/rpl_test_framework.inc	2010-07-26 13:31:58 +0000
> @@ -0,0 +1,74 @@
> +# ==== Purpose ====
> +#
> +# Auxiliary file used by suite/rpl/t/rpl_test_framework.test
> +#
> +# The purpose is to check that the sync chain generated in
> +# rpl_change_topology.inc (invoked from rpl_init.inc) is correct. This
> +# is done in two ways:
> +#  (1) Print the sync chain.
> +#  (2) Execute a statement and verify that it replicates to all slaves.
> +#
> +#
> +# ==== Implementation ====
> +#
> +# Does this:
> +# (1) Set up a given replication topology (rpl_init.inc)
> +# (2) Print $rpl_sync_chain
> +# (3) Execute "DELETE FROM t1" and then "INSERT INTO t1" on the master
> +# (4) Sync and compare all servers.
> +# (5) Clean up the replication topology (rpl_end.inc)
> +#
> +# (Technical detail: Since DELETE FROM t1 is not executed at the end,
> +# some servers may have rows left in t1 from a previous invokation of
typo: invocation 
> +# rpl_test_framework.inc.  Therefore, this file will only work in
> +# statement mode where "DELETE FROM t1" removes rows that exist on
> +# slave but not on master.)
> +#
> +#
> +# ==== Usage ====
> +#
> +# --let $rpl_server_count= <number>
> +# --let $rpl_topology= <topology specification>
> +# --let $masters= <list of masters>
> +# [--let $rpl_diff_servers= <list of servers>]
> +# --source extra/rpl_tests/rpl_test_framework.inc
> +#
> +# Parameters:
> +#   $rpl_server_count, $rpl_topology:
> +#     See include/rpl_init.inc
> +#
> +#   $masters
> +#     This should be a list of numbers, each identifying a server.
> +#     The DELETE and INSERT statements will be executed on all servers
> +#     in the list.
> +#
> +#   $rpl_diff_servers
> +#     See include/rpl_diff.inc
> +
> +--source include/rpl_init.inc
> +--echo rpl_sync_chain= '$rpl_sync_chain'
> +
> +--inc $next_number
> +
> +if ($masters)
> +{
> +  # Iterate over masters
> +  while ($masters)
> +  {
> +    --let $master_i= `SELECT SUBSTRING_INDEX('$masters', ',', 1)`
> +    --let $masters= `SELECT SUBSTRING('$masters', LENGTH('$master_i') + 2)`
> +
> +    # Connect to master and execute statement
> +    --let $rpl_connection_name= server_$master_i
> +    --source include/rpl_connection.inc
> +    DELETE FROM t1;
> +    --eval INSERT INTO t1 VALUES ($next_number)
> +  }
> +
> +  # Compare all servers.
> +  --let $rpl_diff_statement= SELECT * FROM t1
> +  --source include/rpl_diff.inc
> +}
> +
> +--let $rpl_diff_servers=
> +--source include/rpl_end.inc
> 

> === modified file 'mysql-test/include/analyze-sync_with_master.test'
> --- a/mysql-test/include/analyze-sync_with_master.test	2008-08-04 19:54:44 +0000
> +++ b/mysql-test/include/analyze-sync_with_master.test	2010-07-26 13:31:58 +0000
What is this file for? 
> @@ -1,6 +1,2 @@
> -SHOW PROCESSLIST;
> -
> -let $binlog_name= query_get_value("SHOW MASTER STATUS", File, 1);
> -eval SHOW BINLOG EVENTS IN '$binlog_name';
> -
> -exit;
> \ No newline at end of file
> +--let $rpl_only_current_connection= 1
> +--source include/show_rpl_debug_info.inc
> 
> === modified file 'mysql-test/include/check-testcase.test'
> --- a/mysql-test/include/check-testcase.test	2008-12-13 19:42:12 +0000
> +++ b/mysql-test/include/check-testcase.test	2010-07-26 13:31:58 +0000
> @@ -8,8 +8,10 @@
>  # any unwanted side affects.
>  #
>  --disable_query_log
> +--replace_column 5 # 6 # 7 # 8 # 9 # 10 # 22 # 23 # 24 # 25 # 26 #
> +query_vertical
> +SHOW SLAVE STATUS;
> +
Can't we use show_slave_status.inc here? 
> call mtr.check_testcase();
>  --enable_query_log
>  
> -
> -
> 

[snip] 
> === modified file 'mysql-test/include/have_innodb.inc'
> --- a/mysql-test/include/have_innodb.inc	2010-03-31 13:04:40 +0000
> +++ b/mysql-test/include/have_innodb.inc	2010-07-26 13:31:58 +0000
> @@ -1,4 +1,4 @@
> -disable_query_log;
> ---require r/true.require
> -select (support = 'YES' or support = 'DEFAULT' or support = 'ENABLED') as `TRUE`
> from information_schema.engines where engine = 'innodb';
> -enable_query_log;
> +if (`SELECT COUNT(*) = 0 FROM INFORMATION_SCHEMA.ENGINES WHERE engine = 'innodb' AND
> (support = 'YES' OR support = 'DEFAULT' OR support = 'ENABLED')`)
how about 'AND support IN('YES', 'DEFAULT', 'ENABLED')`)' 
> +{
> +  --skip Test requires InnoDB.
> +}
> 

> 

> 

[snip] 
> 
> === added file 'mysql-test/include/rpl_begin_include_file.inc'
> --- a/mysql-test/include/rpl_begin_include_file.inc	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/include/rpl_begin_include_file.inc	2010-07-26 13:31:58 +0000
> @@ -0,0 +1,120 @@
> +# ==== Purpose ====
> +#
> +# This is an auxiliary file that facilitates writing include/*.inc
> +# files that invoke each other and reset the correct state afterwards.
> +# Assume that both rpl_a.inc and rpl_b.inc execute 'source
> +# include/rpl_begin_include_file.inc' at the beginning and 'source
> +# include/rpl_end_include_file.inc' at the end, and assume further
> +# that rpl_a.inc needs to source rpl_b.inc.  Then:
> +#
> +#  - When a user sources include/rpl_b.inc directly, the text
> +#    'include/rpl_b.inc' is written to the query log.  When a user
> +#    sources rpl_a.inc, the text 'include/rpl_a.inc' is written to the
> +#    query log (but the text include/rpl_b.inc is not written; i.e., the
> +#    internal workings of include/rpl_a.inc is not exposed to the
> +#    query log).
> +#
> +#  - If rpl_a.inc and/or rpl_b.inc needs to disable the query log,
> +#    then they can specify this to rpl_begin_include_file.inc and
> +#    rpl_end_include_file.inc.  If the user invokes rpl_b.inc or
> +#    rpl_a.inc, then rpl_b.inc will enable the query log at the end of
> +#    rpl_b.inc.  However, if rpl_a.inc needs the query log to be off,
> +#    then rpl_b.inc will not turn on the query log when rpl_a.inc
> +#    sources rpl_b.inc.
Could you please describe it in another way, I could not get the idea. 
> +#
> +#  - Warnings can be disabled similarly to how the query log is
> +#    disabled.
> +#
> +#  - If the mysqltest variable $rpl_debug is set, then
> +#    rpl_begin_include_file.inc will print
> +#    '==== BEGIN include/<filename> ====' and rpl_end_include_file.inc
> +#    will print '==== END include/<filename> ===='.
> +#
> +#
> +#
> 
> === added file 'mysql-test/include/rpl_change_topology.inc'
> --- a/mysql-test/include/rpl_change_topology.inc	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/include/rpl_change_topology.inc	2010-07-26 13:31:58 +0000
> @@ -0,0 +1,351 @@
> +# ==== Purpose ====
> +#
> +# Changes replication topology. This file is normally sourced from
> +# include/rpl_init.inc, but test cases can also source it if they
> +# need to change topology after they have sourced include/rpl_init.inc
> +#
> +# This file sets up variables needed by include/rpl_sync.inc and many
> +# other replication scripts in the include/ directory.  It also issues
> +# CHANGE MASTER on all servers where the configuration changes from
> +# what it was before.  It does not issue START SLAVE (use
> +# include/rpl_start_slaves.inc for that).
> +#
> +# Note: it is not currently possible to change the number of servers
> +# after the rpl_init.inc, without first calling rpl_end.inc. So the
> +# test has to set $rpl_server_count to the total number of servers
> +# that the test uses, before it sources include/rpl_init.inc.  After
> +# that, $rpl_server_count must not change until after next time the
> +# test sources include/rpl_end.inc.
> +#
> +# Note: Since this script issues CHANGE MASTER, the test case must
> +# ensure that all slaves where the configuration changes have stopped
> +# both the IO thread and the SQL thread before this script is sourced.
> +#
> +#
> +# ==== Usage ====
> +#
> +# --let $rpl_server_count= 6
hmm, I think rpl_server_count can get from rpl_topology.
We can use the max number in rpl_topology. 
> +# --let $rpl_topology= 1->2->3->1->4, 2->5, 6->7
> +# [--let $rpl_skip_change_master= 1]
> +# [--let $rpl_master_log_file= 1:master-bin.000001,3:master-bin.000003]
> +# [--let $rpl_master_log_pos= 1:4711,3:107]
> +# [--let $rpl_debug= 1]
> +# --source include/rpl_change_topology.inc
> +#
> +# Parameters:
> +#   $rpl_master_log_file
> +#     By default, CHANGE MASTER is executed with MASTER_LOG_FILE set
> +#     to the name of the last binlog file on the master (retrieved by
> +#     executing SHOW MASTER STATUS). This variable can be set to
> +#     specify another filename.  This variable should be a
> +#     comma-separated list of the following form:
> +#
> +#       SERVER_NUMBER_1:FILE_NAME_1,SERVER_NUMBER_2:FILE_NAME_2,...
> +#
> +#     Before CHANGE MASTER is executed on server N, this script checks
> +#     if $rpl_master_log_file contains the text N:FILE_NAME. If it
> +#     does, then MASTER_LOG_FILE is set to FILE_NAME. Otherwise,
> +#     MASTER_LOG_FILE is set to the last binlog on the master.  For
> +#     example, to specify that server_1 should start replicate from
> +#     master-bin.000007 and server_5 should start replicate from
> +#     master-bin.012345, do:
> +#       --let $rpl_master_log_file= 1:master-bin.000007,5:master-bin.012345
> +#
> +#   $rpl_master_log_pos
> +#     By default, CHANGE MASTER is executed without specifying the
> +#     MASTER_LOG_POS parameter.  This variable can be set to set a
> +#     specific position.  It has the same form as $rpl_master_log_file
> +#     (see above).  For example, to specify that server_3 should start
> +#     replicate from position 4711 of its master, do:
> +#       --let $rpl_master_log_pos= 3:4711
> +#
> +#   $rpl_server_count, $rpl_topology, $rpl_debug, $rpl_skip_change_master
> +#     See include/rpl_init.inc
> +#
> +#
> +# ==== Side effects ====
> +#
> +# Turns on enable_query_log.
> +# Does not change the status of enable_warnings.
> +# Does not change current connection.
> +#
> +#
> +# ==== Internal variables configured by this file ====
> +#
> +# This file sets up the following variables, which are used by other
> +# low-level replication files such as:
> +#   include/rpl_sync.inc
> +#   include/rpl_start_slaves.inc
> +#   include/rpl_stop_slaves.inc
> +#   include/rpl_end.inc
> +#
> +# $rpl_server_count_length:
> +#   Set to LENGTH($rpl_server_count). So if $rpl_server_count < 10,
> +#   then $rpl_server_count_length = 1; if 10 <= $rpl_server_count <
> +#   100, then $rpl_server_count_length = 2, etc.
> +#
> +# $rpl_master_list
> +#   Set to a string consisting of $rpl_server_count numbers, each one
> +#   whitespace-padded to $rpl_server_count_length characters. If
> +#   server N is a slave, then the N'th number is the master of server
> +#   N. If server N is not a slave, then the N'th number is just spaces
> +#   (so in fact it is not a number). For example, if $rpl_topology is
> +#   '1->2,2->3,3->1,2->4,5->6', then $rpl_master_list is '3122 6'.
> +#
> +# $rpl_sync_chain
> +#    Set to a string that specifies in what order servers should be
> +#    synchronized in include/rpl_sync.inc. This has the form of a
> +#    sequence of "chains". Each chain begins with
> +#    $rpl_server_count_length space characters, followed by a sequence
> +#    of numbers, each number whitespace-padded to
> +#    $rpl_server_count_length characters. Each number in the sequence
> +#    denotes a server, and the N'th server is a master of the (N+1)'th
> +#    server. For example, if $rpl_topology is
> +#    '1->2,2->3,3->1,2->4,5->6', then $rpl_sync_chain is ' 56
> 123124'.
> +
> +
> +# Remove whitespace from $rpl_topology
> +--let $rpl_topology= `SELECT REPLACE('$rpl_topology', ' ', '')`
> +
> +--let $rpl_include_filename= rpl_change_topology.inc [new topology=$rpl_topology]
> +--let $rpl_disable_query_log= 1
> +--source include/rpl_begin_include_file.inc
> +
> +
> +--let $_rpl_change_topology_old_connection= $CURRENT_CONNECTION
> +
> +
> +if ($rpl_debug)
> +{
> +  --echo ---- Check input ----
> +}
> +
> +
> +if (`SELECT '$rpl_topology' = '' OR '$rpl_server_count' = ''`)
> +{
> +  --die You must set $rpl_topology and $rpl_server_count before you source
> rpl_change_topology.inc. If you really want to change to the empty topology, set
> $rpl_topology= none
Could you please add the description into rpl_init.inc?
"If you really want to change to the empty topology, set $rpl_topology=
none" 
> +}
[snip] 
> +--let $_rpl_topology= $rpl_topology
> +if (`SELECT '$_rpl_topology' = 'none'`)
> +{
> +  --let $_rpl_topology=
> +}
It is not need. 'none' will be treated as 0. 
> +if (`SELECT '!$rpl_master_list!' = '!!'`)
> +{
> +  --die You must source include/rpl_init.inc before you source
> include/rpl_change_topology.inc
> +}
> +--let $_rpl_old_master_list= $rpl_master_list
> +
> +if ($rpl_debug)
> +{
> +  --echo \$rpl_server_count='$rpl_server_count'
> +  --echo \$rpl_server_count_length='$rpl_server_count_length'
> +  --echo new \$rpl_topology='$_rpl_topology'
> +  --echo old \$rpl_master_list='$rpl_master_list'
> +  --echo old \$rpl_sync_chain='$rpl_sync_chain'
> +}
> +
> +
> +if ($rpl_debug)
> +{
> +  --echo ---- Generate \$rpl_server_count_length and \$rpl_master_list ----
> +}
> +
> +--let $rpl_server_count_length= `SELECT LENGTH('$rpl_server_count')`
> +--let $rpl_master_list=
> +--let $_rpl_no_server= `SELECT REPEAT(' ', $rpl_server_count_length)`
> +--let $rpl_master_list= `SELECT REPEAT('$_rpl_no_server', $rpl_server_count)`
> +while ($_rpl_topology)
> +{
> +  # Get 's1->s2' from 's1->s2->s3->...' or from
> 's1->s2,s3->s4,...'
Please use '1->2' to replace 's1->s2', etc. 
> +  --let $_rpl_master_slave= `SELECT
> SUBSTRING_INDEX(SUBSTRING_INDEX('$_rpl_topology', ',', 1), '->', 2)`
> +  # Remove 's1->s2,' from 's1->s2,s3->s4,...' or remove
> +  # 's1->' from 's1->s2->...'
> +  --let $_rpl_topology= `SELECT SUBSTR('$_rpl_topology', IF(SUBSTR('$_rpl_topology',
> LENGTH('$_rpl_master_slave') + 1, 1) = ',', LENGTH('$_rpl_master_slave'), LOCATE('->',
> '$_rpl_master_slave')) + 2)`
> +  # Get 's1' from 's1->s2'
> +  --let $_rpl_master= `SELECT SUBSTRING_INDEX('$_rpl_master_slave', '->', 1)`
> +  # Get 's2' from 's1->s2'
> +  --let $_rpl_slave= `SELECT SUBSTRING('$_rpl_master_slave', LENGTH('$_rpl_master')
> + 3)`
> +  # Check that s2 does not have another master.
> +  if (`SELECT LOCATE('->$_rpl_slave', '$_rpl_topology')`)
> +  {
> +    --echo ERROR IN TEST: Server $_rpl_slave has more than one master in topology
> '$rpl_topology'
> +    --die ERROR IN TEST: found a server with more than one master in the
> $rpl_topology variable
> +  }
> +  # Save 's1' at position 's2' in $rpl_master_list
> +  --let $rpl_master_list= `SELECT INSERT('$rpl_master_list', 1 + ($_rpl_slave - 1) *
> $rpl_server_count_length, $rpl_server_count_length, RPAD('$_rpl_master',
> $rpl_server_count_length, ' '))`
> +}
> +
> +
> 
[snip] 
> === added file 'mysql-test/include/rpl_connect.inc'
> --- a/mysql-test/include/rpl_connect.inc	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/include/rpl_connect.inc	2010-07-26 13:31:58 +0000
> @@ -0,0 +1,76 @@
> +# ==== Purpose ====
> +#
> +# Create a connection to a given numbered server.
> +#
> +# This script is normally used internally by rpl_init.inc and
> +# master-slave.inc, but it can also be used in test cases that need to
> +# create more connections or re-create connections after disconnect.
> +#
> +#
> +# ==== Usage ====
> +#
> +# --let $rpl_connection_name= <connection_name>
> +# [--let $rpl_server_number= <server_number>]
> +# [--let $rpl_debug= 1]
> +# --source include/rpl_connect.inc
> +#
> +# Parameters:
> +#   $rpl_connection_name
> +#     The name of the connection to create.
> +#
> +#   $rpl_server_number
> +#     By default, $rpl_connection_name will be parsed to get the
> +#     server number. The following values for $rpl_connection_name are
> +#     recognized:
> +#
> +#       server_1, server_1_1, master, master1 - 1st server
> +#       server_2, server_2_1, slave, slave1 - 2nd server
> +#       server_3, server_3_1 - 3rd server
> +#       server_4, server_4_1 - 4th server
> +#       ...
> +#       server_N, server_N_1 - Nth server
> +#
> +#     Here, N = $rpl_server_count, as specified before rpl_init.inc
> +#     was sourced.
> +#
> +#     However, if $rpl_server_number is set, then $rpl_connection_name
> +#     is not parsed and $rpl_server_number is used instead.
> +#
> +#   $rpl_debug
> +#     See include/rpl_init.inc
> +
> +
> +--let $rpl_include_filename= rpl_connect.inc [creating $rpl_connection_name]
> +--source include/rpl_begin_include_file.inc
> +
> +
> +# Get server number for this connection.
> +if (!$rpl_server_number)
> +{
> +  --let $rpl_server_number= `SELECT IF('$rpl_connection_name' = 'master' OR
> '$rpl_connection_name' = 'master1', 1, IF('$rpl_connection_name' = 'slave' OR
> '$rpl_connection_name' = 'slave1', 2, IF('$rpl_connection_name' REGEXP '^server_[0-9]+',
> SUBSTRING_INDEX(SUBSTR('$rpl_connection_name', 8), '_', 1), '')))`
I think the caller have to set rpl_server_number and we don't need to
get server's number from the name. 
> +  if (!$rpl_server_number)
> +  {
> +    --echo Bug in test case: \$rpl_connection_name = '$rpl_connection_name' is not a
> known connection
> +    --die Bug in test case: $rpl_connection_name is not a known connection
> +  }
> +}
> +
> +# Get port number
> +--let $_rpl_port= \$SERVER_MYPORT_$rpl_server_number
> +if (`SELECT '$_rpl_port' = ''`)
> +{
> +  --echo Bug in test case: '\$SERVER_MYPORT_$rpl_server_number' not initialized.
> Check the test's .cfg file.
> +  --die Not all SERVER_MYPORT_* environment variables are setup correctly.
> +}
> +
> +# Create connection.
> +if ($rpl_debug)
> +{
> +  --echo connect ($rpl_connection_name,127.0.0.1,root,,test,$_rpl_port,)
> +}
> +--connect ($rpl_connection_name,127.0.0.1,root,,test,$_rpl_port,)
> +
> +--let $rpl_server_number=
> +
> +--let $rpl_include_filename= rpl_connect.inc
> +--source include/rpl_end_include_file.inc
> 
> === added file 'mysql-test/include/rpl_connection.inc'
> --- a/mysql-test/include/rpl_connection.inc	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/include/rpl_connection.inc	2010-07-26 13:31:58 +0000
> @@ -0,0 +1,47 @@
> +# ==== Purpose ====
> +#
> +# The same as 'connection $rpl_connection_name', but it can also
> +# prints the connection name.  The connection is printed if $rpl_debug
> +# is set, or if rpl_connection.inc is not called between two
> +# invokations of rpl_begin_include_file.inc/rpl_end_include_file.inc.
typo: invocation. 
> +# Otherwise the connection name is not printed.
> +#
> +#
> +
> 
> === renamed file 'mysql-test/include/diff_master_slave.inc' =>
> 'mysql-test/include/rpl_diff.inc'
> --- a/mysql-test/include/diff_master_slave.inc	2009-03-27 05:19:50 +0000
> +++ b/mysql-test/include/rpl_diff.inc	2010-07-26 13:31:58 +0000
How about expanding diff_tables.inc to support a diff_statement?
It is easy to maintain. 
> 
> ---echo source include/diff_master_slave.inc;
> -disable_query_log;
> -disable_result_log;
> +  # Execute statement
> +  --let $_rpl_diff_file=
> $MYSQLTEST_VARDIR/tmp/_rpl_diff_server-$_rpl_diff_server_i.tmp
> +  --exec $MYSQL --defaults-group-suffix=.$_rpl_diff_server_i $_rpl_diff_database
> < $_rpl_diff_statement_file > $_rpl_diff_file
Please make sure it can work well on Windows platforms. 
> 
> 
[snip] 
> 
> === renamed file 'mysql-test/include/master-slave-end.inc' =>
> 'mysql-test/include/rpl_end.inc'
> --- a/mysql-test/include/master-slave-end.inc	2006-04-12 13:56:29 +0000
> +++ b/mysql-test/include/rpl_end.inc	2010-07-26 13:31:58 +0000
> @@ -1,6 +1,90 @@
> ---connection master
> ---sync_slave_with_master
> ---connection slave
> ---disable_query_log
> -STOP SLAVE;
> ---enable_query_log
> +# ==== Purpose ====
> +#
> +# 
> +
> +--source include/rpl_sync.inc
> +--source include/rpl_stop_slaves.inc
> +--let $rpl_topology= 1->2
> +--source include/rpl_change_topology.inc
> +
Why do you change the rpl topology? 
> +
> +--connection default
> +--let $_rpl_server= $rpl_server_count
> +--let $_rpl_one= _1
> +while ($_rpl_server)
> +{
> +  --disconnect server_$_rpl_server
> +  --disconnect server_$_rpl_server$_rpl_one
> +  --dec $_rpl_server
> +}
> +
> +
> +--let $rpl_include_filename= rpl_end.inc
> +--source include/rpl_end_include_file.inc
> 
[snip] 
> 
> === renamed file 'mysql-test/include/circular_rpl_for_4_hosts_init.inc' =>
> 'mysql-test/include/rpl_init.inc'
> --- a/mysql-test/include/circular_rpl_for_4_hosts_init.inc	2008-04-25 16:54:42 +0000
> +++ b/mysql-test/include/rpl_init.inc	2010-07-26 13:31:58 +0000
> @@ -1,130 +1,229 @@
> +# Check that $rpl_server_count is set
> +if (`SELECT '$rpl_server_count' NOT REGEXP '^[0-9]+\$'`)
> +{
> +  --die You must set the mysqltest variable $rpl_server_count to a number before you
> source include/rpl_init.inc
> +}
Could you please check SERVER_MYPORT_N is not null(n is
1-$rpl_server_count)? 
> +
> +
> +
> 

[snip]

> 
> === renamed file 'mysql-test/include/circular_rpl_for_4_hosts_sync.inc' =>
> 'mysql-test/include/rpl_sync.inc'
> --- a/mysql-test/include/circular_rpl_for_4_hosts_sync.inc	2008-04-24 20:41:04 +0000
> +++ b/mysql-test/include/rpl_sync.inc	2010-07-26 13:31:58 +0000
> +--let $_rpl_i= 1
> +--let $_rpl_connect= 0
> +while ($_rpl_i) {
> +  --let $_rpl_server= `SELECT TRIM(SUBSTR('$rpl_sync_chain', 1 + ($_rpl_i - 1) *
> $rpl_server_count_length, $rpl_server_count_length))`
> +
Please add comment to explain that rpl_sync_chain always starts with
' ', so the first $_rpl_server is always null. 
> +  if ($_rpl_server)
> +  {
> +    if ($rpl_debug)
> +    {
> +      --echo [sync server_$_rpl_prev_server -> server_$_rpl_server]
> +    }
> +    if ($rpl_only_running_threads)
> +    {
> +      --connection server_$_rpl_server
> +      --let $_rpl_slave_io_running= query_get_value(SHOW SLAVE STATUS,
> Slave_IO_Running, 1)
> +      --let $_rpl_slave_sql_running= query_get_value(SHOW SLAVE STATUS,
> Slave_SQL_Running, 1)
> +      if ($rpl_debug)
> +      {
> +        --echo Sync IO: $_rpl_slave_io_running; Sync SQL: $_rpl_slave_sql_running
> +      }
> +      --let $_rpl_slave_io_running= `SELECT IF('$_rpl_slave_io_running' = 'Yes', 1,
> '')`
> +      --let $_rpl_slave_sql_running= `SELECT IF('$_rpl_slave_sql_running' = 'Yes',
> 1, '')`
> +      if ($_rpl_slave_io_running)
> +      {
> +        --connection server_$_rpl_prev_server
> +        if ($_rpl_slave_sql_running)
> +        {
> +          if ($rpl_debug)
> +          {
> +            --let $_rpl_master_file= query_get_value("SHOW MASTER STATUS", File, 1)
> +            --let $_rpl_master_pos= query_get_value("SHOW MASTER STATUS", Position,
> 1)
> +            --echo syncing master_file='$_rpl_master_file'
> master_pos='$_rpl_master_pos'
> +          }
> +          --sync_slave_with_master server_$_rpl_server
> +        }
> +        if (!$_rpl_slave_sql_running)
> +        {
> +          --source include/sync_slave_io_with_master.inc
> +        }
> +      }
> +      if (!$_rpl_slave_io_running)
> +      {
> +        if ($_rpl_slave_sql_running)
> +        {
> +          --source include/sync_slave_sql_with_io.inc
> +        }
> +      }
> +    }
> +    if (!$rpl_only_running_threads)
> +    {
> +      --connection server_$_rpl_prev_server
> +      if ($rpl_debug)
> +      {
> +        --let $_rpl_master_file= query_get_value("SHOW MASTER STATUS", File, 1)
> +        --let $_rpl_master_pos= query_get_value("SHOW MASTER STATUS", Position, 1)
> +        --echo syncing master_file='$_rpl_master_file'
> master_pos='$_rpl_master_pos'
> +      }
> +      --sync_slave_with_master server_$_rpl_server
> +    }
> +  }
> +
[snip] 
> +  # This happens at the beginning of a new sync subchain and at the
> +  # end of the full sync chain.
> +  if (!$_rpl_server)
> +  {
> +    --inc $_rpl_i
> +    --let $_rpl_server= `SELECT TRIM(SUBSTR('$rpl_sync_chain', 1 + ($_rpl_i - 1) *
> $rpl_server_count_length, $rpl_server_count_length))`
> +
> +    if (!$_rpl_server)
> +    {
> +      # terminate loop
> +      --let $_rpl_i= -1
> +    }
> +  }
> +
Could you please put this snip before 'if ($_rpl_server)', as it handles
the first $_rpl_server which is null. It is more clear if we put it at
the begin for the loop. 
> +  --let $_rpl_prev_server= $_rpl_server
> +  --inc $_rpl_i
> +}
> +
> +
> +--let $rpl_connection_name= $_rpl_sync_old_connection
> +--source include/rpl_connection.inc
> +
> +
> +--let $rpl_include_filename= rpl_sync.inc
> +--source include/rpl_end_include_file.inc
> 

> 
[snip] 
> === modified file 'mysql-test/include/show_rpl_debug_info.inc'
> --- a/mysql-test/include/show_rpl_debug_info.inc	2010-05-24 13:54:08 +0000
> +++ b/mysql-test/include/show_rpl_debug_info.inc	2010-07-26 13:31:58 +0000
> +
> +
> +--enable_query_log
> +--enable_result_log
> +--enable_warnings
> +--disable_abort_on_error
> +--horizontal_results
> +
> +
> +--let $_rpl_old_con= $CURRENT_CONNECTION
> +--let $_rpl_is_first_server= 1
$_rpl_is_first_server can be removed.
see below code. 
> +--let $_rpl_server= $rpl_server_count
> +--inc $_rpl_server
'inc $_rpl_server' should be removed. 
> +
>  
> -let $_master_con= $master_connection;
> -if (`SELECT '$_master_con' = ''`)
> +while ($_rpl_server)
>  {
> -  if (`SELECT '$_con' = 'slave'`)
> +  if (!$_rpl_is_first_server)
Replaced by 
+ if (!$rpl_only_current_connection) 
> {
> -    let $_master_con= master;
> +    --connection server_$_rpl_server
>    }
[snip]

> +
> +  --let $_rpl_is_first_server= 0
Remove it. 
> +  --dec $_rpl_server
> +  # Don't use same connection twice.
[snip] 
> +  if (`SELECT 'server_$_rpl_server' = '$_rpl_old_con'`)
>    {
> -    eval SHOW BINLOG EVENTS IN '$master_binlog_name_io';
> +    --dec $_rpl_server
> +    if ($rpl_only_current_connection)
> +    {
> +      --let $_rpl_server= 0
> +    }
>    }
Replaced by
+    if ($rpl_only_current_connection)
+    {
+      --let $_rpl_server= 0
+    }

> 
> === modified file 'mysql-test/include/show_slave_status.inc'
> --- a/mysql-test/include/show_slave_status.inc	2010-05-24 13:54:08 +0000
> +++ b/mysql-test/include/show_slave_status.inc	2010-07-26 13:31:58 +0000
> +#
> +# When none of the above applies, you may use this script instead.
> +# However, take care so that the test never contains explicit binlog
> +# coordinates. Usually you can read the binlog coordinate into a
> +# variable and compare it to some other coordinate.
typo: coordinates.

> 

> 
> === modified file 'mysql-test/include/stop_slave.inc'
> --- a/mysql-test/include/stop_slave.inc	2009-01-09 14:12:31 +0000
> +++ b/mysql-test/include/stop_slave.inc	2010-07-26 13:31:58 +0000
> @@ -3,19 +3,89 @@
> +
> +if ($rpl_only_running_threads)
> +{
> +  --let $_slave_sql_running= query_get_value(SHOW SLAVE STATUS, Slave_SQL_Running,
> 1)
> +  --let $_slave_io_running= query_get_value(SHOW SLAVE STATUS, Slave_IO_Running, 1)
> +  if ($rpl_debug)
> +  {
> +    --echo Stop SQL: $_slave_sql_running; Stop IO: $_slave_io_running
> +  }
> +
[snip] 
> +  --let $_slave_running_bits= `SELECT IF('$_slave_io_running' = 'Yes', 1, 0) +
> IF('$_slave_sql_running' = 'Yes', 2, 0)`
> +  if ($_slave_running_bits)
> +  {
> +    --dec $_slave_running_bits
> +    # $_slave_running_bits=1: io thread running
> +    if (!$_slave_running_bits)
> +    {
> +      --source include/stop_slave_io.inc
> +    }
> +    --dec $_slave_running_bits
> +    # $_slave_running_bits=2: sql thread running
> +    if (!$_slave_running_bits)
> +    {
> +      --source include/stop_slave_sql.inc
> +    }
> +    --dec $_slave_running_bits
> +    # $_slave_running_bits=2: both threads running
> +    if (!$_slave_running_bits)
> +    {
> +      STOP SLAVE;
> +      --source include/wait_for_slave_to_stop.inc
> +    }
It can be simplified as
  if (`SELECT '$_slave_io_running' = 'YES'`)
  { 
      --source include/stop_slave_io.inc
  } 
  if (`SELECT '$_slave_sql_running' = 'YES'`)
  {
      --source include/stop_slave_sql.inc
  }

> +  }
> +}
> +if (!$rpl_only_running_threads)
> +{
> +  STOP SLAVE;
> +  --source include/wait_for_slave_to_stop.inc
> +}
> +
> +
> +--let $rpl_include_filename= stop_slave.inc
> +--source include/rpl_end_include_file.inc
> 

> 
> === added file 'mysql-test/include/sync_io_with_master.inc'
> --- a/mysql-test/include/sync_io_with_master.inc	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/include/sync_io_with_master.inc	2010-07-26 13:31:58 +0000
I think we should combine sync_io_with_master.inc and
sync_slave_io_with_master.inc.

> === modified file 'mysql-test/include/sync_slave_io_with_master.inc'
> --- a/mysql-test/include/sync_slave_io_with_master.inc	2009-01-09 14:12:31 +0000
> +++ b/mysql-test/include/sync_slave_io_with_master.inc	2010-07-26 13:31:58 +0000
see above.

> 

> === added file 'mysql-test/include/wait_for_query_to_fail.inc'
> --- a/mysql-test/include/wait_for_query_to_fail.inc	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/include/wait_for_query_to_fail.inc	2010-07-26 13:31:58 +0000
> @@ -0,0 +1,25 @@
> +#
> +# Run a query over and over until it succeeds ot timeout occurs
typo: s/succeeds/fails/  s/ot/or/ 
> +#
> +
> +
> +let $counter= 100;
> +
> +disable_abort_on_error;
> +disable_query_log;
> +disable_result_log;
> +eval $query;
> +while (!$mysql_errno)
> +{
> +  eval $query;
> +  sleep 0.1;
> +  dec $counter;
> +
> +  if (!$counter)
> +  {
> +    --die "Waited too long for query to fail";
> +  }
> +}
> +enable_abort_on_error;
> +enable_query_log;
> +enable_result_log;
> 
> === modified file 'mysql-test/include/wait_for_slave_io_error.inc'
> --- a/mysql-test/include/wait_for_slave_io_error.inc	2010-05-28 02:57:45 +0000
> +++ b/mysql-test/include/wait_for_slave_io_error.inc	2010-07-26 13:31:58 +0000
> @@ -4,32 +4,37 @@
>  # error, or until a timeout is reached. Also waits until the IO
>  # thread has completely stopped.
It don't stop IO thread, please remove the comments.


-- 
Your Sincerely,
Libing Song
==================================
MySQL Replication Team
Software Engineer


Email : Li-Bing.Song@stripped
Skype : libing.song
MSN   : slb_database@stripped
Phone : +86 010-6505-4020 ext. 319
Mobile: +86 138-1144-2038
==================================


Thread
bzr commit into mysql-5.1-bugteam branch (sven.sandberg:3497) Bug#49978Sven Sandberg26 Jul
  • Re: bzr commit into mysql-5.1-bugteam branch (sven.sandberg:3497)Bug#49978Libing Song30 Jul
    • Re: bzr commit into mysql-5.1-bugteam branch (sven.sandberg:3497)Bug#49978Sven Sandberg5 Aug