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/have_meb.inc


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 setup_meb.inc?

>         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/have_meb.inc
>       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/have_meb.inc'
> --- a/mysql-test/include/have_meb.inc	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/include/have_meb.inc	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.

> +let $SERVER_CNF= $MYSQLTEST_VARDIR/my.cnf;


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


Regards
Ingo
-- 
Ingo Strüwing, Database Group
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Geschäftsführer: Jürgen Kunz                     HRB München 161028
Thread
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