Re: [Maria-developers] wl#127 - changes to mtr for sphinx

Hi, Kristian! On Aug 04, Kristian Nielsen wrote:

Sergei Golubchik writes:

My main comment is that there should be some documentation of the My::Suite class, as I write in a comment below.

I'll add it, thanks.

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.

Now, that you've seen the patch - should I push it in 5.1 too ?

...

=== 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?

Yes.

...

=== 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.

Right, I've deleted it after I sent you a patch for review (but I had no internet access at that moment so I couldn't tell you that, sorry)

...

=== 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.

Will do.

...

=== 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).

Yes. There's a little trick here. ndbcluster_wait_started is called as a WAIT method, $server->{WAIT}->($server) and it's also called directly by another ndb-specific function. That other function uses two-argument form, the WAIT invocation - one-argument. Having ($$) prototype would not hurt, as calling a function via a reference does not check the prototype. But it would be confusing - the prototype says there should be two arguments, while we call it with one. But I think I can use $;$ prototype, if you want.

...

@@ -2639,7 +2641,7 @@

sub ndbcluster_start ($) { - my $cluster= shift; + my ($cluster) = @_;

Same as above: intended?

it's more in style with other functions, in particular other START function. I can restore the old line though it you want.

...

+ unless ($opt_start_dirty) + {

Hm, I prefer to stick to if (! ... )

ok.

...

@@ -3082,7 +3187,7 @@ path => $exe_mysql, args => \$args, output => '/dev/null', - error => '/dev/null' + error => '/dev/null',

Intended?

I'll restore it too. I've originally added one more argument there, and a comma was needed. But then I removed it.

...

+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 suppose so. But the old code was not doing it. FYI: later I found a bug in this code - it only copies START and SORT, but not WAIT (that was added later). Changed to @$_{keys %$prop} = values %$prop if $_->{name} =~ /^$re/;

(I tend to prefer not interpolating strings as regexps, rather use qr/mysql_cluster\./ to make things clearer. Or just use substr() and eq.)

I prefer to interpolate within \Q...\E :) But here qr/.../ would be better as it explicitly shows that the key is a regex.

...

@@ -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 \. ?

It's a copy from the above. Either \. in both places or in neither.

...

+[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]?

Yes.

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.

I'll try to make it clearer

...

=== 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?

No, because I've added the contents of a have_shpinx_storage_engine_plugin.inc directly into sphinx.test file. Simply because there was only one .test file, if there were many I'd keep the inc file separate.

...

=== 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)

ok.

...

=== 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

Already fixed.

...

+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} ?)

Right, I mostly copied sphinx suite changes from your patch without thinking much about them. Okay, I can use PATH. Thanks! Regards, Sergei