List:Commits« Previous MessageNext Message »
From:Ingo Strüwing Date:May 4 2010 1:06pm
Subject:Re: bzr commit into mysql-5.1 branch (Rafal.Somla:3450)
View as plain text  
Hi Rafal,

the patch is good to push. I just have minor suggestions. Please see below.

Rafal Somla, 04.05.2010 13:58:

>  3450 Rafal Somla	2010-05-04
>       Common infrastructure for testing MEB tools.
>       This patch contains infrastructure for invoking
>       MEB tools from test scripts.
>      @ mysql-test/include/

I'd prefer a name, which tells better, what the file does.
Yes, it does also check, if we have meb available, but its main part is
to set up for meb use. So perhaps

>         File to be included by meb tests. Test will be skipped if
>         IBBACKUP or INNOBACKUP variables are not set, will fail if
>         they do not point at the tools. Also setups backup directory
>         and config file for ibbackup tool.
>         NOTE: PATH setting is temporary and needs to be reworked.
>      @ mysql-test/suite/meb/my.cnf
>         Configuration for server instance used in meb
>         tests. Innodb options used by MEB tools are 
>         explicitly set because these tools do not know
>         the details. Relevant settings are also exported to
>         the environment.
>      @ mysql-test/suite/meb/t/ibb_example.test

Don't we want to start all our test names with meb_ ?

>         Example showing how to use ibbackup tool in a test.
>      @ mysql-test/suite/meb/t/inb_example.test
>         Example showing how to use innobackup in a test.
>     added:
>       mysql-test/include/
>       mysql-test/suite/meb/my.cnf
>       mysql-test/suite/meb/t/ibb_example.test
>       mysql-test/suite/meb/t/inb_example.test
> === added file 'mysql-test/include/'
> --- a/mysql-test/include/	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/include/	2010-05-04 11:58:37 +0000
> @@ -0,0 +1,52 @@
> +--disable_query_log
> +
> +# Check that we have MEB setup
> +

Comments are often a matter of taste. I find this form to stand out better:

# comment

> +if (`select '$IBBACKUP'='' OR '$INNOBACKUP'=''`)
> +{
> +--skip You must set IBBACKUP and INNOBACKUP variables to run this test.
> +}
> +
> +--enable_query_log

I would prefer to have --{dis|en}able_query_log tied together with what
needs it to turned off temporarily. That is, I'd remove some blank lines.

> +
> +--perl
> +if (! -x "$ENV{'IBBACKUP'}")
> +{
> +  die "Can't find ibbackup tool at \"$ENV{'IBBACKUP'}\"\n";
> +}
> +if (! -f "$ENV{'INNOBACKUP'}")

That's fine for now. But as soon as innobackup is a program, it would be
better to use -x here too. IMHO it won't hurt to do it already now.

> +{
> +  die "Can't find innobackup tool at \"$ENV{'INNOBACKUP'}\"\n";
> +}
> +EOF
> +

I'd place a comment here, telling that the setup part starts.

> +let $INNOBACKUP= perl -w $INNOBACKUP --ibbackup=$IBBACKUP;

That's fine for now. But as soon as innobackup is a program, it won't
work any more. IMHO we can live without "perl -w" already now (not 100%
sure about Windows though). If it doesn't work without "perl -w", we
will have a problem when testing different versions on innobackup later.


Would be nice to have a comment, explaining, how this file is made, for
later reference (so that it isn't forgotten).

> +let BACKUP_DIR=  $TMP_DIR/backup;
> +let BACKUP_CNF= $TMP_DIR/ib.cnf;
> +
> +# FIXME: make it portable (windows) and work also in binary distribution
> +# where clients are placed in a different path.
> +#
> +let PATH=../client:$PATH;
> +
> +--perl
> +
> +$backup_dir= "$ENV{BACKUP_DIR}";
> +
> +unless (-d $backup_dir)
> +{
> +  mkdir($backup_dir) or die "Can't create backup directory \"$backup_dir\"\n";
> +}

To my experience, innobackup creates this directory, if necessary. Not
sure about ibbackup though. But as it can't hurt to create it in
advance, that was just a "FYI".

> +

I'd like to have a group comment here, saying that BACKUP_CNF is created
for restore purposes.

> +open (CNF, ">$ENV{BACKUP_CNF}") or die "Can't write to \"$ENV{BACKUP_CNF}\"\n";

Ingo Strüwing, Database Group
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Geschäftsführer: Jürgen Kunz                     HRB München 161028
bzr commit into mysql-5.1 branch (Rafal.Somla:3450)Rafal Somla4 May
  • Re: bzr commit into mysql-5.1 branch (Rafal.Somla:3450)Ingo Strüwing4 May
    • Re: bzr commit into mysql-5.1 branch (Rafal.Somla:3450)Rafal Somla4 May
      • Re: bzr commit into mysql-5.1 branch (Rafal.Somla:3450)Ingo Strüwing4 May
  • Re: bzr commit into mysql-5.1 branch (Rafal.Somla:3450)Ingo Strüwing5 May