Sergei Golubchik <serg@askmonty.org> 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.