Hi Serge,
On Tue, 2009-10-20 at 23:43 +0400, Serge Kozlov wrote:
> Hi, Luis.
>
> A agree with your notes from review but have just one comment.
>
> For semi-sync like for some other specific things we need to have an
> ability either use own (test) my.cnf for starting server or set command
> line options. For this patch we need set system variable --plugin_dir
> and point to the proper location.
That's correct.
> Could we do that in nuts or not?
I think we should. I believe at some point in time this was
considered... But then the feature was removed or left
unsupported. In any case it was rather primitive. I agree,
this should be improved or implemented in a proper way.
I concur that for this specific test case, one needs support
from nuts for loading plugins, thence the need for --plugin-dir
option.
Cheers,
Luís
>
> 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:
> >
> > $ 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
> >>
> >>
>
>
> --
> Serge Kozlov, QA Developer
> MySQL AB, Moscow, Russia, www.mysql.com
> Office:
>
> Are you MySQL certified? www.mysql.com/certification
>
--
Luís
