Sergei Golubchik <sergii@pisem.net> writes:
Kristian, I've done the changes - would you mind to review the patch ?
P.S. there are three patches attached - three bundles.
the first one fixes ndb. as ndb is hacked in mtr and is all over the place, I could not change mtr without touching ndb related pieces, and I need to make sure that I didn't break them. I found out that ndb doesn't even build - without my changes, so I fixed that. I think we need to build ndb at on some buildbot slaves, just to make sure we don't break it. Or remove it completely - as it's clearly not used, and not expected to be used, ndb users should use telco trees.
the second contain mtr changes.
the third adds sphinx suite.
My main comment is that there should be some documentation of the My::Suite class, as I write in a comment below. Other than that I think it is ok to push. I have a number of small remarks inline below, you can fix what you agree with and ignore the rest. There are also a couple of questions to check that my understanding is correct. - Kristian.
=== modified file 'BUILD/SETUP.sh' --- BUILD/SETUP.sh 2010-03-30 16:16:57 +0000 +++ BUILD/SETUP.sh 2010-08-02 20:20:47 +0000
@@ -178,8 +178,7 @@ max_no_qc_configs="$SSL_LIBRARY --with-plugins=max --without-query-cache" max_no_ndb_configs="$SSL_LIBRARY --with-plugins=max-no-ndb --with-embedded-server --with-libevent" max_configs="$SSL_LIBRARY --with-plugins=max --with-embedded-server --with-libevent" -# Disable NDB in maria max builds -max_configs=$max_no_ndb_configs
One might think that this change would cause BUILD/xxx-max to include NDB. But it seems it doesn't. I think the reason is that storage/ndb/plug.in does not list itself as a member of the "max" configuration. Is my understanding correct, so that this particular change is just a cleanup of redundant code with no functional change?
=== modified file 'mysql-test/lib/My/ConfigFactory.pm' --- mysql-test/lib/My/ConfigFactory.pm 2010-04-28 12:52:24 +0000 +++ mysql-test/lib/My/ConfigFactory.pm 2010-08-02 20:33:34 +0000
+ my ($res, $after); + + while (m/(.*?)\@((?:\w+\.)+)(#?[-\w]+)/gi) { + my ($before, $group_name, $option_name)= ($1, $2, $3); + $after = $'; + chop($group_name); + + my $from_group= $config->group($group_name) + or croak "There is no group named '$group_name' that ", + "can be used to resolve '$option_name'"; + + my @auto_options = ( + 'port' => sub { fix_port($self, $config, $group_name, $group) }, + '#port' => sub { fix_port($self, $config, $group_name, $group) }, + ); + my $value= $from_group->value($option_name, @auto_options); + $res .= $before.$value; + } + m/\G.*/;
What is the purpose of this m// ? Probably it is left-over code from previous version that was forgotten to be deleted.
=== added file 'mysql-test/lib/My/Suite.pm' --- mysql-test/lib/My/Suite.pm 1970-01-01 00:00:00 +0000 +++ mysql-test/lib/My/Suite.pm 2010-08-02 20:33:34 +0000 @@ -0,0 +1,7 @@ +package My::Suite; + +sub config_files { () } +sub servers { () } + +bless { };
I think we need some documentation here (or elsewhere, but comments in this file seems a good place) of what the purpose is of this class (and derived classes in particular), and how they must be used. Eg. that a derived class should be placed in suite.pm, and explanation about what the methods should do. And the need to return an instance at the end. Etc.
=== modified file 'mysql-test/lib/mtr_cases.pm' --- mysql-test/lib/mtr_cases.pm 2010-06-14 16:58:52 +0000 +++ mysql-test/lib/mtr_cases.pm 2010-08-02 20:33:34 +0000
@@ -2471,7 +2473,7 @@ }
-sub ndbcluster_wait_started($$){ +sub ndbcluster_wait_started {
Did you intend to change this? (well, those ($$) annoy me as well, but my guess is that like me, you prefer to keep them to ease merges, but introduced this while trying different things in the code).
@@ -2639,7 +2641,7 @@
sub ndbcluster_start ($) { - my $cluster= shift; + my ($cluster) = @_;
Same as above: intended?
+ unless ($opt_start_dirty) + {
Hm, I prefer to stick to if (! ... )
@@ -3082,7 +3187,7 @@ path => $exe_mysql, args => \$args, output => '/dev/null', - error => '/dev/null' + error => '/dev/null',
Intended?
+sub fix_servers($) { + my ($tinfo) = @_; + return () unless $config; + my %servers = ( + 'mysqld.' => { + SORT => 300, + START => \&mysql_server_start, + WAIT => \&mysql_server_wait, + }, + 'mysql_cluster.' => { + SORT => 200, + START => \&ndbcluster_start, + WAIT => \&ndbcluster_wait_started, + }, + 'cluster_config.ndb_mgmd.' => { + SORT => 210, + START => undef, + }, + 'cluster_config.ndbd.' => { + SORT => 220, + START => undef, + }, + $suites{$tinfo->{suite}}->servers() + ); + for ($config->groups()) { + while (my ($re,$prop) = each %servers) { + @$_{'SORT','START'} = @$prop{'SORT','START'} if $_->{name} =~ /^$re/;
Well, the dots '.' in the keys of %servers will need to be \escaped, or they will match any char, not just '.'. (I tend to prefer not interpolating strings as regexps, rather use qr/mysql_cluster\./ to make things clearer. Or just use substr() and eq.)
@@ -4810,8 +4960,7 @@ return 1; }
- my $is_mysqld= grep ($server eq $_, mysqlds()); - if ($is_mysqld) + if ($server->name() =~ /^mysqld./)
That trailing dot in the regexp is suspicious. Did you mean \. ?
=== added directory 'mysql-test/suite/sphinx' === added file 'mysql-test/suite/sphinx/my.cnf' --- mysql-test/suite/sphinx/my.cnf 1970-01-01 00:00:00 +0000 +++ mysql-test/suite/sphinx/my.cnf 2010-08-02 20:35:41 +0000 @@ -0,0 +1,29 @@ +!include include/default_my.cnf + +[source src1] +type = xmlpipe2 +xmlpipe_command = cat suite/sphinx/testdata.xml + +[index test1] +source = src1 +docinfo = extern +charset_type = utf-8 +path = @mysqld.1.#vardir/searchd/test1 + +[indexer] +mem_limit = 32M + +[searchd] +read_timeout = 5 +max_children = 30 +max_matches = 1000 +seamless_rotate = 1 +preopen_indexes = 0 +unlink_old = 1 +log = @mysqld.1.#vardir/searchd/sphinx-searchd.log +query_log = @mysqld.1.#vardir/searchd/sphinx-query.log +#log-error = @mysqld.1.#vardir/searchd/sphinx.log +pid_file = @mysqld.1.#vardir/run/searchd.pid + +[ENV] +SPHINXSEARCH_PORT = @searchd.port
If I understand correctly, this reference in [ENV] to @searchd.port magically makes an extra option port=XXX appear in [searchd]? Now I know you didn't invent this magic in ConfigFactory ;-) But it is still very unintuitive for anyone who has not spent the time understanding ConfigFactory etc. like we had to do. I would suggest adding a comment explaining this.
=== added file 'mysql-test/suite/sphinx/sphinx.test' --- mysql-test/suite/sphinx/sphinx.test 1970-01-01 00:00:00 +0000 +++ mysql-test/suite/sphinx/sphinx.test 2010-08-02 20:35:41 +0000 @@ -0,0 +1,25 @@ +if (`SELECT "$HAVE_SPHINX" < "0000.0009.0009"`) { + skip Need Sphinx version 0.9.9 or later; +}
There is no --include have_shpinx_storage_engine_plugin.inc. Do I understand correctly that this is because the presense of --plugin-load in suite.opt automagically disables the suite if the plugin is not available?
=== added file 'mysql-test/suite/sphinx/suite.opt' --- mysql-test/suite/sphinx/suite.opt 1970-01-01 00:00:00 +0000 +++ mysql-test/suite/sphinx/suite.opt 2010-08-02 20:35:41 +0000 @@ -0,0 +1,1 @@ +--plugin-load=$HA_SPHINX_SO \ No newline at end of file
(maybe fix the missing newline while you're at it, though it's a nitpick)
=== added file 'mysql-test/suite/sphinx/suite.pm' --- mysql-test/suite/sphinx/suite.pm 1970-01-01 00:00:00 +0000 +++ mysql-test/suite/sphinx/suite.pm 2010-08-02 20:35:41 +0000 @@ -0,0 +1,116 @@ +package My::Suite_A;
Any reason for the strange name? I would expect something like My::Suite::SphinxSE
+sub locate_sphinx_binary { + my ($name)= @_; + my $res; + my @list= map "$_/bin/$name", qw(/usr /usr/local /usr/local/sphinx); + my $env_override= $ENV{"SPHINXSEARCH_\U$name"}; + @list= ($env_override) if $env_override; + for (@list) { return $_ if -x $_; } +}
(I know it was me who did it this way. But it would probably be better to check $ENV{PATH} ?)