Hi Sven,
Thanks for your feedback. Your suggestions sounds reasonable and I will take a look at it.
There is one issue with adding the test cases for filtering by server id. I actually
initially was looking at adding some tests that included filtering by server id, however
the result was that I discovered BUG#59530, so those tests will in general not pass.
Thanks,
Jesper
On 01/03/2011, at 8:23 PM, Sven Sandberg wrote:
> Hello Jesper,
>
> Thank you for this contribution. It's a good idea to add the
> --thread-id flag to mysqlbinlog. The source patch is well written, it
> works for both SBL and RBL, and I see no immediate problems with it.
> The feature is not very intrusive either; it is unlikely to be in the
> way of possible future work such as scripted or pluggable filters.
>
> I would suggest two improvements:
>
> 1. If the binary log contains events originating from more than one
> server, then it is not safe to filter by only thread_id, because
> two unrelated threads on two different servers may have the same
> thread_id. Hence, we should generate a warning when --thread-id
> is set and --server-id is not set.
>
> 2. The test case needs to be improved to cover more cases and to
> reduce future maintenance. I will be quite rigorous and require
> several changes. Many of our existing tests don't follow such
> strict guidelines, but we try to enforce them when we add or
> update tests. This is important because we spend a lot of time
> maintaining test cases. So we need to write tests that don't need
> to be updated when something in the server changes.
>
> 1. Test that --thread-id without --server-id gives a warning.
>
> 2. Test the same thing for a binary log that contains row events
> and a binary log that contains statement events. It may be
> easiest to move the test to the mysql-test/suite/binlog/t;
> tests in that directory are run once for each binlog_format
> (you have to remove 'source
> include/have_binlog_format_statement.inc').
>
> 3. Don't assume that the binlog name is master-bin.000001. Read
> the binlog name like this:
>
> --let $binlog_file= query_get_value(SHOW MASTER STATUS, File, 1)
>
> 4. Please avoid relying on comparison with the result file. See
> the suggested fixes below: if the feature does not work as
> specified, we will get an error in diff_tables instead of a
> result file mismatch. This makes the test much easier to
> maintain in the future.
>
> 5. Test mysqlbinlog with no flag, with --thread-id alone, with
> --server-id alone, and with --thread-id and --server-id
> together; and in each case with ids that exist in the binlog
> and ids that don't exist in the binlog. I'd suggest something
> like:
>
> CREATE TABLE t1 (_server_id INT, _thread_id INT);
>
> FLUSH LOGS;
> SET @@GLOBAL.SERVER_ID = 1, @@SESSION.PSEUDO_THREAD_ID = 1;
> INSERT INTO t1 SET _server_id=1, _thread_id=1;
> SET @@GLOBAL.SERVER_ID = 1, @@SESSION.PSEUDO_THREAD_ID = 2;
> INSERT INTO t1 SET _server_id=1, _thread_id=2;
> SET @@GLOBAL.SERVER_ID = 2, @@SESSION.PSEUDO_THREAD_ID = 1;
> INSERT INTO t1 SET _server_id=2, _thread_id=1;
> SET @@GLOBAL.SERVER_ID = 2, @@SESSION.PSEUDO_THREAD_ID = 2;
> INSERT INTO t1 SET _server_id=2, _thread_id=2;
> FLUSH LOGS;
>
> CREATE TABLE data SELECT * FROM t1;
> CREATE TABLE t2 LIKE t1;
>
> # Test all combinations of --server-id and --thread-id.
> --let $server_id= 0
> while ($server_id <= 3)
> {
> --let $thread_id= 0
> while ($thread_id <= 3)
> {
> --source extra/binlog_tests/binlog_mysqlbinlog_thread_id.inc
> --inc $thread_id
> }
> --inc $server_id
> }
>
> Then, create the file
> mysql-test/extra/binlog_tests/binlog_mysqlbinlog_thread_id.inc
> that contains:
>
> # ==== Purpose ====
> #
> # Auxiliary file used by binlog_mysqlbinlog_thread_id. Runs
> # mysqlbinlog [--thread-id=X] [--server-id=Y]
> # and checks that the correct statements were filtered.
> #
> # ==== Usage ====
> #
> # --let $thread_id= X
> # --let $server_id= Y
> # --source extra/binlog_tests/binlog_mysqlbinlog_thread_id.inc
> #
> # If $thread_id is set, passes --thread-id=$thread_id to
> # mysqlbinlog. If $server_id is set, passes
> # --server-id=$server_id to mysqlbinlog.
> DELETE FROM t1;
> DELETE FROM t2;
> --let $options=
> --let $where=
> if ($thread_id)
> {
> --let $options= $options --thread-id=$thread_id
> --let $where= $where _thread_id=$thread_id
> }
> if ($server_id)
> {
> --let $options= $options --server-id=$server_id
> --let $where= $where _server_id=$server_id
> }
> if ($where)
> {
> --let $where= WHERE $where
> }
> --exec $MYSQL_BINLOG $options $MYSQLD_DATADIR/$binlog_file | $MYSQL
> INSERT INTO t2 SELECT * FROM data $where;
> --let $diff_tables= t1, t2
> --source include/diff_tables.inc
>
> 6. Please add a comment to the top of the test case. Something
> like:
>
> # ==== Purpose ====
> #
> # Verify that mysqlbinlog --thread-id works as expected. The
> # flag is tested both alone and in conjuction with --server-id,
> # and both with id that exists and that don't exist in the
> # binary log. Also test that --thread-id without --server-id
> # gives a warning.
> #
> # ==== Method ====
> #
> # Create tables t1, t2, and data, with two int columns. Insert
> # the current values of pseudo_thread_id and server_id into t1
> # for some different values. Record the inserts in a separate
> # binlog. Copy the contents of t1 to data and delete all rows
> # from t1. Run mysqlbinlog with several different combinations
> # of --thread-id and --server-id and pipe the output to a
> # client. For each combination, copy the rows that we expect
> # mysqlbinlog to filter in from data to t2 and then compare
> # t1 and t2.
> #
> # ==== References ====
> #
> # BUG#59529: Add support for filtering by thread id in mysqlbinlog
>
> 7. We always guarantee that tests start from a clean state, so
> there is no need to drop tables or call RESET MASTER at the
> beginning of the test.
>
> 8. Please don't disable warnings in the test. In the end of the
> test, drop exactly the tables that were created, without
> IF EXISTS.
>
> 9. Please restore the value of SERVER_ID at the end of the test.
>
> Best regards,
> Sven Sandberg
> Software engineer
> Oracle / MySQL Replication team
