Re: bzr commit into mysql-6.0 branch (sven:2662) Bug#37373

[Sven Sandberg]

Hi Andrei,

Thank you, I agree with all your comments. New patch committed. See my 
replies below.


Andrei Elkin wrote:
[...]
>>> However, in the case of the 2nd part of the original
>>> rpl_ndb_transaction.test a basic property is verified.
>>> That makes me think the 2nd part of the test is better to remain in a
>>> basic suite test.
>>> I guess you will second that the name rpl_ndb_transaction.test
>>> neither the suite rpl_ndb does not fit to problems of the test.
>>> It is supposed to verify XA-aware and XA-ignorant egnines replication,
>>> ndb is just one of them.
>>>
>>> So I'd rather to rename it like
>>>
>>>     rpl_xa_various_engine.test
>> mtr only sets up ndb and replication for tests in the rpl_ndb
>> suite. So the test has to stay in rpl_ndb.
> 
> Could it be renamed to reflect its purpose anyway? (only if you share
> this concern).

OK, you are right, rpl_ndb_transaction is no good. I'll rename it to 
rpl_ndb_mixed_storage_engine_transactions.test

> 
> ======================================================================
> Now, the newest patch:
> ======================================================================
> ...
>> === added file 'mysql-test/include/save_master_pos.inc'
>> --- a/mysql-test/include/save_master_pos.inc	1970-01-01 00:00:00 +0000
>> +++ b/mysql-test/include/save_master_pos.inc	2008-07-03 09:28:39 +0000
>> @@ -0,0 +1,16 @@
>> +# ==== Purpose ====
>> +#
>> +# Saves the master position in the variables
>> +# $saved_master_file and $saved_master_position.
>> +#
>> +# ==== Usage ====
>> +#
>> +# source include/save_master_pos.inc;
>> +#
>> +# Must be executed on a master.
>> +#
>> +# Usually followed by source include/wait_for_slave_io_to_sync.inc; or
>> +# some other script that uses the saved position.
>> +
>> +let $saved_master_file= query_get_value("SHOW MASTER STATUS", File, 1);
>> +let $saved_master_pos= query_get_value("SHOW MASTER STATUS", Position, 1);
>>
> 
> it's not a bad idea. 
> However, please notice that wait_for_... macros
> come with one or a pair of conditions like
> 
> #    let $slave_param= Slave_SQL_Running;
> #    let $slave_param_value= No;
> 
> wait_for_slave_io_to_sync.inc identifies it as member of the wait_for
> by name, but it does not use the set's convention.
> A way to exploit your idea, as stands to me, could be
> 
> to rename 
> 
> wait_for_slave_io_to_sync.inc -> sync_slave_io_with_master
> 
> and extend it with making 
> 
> source include/save_master_pos in its defintion.

Right, that's easier to use, and saves some lines in the test. I 
originally split it into two files since I thought it would be useful to 
be able to execute test commands between 'save master pos' and 'wait for 
io to sync', but you probably always want to sync the io thread to the 
last master pos. The only thing we don't allow by doing this is to sync 
some other connection than 'slave' with master, but that seems like an 
odd border case...

> I don't insist you have to do this though.
> 
> But there is important details.
> 
>> === added file 'mysql-test/include/wait_for_slave_io_to_sync.inc'
>> --- a/mysql-test/include/wait_for_slave_io_to_sync.inc	1970-01-01 00:00:00 +0000
>> +++ b/mysql-test/include/wait_for_slave_io_to_sync.inc	2008-07-03 09:28:39 +0000
>> @@ -0,0 +1,40 @@
>> +# ==== Purpose ====
>> +#
>> +# Waits until the slave IO thread has been synced, i.e., all events
>> +# have been copied over to slave.  Does not care if the SQL thread is
>> +# in sync.
>> +#
>> +#
>> +# ==== Usage ====
>> +#
>> +# Syncs the slave to the position saved in the last call to
>> +# include/save_master_pos.inc.
>> +#
>> +# Must be called on the slave.
>> +
>> +let $_error= `SELECT '$saved_master_pos' = '' OR '$saved_master_file' = ''`;
>> +if ($_error)
>> +{
>> +  --echo ERROR IN TEST SCRIPT: you must source include/save_master_pos.inc
> before include/wait_for_slave_io_to_sync.inc
>> +  exit;
>> +}
> 
> sync_slave_io_with_master whould not need that.
> 
>> +
>> +let $_counter= 300;
>> +let $_not_done= 0;
>> +
>> +while ($_not_done)
>> +{
>> +  dec $_counter;
>> +  if (!$_counter)
>> +  {
>> +    --echo ERROR: failed while waiting for slave io to sync. _file=$_file
> _pos=$_pos saved_master_file=$saved_master_file saved_master_pos=$saved_master_pos
>> +    query_vertical show slave status;
>> +    exit;
>> +  }
>> +
>> +  sleep 0.1;
>> +
>> +  let $_file= query_get_value("SHOW SLAVE STATUS", Master_Log_File, 1);
>> +  let $_pos= query_get_value("SHOW SLAVE STATUS", Read_Master_Log_Pos, 1);
>> +  let $_not_done= `SELECT '$_file' != '$saved_master_file' OR '$_pos' !=
> '$saved_master_pos'`;
>> +}
> 
> A way to define the new macro that looks to be the right one, is to
> reuse wait_for_slave_param.inc
> 
> connection master;
> source include/save_master_pos.inc;
> 
> connection slave;
> 
> let $slave_param= Master_Log_File;
> let $slave_param_value= $saved_master_file;
> include/wait_for_slave_param.inc
> let $slave_param= Read_Master_Log_Pos;
> let $slave_param_value= $saved_master_pos;
> include/wait_for_slave_param.inc;
> 
> EOF

Good idea, that's more elegant. I'll do like this.

> 
> 
> I am okay with the rest and am grateful that you accounted some of my
> former comments.
> 
> cheers,
> 
> Andrei

-- 
Sven Sandberg, Software Engineer
MySQL AB, www.mysql.com