From: Date: November 28 2008 4:42pm
Subject: Re: bzr commit into mysql-5.1 branch (aelkin:2705) Bug#40129
List-Archive: http://lists.mysql.com/commits/60192
Message-Id: <49301151.7080903@sun.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7BIT

Hi Andrei,

I have just two suggestions:

1- You should remove from the description of the patch information on
possible
alternative implementations. Please, be more assertive.

2 - Indicate which is the current hierarchy of parameters. It is hard to
tell from the
description.

I will open an worklog for the the warning messages which should be
thrown if conflicting parameters
are defined. I will change MTR due to the new testing framework... and I
need this feature. Any problem?

Cheers.


Andrei Elkin wrote:
> #At file:///home/andrei/MySQL/BZR/FIXES/5.1-rpl-bug41003-40129-log_bin_trust_no-threads/
>
>  2705 Andrei Elkin	2008-11-27
>       Bug #40129  no-threads fails on pushbuild in 6.0-rpl, ps_stm_threadpool
>       
>       The test reacted on the way how mtr orders arguments for the server that are gathered from
>       different source. It appeared that the opt-file options were parsed before those that supplied
>       to mtr via its command line. In effect, the opt-file preferences got overriden 
>       by the command line and some tests, like no-threads, were caught by surprise: a test expects
>       an option value that had been "hardcoded" into its opt-file but gets another one.
>       
>       This server options ordering problem exists on in the new rpl trees mtr.
>       In option of the author of this patch, the opt-file shall be considered as having
>       the highest preference weight. The opt-file is merely a part of the header of a test, namely 
>       a part that can not be technically deployed along the test file.
>       Another possible option might be for a test to refuse to execute via 
>       `source have_some_option_value' guard.
>       However, this way looks complicated. Indeed, for the referred test there should be a new 
>       guard constructed that allows the test to run only if the interested to the test option
>       `thread_handling' has  a specific value `pool-of-threads'.
>       It's unnatural in opinion of this patch author for the test writer to provide 
>       both the opt file value and a guard that guarantees the value will be set on in the run time.
>       On the contrary, it's logical to provide either one: the option and its value or the guard.
>       
>       Fixed with relocating parse of the opt file to be the last among sources of the sever's options.
>       A side effect: fixing a small problem of resetting the suite options at time the opt file starts
>       parsing.
>       
>       
>        
>        
> modified:
>   mysql-test/lib/mtr_cases.pm
>
> per-file messages:
>   mysql-test/lib/mtr_cases.pm
>     Relocating parse of the opt file to be the last. This ensure the opt file is the last provider
>     for the server options so that the opt-file options have the highest preference;
>     
>     fixing a separate issue of incorrect resetting the suite options for the server;
> === modified file 'mysql-test/lib/mtr_cases.pm'
> --- a/mysql-test/lib/mtr_cases.pm	2008-11-14 20:35:32 +0000
> +++ b/mysql-test/lib/mtr_cases.pm	2008-11-27 11:20:33 +0000
> @@ -580,7 +580,7 @@ sub optimize_cases {
>  sub process_opts_file {
>    my ($tinfo, $opt_file, $opt_name)= @_;
>  
> -  $tinfo->{$opt_name}= [];
> +  ###  $tinfo->{$opt_name}= [];
>    if ( -f $opt_file )
>    {
>      my $opts= opts_from_file($opt_file);
> @@ -756,17 +756,6 @@ sub collect_one_test_case {
>    push(@{$tinfo->{'master_opt'}}, @$suite_opts);
>    push(@{$tinfo->{'slave_opt'}}, @$suite_opts);
>  
> -  # ----------------------------------------------------------------------
> -  # Add master opts, extra options only for master
> -  # ----------------------------------------------------------------------
> -  process_opts_file($tinfo, "$testdir/$tname-master.opt", 'master_opt');
> -
> -  # ----------------------------------------------------------------------
> -  # Add slave opts, list of extra option only for slave
> -  # ----------------------------------------------------------------------
> -  process_opts_file($tinfo, "$testdir/$tname-slave.opt", 'slave_opt');
> -  
> -  
>    #-----------------------------------------------------------------------
>    # Check for test specific config file
>    #-----------------------------------------------------------------------
> @@ -987,6 +976,16 @@ sub collect_one_test_case {
>    push(@{$tinfo->{'master_opt'}}, @::opt_extra_mysqld_opt);
>    push(@{$tinfo->{'slave_opt'}}, @::opt_extra_mysqld_opt);
>  
> +  # ----------------------------------------------------------------------
> +  # Add master opts, extra options only for master
> +  # ----------------------------------------------------------------------
> +  process_opts_file($tinfo, "$testdir/$tname-master.opt", 'master_opt');
> +
> +  # ----------------------------------------------------------------------
> +  # Add slave opts, list of extra option only for slave
> +  # ----------------------------------------------------------------------
> +  process_opts_file($tinfo, "$testdir/$tname-slave.opt", 'slave_opt');
> +
>    return $tinfo;
>  }
>  
>
>
>