List:Internals« Previous MessageNext Message »
From:Sven Sandberg Date:March 1 2011 9:23am
Subject:Review of contributed patch for BUG#59529
View as plain text  
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

     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

          CREATE TABLE t1 (_server_id INT, _thread_id INT);

          FLUSH LOGS;
          INSERT INTO t1 SET _server_id=1, _thread_id=1;
          INSERT INTO t1 SET _server_id=1, _thread_id=2;
          INSERT INTO t1 SET _server_id=2, _thread_id=1;
          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/
              --inc $thread_id
            --inc $server_id

        Then, create the file
        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/
          # 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 | 
          INSERT INTO t2 SELECT * FROM data $where;
          --let $diff_tables= t1, t2
          --source include/

     6. Please add a comment to the top of the test case.  Something

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