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.
> 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.
> 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.
> 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.
> 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."
> 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.
> 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 ?
> RC6. There are other suites that were not taken into
> consideration in this patch. Eg, suites/engines or
> I think these should be fixed as well.
> 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
> 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',
> 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?
> 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.
MySQL Replication Team
Email : Li-Bing.Song@stripped
Skype : libing.song
MSN : slb_database@stripped
Phone : +86 010-6505-4020 ext. 319
Mobile: +86 138-1144-2038