Hi, Luis.
I've committed new patch based on your review:
http://lists.mysql.com/commits/88215
Could you review it ASAP .. it should be done past week but I spent some
time for improvements for Nuts and semisync bugs (found in Nuts).
See my comments in-line.
Luís Soares пишет:
> Hi Serge,
>
> Good work. Please find my review comments below.
>
> STATUS
> ------
>
> Not approved.
>
> REQUIRED CHANGES
> ----------------
>
> RC1. I could not execute the test case:
Fixed.
>
> $ perl bin/nuts.pl --verbose --build ../mysql-5.1-rep+2/ --test
> semi_sync
> [12:11:19] rep::semi_sync ..
> 1..0
> skipped: (no reason given)
> [12:12:13]
> Files=1, Tests=0, 54 wallclock secs ( 0.31 usr 0.17 sys + 0.79 cusr
> 0.54 csys = 1.81 CPU)
> Result: NOTESTS
>
> I think this is because the check for semisync
> availability may be wrong. Shouldn't one:
>
> 1. Load the master plugin
> 2. Load the slave plugin
> 3. Check that the plugins were installed
> successfully. Maybe, check results for:
>
> select * from mysql.plugin where name like '%semi_sync_master';
> select * from mysql.plugin where name like '%
> semi_sync_slave';
>
> It's probably safe to load both plugins on every host and
> just activate them when needed (as it is today in the:
> change_rpl_mode function).
It implemented in new test template WithPlugin. I don't focus on
semisync plugin and added support for plugins in general.
>
> RC2. I think one needs to sync master and slave for both async
> and semisync replication. If one does not sync in the case
> of semisync replication, there can be the case that some
> events were not replayed by the SQL thread.
>
> If it was fully synchronous replication (meaning that the
> slave would only reply to the client once the transaction
> was replayed in all (a majority, or just one slave -
> depends on the resilience wanted), then we would not need
> to sync the slave(s).
>
> So the loop below the comment:
> # Sync master and slave for async replication
>
> needs to sync all slaves, not just the async ones.
Agree, fixed.
>
> REQUESTS
> --------
>
> R1. "attach" is starting the slave implicitely... I wonder if
> it should do it or not. We do have the "start_replication"
> function.
>
> In this test attach starts the slave to immediately after
> stop it, reconfigure semisync and start it again. Perhaps
> we should consider removing the:
>
> return ( $slave->execute ("START SLAVE") );
>
> from attach function.
Hm. Anyway I use wait_start_replication that just check that it started.
>
> SUGGESTIONS
> -----------
>
> S1. Perhaps I would consider making the function
> change_rpl_mode as part of the replication library. This
> function could:
>
> 1. Check whether semisync is active.
> 2. stop replication.
> 3. wait for replication to stop.
> 4. Set the correct active profile (master/slave).
> 5. start replication.
> 6. wait for replication to start.
>
> We could name it:
>
> enable_semi_sync or ok_enable_semisync.
Added to Replication.pm
enable_semi_sync
disable_semi_sync
ok_enable_semi_sync
ok_disable_semi_sync
>
> S2. As part of the replication library, why not also make the
> functions (this is just a suggestion):
>
> 1. is_plugin_available_semi_sync_master
> 2. install_plugin_semi_sync_master
> 3. install_plugin_semi_sync_slave
>
> Each of these functions would act as a facade for a two
> functions:
>
> 1. is_plugin_available(PLUGIN_ID)
> 2. install_plugin(PLUGIN_ID)
>
> So that, one could have something like:
>
> sub install_plugin_semi_sync_master()
> {
> return install_plugin(PLUGIN_SEMI_SYNC_MASTER_ID);
> }
>
> These are just suggestions, but something done in this context
> would be great (I think). But Alfranio should have a saying in
> this as well.
It is responsibility of server() of WithPlugin template.
Server() is looking for correct plugin dir by library name, starts
server with proper --plugin-dir and then loads plugins.
>
> DETAILS
> -------
>
> Please find some minor comments inline.
>
> On Tue, 2009-09-29 at 01:48 +0400, Serge Kozlov wrote:
>> #At file:///home/ksm/sun/repo/WL5038/nuts-commit/ based on
> revid:serge.kozlov@stripped
>>
>> 356 Serge Kozlov 2009-09-29
>> WL#5038 Test case
>> added:
>> suites/rep/semi_sync.pm
>> modified:
>> lib/My/Nuts/Library/Kernel/ServerResult.pm
>>
>> === modified file 'lib/My/Nuts/Library/Kernel/ServerResult.pm'
>> --- a/lib/My/Nuts/Library/Kernel/ServerResult.pm 2009-09-25 11:24:12 +0000
>> +++ b/lib/My/Nuts/Library/Kernel/ServerResult.pm 2009-09-28 21:48:49 +0000
>> @@ -2,7 +2,7 @@ package My::Nuts::Library::Kernel::Serve
>> use Exporter;
>> our @ISA = qw(Exporter);
>> our @EXPORT =
>> - qw(compare_dbs ok_compare_dbs get_supported_engines get_table_list);
>> + qw(compare_dbs ok_compare_dbs get_supported_engines get_table_list
> get_variable);
>> use strict;
>> use warnings;
>> use My;
>> @@ -139,6 +139,21 @@ sub get_table_list
>> return $tables;
>> }
>>
>> +sub get_variable
>> +{
>> + my $params = shift;
>> + my $conn = $params->{"conn"};
>> + my $var = $params->{"var"};
>> + my $result = undef;
>> + my $rs = sql ($conn, "SHOW VARIABLES LIKE '" . $var . "';");
>> + my @rs_data = get_next($rs);
>> + if (defined $rs_data[0])
>> + {
>> + $result = $rs_data[1];
>> + }
>> + return $result;
>> +}
>
> Good. Nice that you've done it.
>
>> 1;
>>
>> __END__;
>>
>> === added file 'suites/rep/semi_sync.pm'
>> --- a/suites/rep/semi_sync.pm 1970-01-01 00:00:00 +0000
>> +++ b/suites/rep/semi_sync.pm 2009-09-28 21:48:49 +0000
>> @@ -0,0 +1,171 @@
>> +package rep::semi_sync;
>> +use Exporter;
>> +our @ISA = qw(Exporter My::Nuts::Library::Tests::SimpleTest);
>> +use strict;
>> +use warnings;
>> +use My;
>> +use My::Nuts::Library::Kernel::Server;
>> +use My::Nuts::Library::Kernel::ServerResult;
>> +use My::Nuts::Library::Kernel::Manager;
>> +use My::Nuts::Library::Kernel::Result;
>> +use My::Nuts::Library::Tests::SimpleTest;
>> +use My::Nuts::Library::Kernel::Replication;
>> +use Test::More;
>> +use DataSource;
>> +
>> +sub prepare
>> +{
>> + return;
>> +}
>> +
>> +sub startup
>> +{
>> + return;
>> +}
>> +
>> +sub fire
>> +{
>> + my ($test) = @_;
>> + # Create 3 servers
>> + my $server1 = ok_server ($test);
>> + my $server2 = ok_server ($test);
>> + my $server3 = ok_server ($test);
>> + # Move server to hash
>> + my $servers = {
>> + "A" => $server1,
>> + "B" => $server2,
>> + "C" => $server3
>> + };
>> + # List of test schemas
>> + my $schemas = [
>> + [
>> + {"topology" => "A->B"},
>> + {"A->B" => "async"},
>> + {"A->B" => "semisync"}
>> + ],
>> + [
>> + {"topology" => "A->B,B->C"},
>> + {"A->B" => "semisync", "B->C" => "semisync"},
>> + {"A->B" => "semisync", "B->C" => "async"}
>> + ],
>> + [
>> + {"topology" => "A->B,A->C"},
>> + {"A->B" => "semisync", "A->C" => "semisync"},
>> + {"A->B" => "semisync", "A->C" => "async"}
>> + ]
>> + ];
>> + # Init stuff
>> + my $dbname = "test";
>> + my $init_queries =[
>> + "DROP DATABASE IF EXISTS " . $dbname,
>> + "CREATE DATABASE " . $dbname,
>> + "USE " . $dbname
>> + ];
>> +
>> + # Check semisync support
>> + my $semisync_enabled = 1;
>> + foreach my $server (values %$servers)
>> + {
>> + $semisync_enabled = 0 if (!defined(get_variable ( {"conn" => $server, "var"
> => "rpl_semi_sync_master_enabled"})));
>
> Before this, we need the plugin to be loaded. Do we load
> it elsewhere? I cannot find the INSTALL PLUGIN clause and the
> test run I did would not succeed, bailing out with skipped...
>
>> + }
>> + if ($semisync_enabled == 1)
>> + {
>> + # Get statements from data source and add creating db
>> + my $data = get_data_from_source();
>> + unshift(@$data, @$init_queries);
>> +
>> + # Plan
>> + plan tests => scalar(@$schemas) * scalar(@$data) + 53;
>> +
>> + # Main loop
>> + foreach my $schema (@$schemas)
>> + {
>> + # Create pairs master->slave and store them into hash
>> + #print $schema->[0]->{"topology"} . "\n";
>> + my @atomic = split(",", $schema->[0]->{"topology"});
>> + my $cur_servers = {};
>> + foreach my $pair (@atomic)
>> + {
>> + $pair =~ m/(.+)\-\>(.+)/i;
>> + $cur_servers->{$pair}->{"master"} = $servers->{$1};
>> + $cur_servers->{$pair}->{"slave"} = $servers->{$2};
>> + attach($cur_servers->{$pair}->{"master"},
> $cur_servers->{$pair}->{"slave"});
>
> I don't recall why attach does an implicit start slave,
> but maybe it should not... What do you think? Alfranio?
>
>> + ok_wait_start_replication($cur_servers->{$pair}->{"slave"});
>> + }
>> + my $num = scalar(@$schema) - 1;
>> + my $block_start = 0;
>> + my $block_end = 0;
>> + for (my $i = 1; $i <= $num; $i++)
>> + {
>> + # Prepare replication
>> + foreach my $detail (keys %{$schema->[$i]})
>> + {
>> + change_rpl_mode($cur_servers->{$detail}->{"master"},
> $cur_servers->{$detail}->{"slave"}, $schema->[$i]->{$detail})
>> + }
>> + # Run bulk of statements on master
>> + $block_start = $block_end;
>> + $block_end = int (($i/$num) * scalar(@$data));
>> + for (my $j = $block_start; $j < $block_end; $j++)
>> + {
>> + ok_sql($servers->{"A"}, $data->[$j]);
>> + }
>> + # Sync master and slave for async replication
>> + foreach my $detail (keys %{$schema->[$i]})
>> + {
>> + if ($schema->[$i]->{$detail} =~ m/async/i)
>> + {
>> + ok_synchronize($cur_servers->{$detail}->{"master"},
> $cur_servers->{$detail}->{"slave"});
>> + }
>
> I think one needs to synchronize both async and semisync
> master->slave(s).
>
>> + }
>> + }
>> + # Check DBs
>> + foreach my $pair (keys %{$cur_servers})
>> + {
>> + ok_compare_dbs([
>> + {"conn" => $cur_servers->{$pair}->{"master"}, "dbname" =>
> $dbname}
>> + ]);
>> + }
>> + # Stop replication links and clean up servers
>> + foreach my $pair (keys %{$cur_servers})
>> + {
>> + stop_replication($cur_servers->{$pair}->{"slave"});
>> + reset_master($cur_servers->{$pair}->{"master"});
>> + reset_slave($cur_servers->{$pair}->{"slave"});
>> + }
>> + }
>> + }
>> +}
>> +
>> +sub shutdown
>> +{
>> + return;
>> +}
>> +
>> +sub change_rpl_mode
>> +{
>> + my ($master, $slave, $mode) = @_;
>> + my $modes = {
>> + "async" => 0,
>> + "semisync" => 1
>> + };
>> + sql($master, "SET GLOBAL rpl_semi_sync_master_enabled = " .
> $modes->{$mode});
>> + sql($slave, "SET GLOBAL rpl_semi_sync_slave_enabled = " .
> $modes->{$mode});
>> + ok_stop_replication($slave);
>> + ok_wait_stop_replication($slave);
>> + ok_start_replication($slave);
>> + ok_wait_start_replication($slave);
>> +}
>> +
>> +1;
>> +__END__;
>> +
>> +=head1 NAME
>> +
>> +rep::semi_sync - Semisynchronous replication
>> +
>> +=head1 SYNOPSIS
>> +
>> +Semisynchronous replication for different topologies (A->B, A->B->C,
> B<-A->C) and switching
>> +to asynchronous and back.
>> +
>> +=back
>>
>>
--
Serge Kozlov, QA Developer
MySQL AB, Moscow, Russia, www.mysql.com
Office:
Are you MySQL certified? www.mysql.com/certification
