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
==================================
