List:Commits« Previous MessageNext Message »
From:Libing Song Date:April 29 2010 2:25am
Subject:Re: bzr commit into mysql-5.1-bugteam branch (Li-Bing.Song:3449)
Bug#49741
View as plain text  
Hi Luis and Sven,
The new patch was committed, please review it.

On Mon, 2010-04-19 at 10:17 +0100, Luís Soares wrote: 
> Hi Libing,
> 
>   Nice Work, especially because this must have been a tedious
>   work. However, I think it's a great job you're doing, as it
>   will make tests more robust.
> 
>   I have some considerations below. Please look at them and let
>   me know what do you think about them.
> 
> STATUS
> ------
>  
>   Not Approved.
> 
> REQUIRED CHANGES
> ----------------
> 
>   RC1. Fix comment in the beginning of get_relay_log_pos:
> 
>        "# $master_pos_log: The log event's position in master
>           binlog file"
> 
>        I think you mean $master_log_pos.
yes. 
> 
> 
>   RC2. The temporary file in get_relay_log_pos should be unique
>        per get_relay_log_pos usage. Otherwise, we might be in
>        trouble when running several tests in
>        parallel (--parallel)...
> 
>        You can probably resort to UUID() to help you. Have a look
>        at include/setup_fake_relay_log.inc for an example.
Done. 
> 
>   RC3. This file needs more comments on its header... Perhaps
>        something like:
> 
>        "For a given event which at position $master_log_pos in
>         the master's binary log, returns its position in the
>         slave's relay log file $relay_log_file.
> 
>         The position is stored in the variable $relay_log_pos.
> 
>         Usage:
>           let $relay_log_file= 'relay-log-bin.000001';
>           let $master_log_pos= 106;
>           source include/get_relay_log_pos.inc;
>           # at this point, get_relay_log_pos.inc sets $relay_log_pos.
> 
>           echo position in $relay_log_file: $relay_log_pos."
Done. 
> 
>    RC4. I see no parameter checking in get_relay_log_pos. Can you
>         please add something like:
> 
>         if (`SELECT '$relay_log_file' = ''`)
>         {
>           -- die ...
>         }
> 
>         Or something similar.
> 
Done. 
> 
>    RC5. I don't get this description:
> 
>         "# The relationship between master_log_pos and
>          #  relay_log_pos is that events_pos.c1 + 1 = events_at.c1."
> 
>         What is the unit for 1 ? 1 byte, 1 event ?
> 
Rewrote it. 
> 
>    RC6. There are other suites that were not taken into
>         consideration in this patch. Eg, suites/engines or
>         suites/rpl_ndb.
> 
>         I think these should be fixed as well.
Done. 
> 
>    RC7. There are still tests that depend on explicit
>         positions. A simple grep using the 'pos' still returns
>         explicit positions, for instance: 
> 
>            grep -i pos suite/rpl/r/* | less
Done. 
> 
>    RC8. In some tests, please make master_log_file to use a
>         variable as well. For isntance, in
>         mysql-test/suite/rpl/t/rpl_slave_skip.test, please change
>         so that MASTER_LOG_FILE='master-bin.000001' becomes
>         MASTER_LOG_FILE='$mysql_bin'. Something like:
> 
>         let $mysql_bin= query_get_value(SHOW MASTER STATUS, File, 1);
>         (...)
>         eval START SLAVE UNTIL MASTER_LOG_FILE='$mysql_bin',
> MASTER_LOG_POS=$master_log_pos;
>         
Done 
> 
> REQUESTS
> --------
> 
>   R1. This is a request for clarification!
> 
>       For some events, the end_log_pos in the relay log seems to
>       be the end_log_pos in the master's binary log. However, for
>       some events (slave generated events), for instance, Rotate,
>       Stop will present the end_log_pos corresponding to the
>       relay log. Won't this cause any troubles ?
I think the Rotate which you said  is master's rotate event. 
So it will not causes trouble.
> 
>   R2. This is a request for feedback.
> 
>       I don't really like the show_slave_status3.inc... Perhaps
>       we could remove the show_slave_status2.inc and have
>       show_slave_status.inc serve all purposes... Maybe we could
>       make it take some parameters so that it selectively shows
>       some fields... Have you thought of that? If not, how do you
>       feel about this approach?
Removed it. 
> 
> 
> SUGGESTIONS
> -----------
>  n/a
> 
> DETAILS 
> -------
>  n/a
> 
> On Tue, 2010-04-13 at 08:56 +0000, Li-Bing.Song@stripped wrote:
> > 
> > #At
> > file:///home/anders/work/bzrwork/worktree1/mysql-5.1-bugteam/ based on
> > revid:omer@stripped
> > 
> >  3449 Li-Bing.Song@stripped       2010-04-13
> >       Bug #49741  test files contain explicit references to
> > bin/relay-log positions
> >       
> >       Some of the test cases reference to binlog position and
> >       these position numbers are written into result explicitly.
> >       It is difficult to maintain if log event format changes. 
> >       
> >       After this patch, All the binlog position numbers which can be
> > effected 
> >       by changing binlog format are removed from the result file. 
> 


-- 
Your Sincerely,
Libing Song
==================================
MySQL Replication Team
Software Engineer


Email : Li-Bing.Song@stripped
Skype : libing.song
MSN   : slb_database@stripped
Phone : +86 010-6505-4020 ext. 319
Mobile: +86 138-1144-2038
==================================


Thread
bzr commit into mysql-5.1-bugteam branch (Li-Bing.Song:3449) Bug#49741Li-Bing.Song13 Apr
  • Re: bzr commit into mysql-5.1-bugteam branch (Li-Bing.Song:3449)Bug#49741Luís Soares19 Apr
    • Re: bzr commit into mysql-5.1-bugteam branch (Li-Bing.Song:3449)Bug#49741Libing Song29 Apr