maria-developers team mailing list archive

[Kristian Nielsen <knielsen@xxxxxxxxxxxxxxx>]

Sergei Golubchik <serg@xxxxxxxxxxxx> writes:

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

I weakly suggest "yes".

The patch does not introduce any interface changes to mysql-test-run.pl, so
probably this one is less important to have identical in all versions.

But it seems to me still preferable to have mysql-test-run as identical as
possible between versions, and I don't see that it could hurt.

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

No, I agree with your change now, thanks for the explanation ;-)

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

No, never mind (I actually prefer your version better also).

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

Ok. (maybe it's not important, lots of Perl code does this and it usually
works as it's not likely that we will ever see a string that wrongly matches
due to '.' rather than '\.'. Since it is a common mistake, I tend to look out
for it).

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

I think you misunderstood me. This check (`SELECT "$HAVE_SPHINX" <
"0000.0009.0009"`) is for the version of the external sphinx binaries. But we
also need to skip the test case if the sphinx storage engine is not built.

So my question is, if MariaDB is built without the spinxs storage engine, but
sphinx binaries of appropriate version are present, how do we make sure to not
attempt to run the test, and fail it because the --plugin-load in suite.opt
fails?

In fact this check must also be in lib/mtr_cases.pm, as we cannot even run the
testcase to give it the chance to skip when we cannot start the server due to
missing plugin .so. For example there is such a hardcoded check for oqgraph in
mtr_cases.pm.

But I didn't find any code for this (generic or sphinxSE specific)?

 - Kristian.