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.
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.
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."
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
suites/rpl_ndb.
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',
MASTER_LOG_POS=$master_log_pos;
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 ?
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?
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.
