maria-developers team mailing list archive

[Kristian Nielsen <knielsen@xxxxxxxxxxxxxxx>]

Sergei Golubchik <sergii@xxxxxxxxx> 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} ?)