maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxxx>]

Hi, Kristian!

On Aug 04, Kristian Nielsen wrote:
> Sergei Golubchik <sergii@xxxxxxxxx> 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