Hi Serge,
Good work. Please find my review comments below.
STATUS
------
Not approved.
REQUIRED CHANGES
----------------
RC1. I could not execute the test case:
$ 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).
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.
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.
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.
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.
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
>
>
--
Luís
