List:Internals« Previous MessageNext Message »
From:Jesper Wisborg Krogh Date:March 6 2011 12:20am
Subject:Re: Review of contributed patch for BUG#59529
View as plain text  
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

Thread
Review of contributed patch for BUG#59529Sven Sandberg1 Mar
  • Re: Review of contributed patch for BUG#59529Jesper Wisborg Krogh6 Mar
    • Re: Review of contributed patch for BUG#59529Sven Sandberg15 Mar