List:Commits« Previous MessageNext Message »
From:Luís Soares Date:November 2 2010 4:10pm
Subject:Re: bzr commit into mysql-next-mr branch (andrei.elkin:3190) Bug#57893
View as plain text  
Hi Andrei,

   Nice Work.  Please find my review comments below. I have a few
   suggestions but not something that would prevent you from
   pushing. Please, when pushing to main, remove the entry from
   mysql-next-mr.push as we will be adding it to other
   collection (as discussed previously).

STATUS
------

   Approved.

REQUIRED CHANGES
----------------
  n/a

REQUESTS
--------
  n/a

SUGGESTIONS
-----------

   S1. I would suggest to enforce the assertion in the test case
       like this:

       set @save_binlog_checksum= @@global.binlog_checksum;
       set @@global.binlog_checksum = default;
       if (`select @@global.binlog_checksum <> NONE`)
       {
          --echo Although we set the default for global.binlog_checksum, it does has not
changed!
          --die
       }

   S2. I would use a collection for the feature tree only:

       1. copy mysql-next-mr.push to mysql-next-mr-wl2540.push
       2. add to mysql-next-mr-wl2540.push the extra tests one
          wants in the feature tree.

   S3. I would probably make use of mtr to run rpl,binlog tests
       with CRC and not the checksum.pl script. Something
       like (note the absence of setting the binlog_format and no
       tests are skipped as well with --skip-test-list=):

       +perl mysql-test-run.pl --timer --force --parallel=auto
--experimental=collections/default.experimental 
--comment=rpl_binlog_checksum --vardir=var-rpl_binlog_checksum
--mysqld=--binlog-checksum=CRC32  --suite=binlog,rpl

DETAILS
-------
  n/a

On 11/02/2010 03:20 PM, Andrei Elkin wrote:
> #At file:///home/andrei/MySQL/BZR/2a-23May/WL/mysql-next-mr-wl2540/ based on
> revid:andrei.elkin@stripped
>
>   3190 Andrei Elkin	2010-11-02
>        Bug #57893  rpl_row_ignorable_event fails when --binlog-checksum=CRC32
>
>        There were two issues with the failure:
>
>        1. The reported explicitly  CRC32 `val' in the header needs filtering out via
>           --replace_regex
>
>        2. More serious is that at verbose mode of event decoding the checksum
> trailing part
>           of Rows-log-event:s was not cut off so the decoder decides there is more
> data.
>
>        Fixed with deploying --replace_regex in the test and correcting the actual
> event size
>        inside the verbose decoder of Log_event::print_base64().
>
>        Side effects of this patch include:
>        forcing PB2 to run rpl,binlog suites with checksum per push additionally to
> the default no-checksum.
>        and fixes for binlog_checksum_basic.
>       @ mysql-test/suite/rpl/t/rpl_row_ignorable_event.test
>          replace_regex-ing  CRC32 `val' to empty in the header to comply with the
> no-checksum
>          version results of the test.
>       @ sql/log_event.cc
>          correcting the actual event size of Rows-events in case of checksum is
> present.
>
>      modified:
>        mysql-test/collections/mysql-next-mr.push
>        mysql-test/suite/rpl/t/rpl_row_ignorable_event.test
>        mysql-test/suite/sys_vars/r/binlog_checksum_basic.result
>        mysql-test/suite/sys_vars/t/binlog_checksum_basic.test
>        sql/log_event.cc
> === modified file 'mysql-test/collections/mysql-next-mr.push'
> --- a/mysql-test/collections/mysql-next-mr.push	2010-10-27 10:47:25 +0000
> +++ b/mysql-test/collections/mysql-next-mr.push	2010-11-02 15:20:42 +0000
> @@ -1,6 +1,6 @@
> +perl suite/rpl/extension/checksum.pl --timer --force --parallel=auto
> --experimental=collections/default.experimental --comment=rpl_binlog_checksum
> --vardir=var-rpl_binlog_checksum --suite=rpl,binlog --percent=99
>   perl mysql-test-run.pl --timer --force --parallel=auto
> --experimental=collections/default.experimental --comment=n_mix         
> --vardir=var-n_mix                        --mysqld=--binlog-format=mixed 
> --suite=main,binlog,innodb,federated,rpl,sys_vars,perfschema
> --skip-test-list=collections/disabled-per-push.list
>   perl mysql-test-run.pl --timer --force --parallel=auto
> --experimental=collections/default.experimental --comment=ps_row        
> --vardir=var-ps_row         --ps-protocol --mysqld=--binlog-format=row   
> --suite=main,binlog,innodb,federated,rpl,sys_vars,perfschema
> --skip-test-list=collections/disabled-per-push.list
>   perl mysql-test-run.pl --timer --force --parallel=auto
> --experimental=collections/default.experimental --comment=embedded      
> --vardir=var-emebbed        --embedded                                   
> --suite=main,binlog,innodb,federated,rpl,sys_vars,perfschema
>   perl mysql-test-run.pl --timer --force --parallel=auto
> --experimental=collections/default.experimental --comment=rpl_binlog_row
> --vardir=var-rpl_binlog_row               --mysqld=--binlog-format=row   
> --suite=rpl,binlog --skip-test-list=collections/disabled-per-push.list
>   perl mysql-test-run.pl --timer --force --parallel=auto
> --experimental=collections/default.experimental --comment=funcs_1       
> --vardir=var-funcs_1                                                      --suite=funcs_1
> -perl suite/rpl/extension/checksum.pl --timer --force --parallel=auto
> --experimental=collections/default.experimental --comment=rpl_binlog_checksum
> --vardir=var-rpl_binlog_checksum --suite=rpl,binlog
>
> === modified file 'mysql-test/suite/rpl/t/rpl_row_ignorable_event.test'
> --- a/mysql-test/suite/rpl/t/rpl_row_ignorable_event.test	2010-08-06 02:20:44 +0000
> +++ b/mysql-test/suite/rpl/t/rpl_row_ignorable_event.test	2010-11-02 15:20:42 +0000
> @@ -73,7 +73,7 @@ source include/show_binlog_events.inc;
>   --echo # MYSQL_BINLOG output base on master-bin.000001
>   let $MYSQLD_DATADIR= `select @@datadir`;
>   --replace_result $MYSQLTEST_VARDIR MYSQLTEST_VARDIR
> ---replace_regex /TIMESTAMP=[0-9]*/TIMESTAMP=t/ /#[0-9]*[ ]*[0-9]*:[0-9]*:[0-9]*
> server id [0-9]*/#server id #/ /exec_time=[0-9]*/exec_time=#/
> /error_code=[0-9]*/error_code=#/ /end_log_pos [0-9]*/end_log_pos #/ /# at [0-9]*/# at #/
> /Xid = [0-9]*/Xid = #/ /thread_id=[0-9]*/thread_id=#/ /table id [0-9]*/table id #/ /mapped
> to number [0-9]*/mapped to number #/ /server v [^ ]*/server v #.##.##/ /Start: binlog v
> [0-9]*/Start: binlog v#/ /created [0-9]*[ ]*[0-9]*:[0-9]*:[0-9]* at startup/created #
> #:#:# at startup/
> +--replace_regex /TIMESTAMP=[0-9]*/TIMESTAMP=t/ /#[0-9]*[ ]*[0-9]*:[0-9]*:[0-9]*
> server id [0-9]*/#server id #/ /exec_time=[0-9]*/exec_time=#/
> /error_code=[0-9]*/error_code=#/ /end_log_pos [0-9]*/end_log_pos #/ /# at [0-9]*/# at #/
> /Xid = [0-9]*/Xid = #/ /thread_id=[0-9]*/thread_id=#/ /table id [0-9]*/table id #/ /mapped
> to number [0-9]*/mapped to number #/ /server v [^ ]*/server v #.##.##/ /Start: binlog v
> [0-9]*/Start: binlog v#/ /created [0-9]*[ ]*[0-9]*:[0-9]*:[0-9]* at startup/created #
> #:#:# at startup/ /[	 ]CRC32 0x[abcdef0-9]+//
>   --exec $MYSQL_BINLOG --base64-output=decode-rows -v -v
> $MYSQLD_DATADIR/$master_binlog
>
>   let $binlog_start= query_get_value(SHOW MASTER STATUS, Position, 1);
>
> === modified file 'mysql-test/suite/sys_vars/r/binlog_checksum_basic.result'
> --- a/mysql-test/suite/sys_vars/r/binlog_checksum_basic.result	2010-10-28 17:09:41
> +0000
> +++ b/mysql-test/suite/sys_vars/r/binlog_checksum_basic.result	2010-11-02 15:20:42
> +0000
> @@ -1,10 +1,10 @@
>   set @save_binlog_checksum= @@global.binlog_checksum;
> +set @@global.binlog_checksum = default;
>   select @@global.binlog_checksum as 'must be NONE by default';
>   must be NONE by default
>   NONE
>   select @@session.binlog_checksum as 'no session var';
>   ERROR HY000: Variable 'binlog_checksum' is a GLOBAL variable
> -set @@global.binlog_checksum = default;
>   set @@global.binlog_checksum = CRC32;
>   set @@global.binlog_checksum = CRC32;
>   set @@global.master_verify_checksum = 0;
>
> === modified file 'mysql-test/suite/sys_vars/t/binlog_checksum_basic.test'
> --- a/mysql-test/suite/sys_vars/t/binlog_checksum_basic.test	2010-10-28 17:09:41
> +0000
> +++ b/mysql-test/suite/sys_vars/t/binlog_checksum_basic.test	2010-11-02 15:20:42
> +0000
> @@ -4,12 +4,12 @@
>   # all checksum related system variables.
>
>   set @save_binlog_checksum= @@global.binlog_checksum;
> +set @@global.binlog_checksum = default;
>
>   select @@global.binlog_checksum as 'must be NONE by default';
>   --error ER_INCORRECT_GLOBAL_LOCAL_VAR
>   select @@session.binlog_checksum as 'no session var';
>
> -set @@global.binlog_checksum = default;
>
>   # testing lack of side-effects in non-effective update of binlog_checksum:
>   set @@global.binlog_checksum = CRC32;
>
> === modified file 'sql/log_event.cc'
> --- a/sql/log_event.cc	2010-10-25 19:02:24 +0000
> +++ b/sql/log_event.cc	2010-11-02 15:20:42 +0000
> @@ -2338,6 +2338,9 @@ void Log_event::print_base64(IO_CACHE* f
>     if (print_event_info->verbose)
>     {
>       Rows_log_event *ev= NULL;
> +    if (checksum_alg != BINLOG_CHECKSUM_ALG_UNDEF&&
> +        checksum_alg != BINLOG_CHECKSUM_ALG_OFF)
> +      size-= BINLOG_CHECKSUM_LEN; // checksum is displayed through the header
>
>       if (ptr[4] == TABLE_MAP_EVENT)
>       {
>
>
>
>
>

Thread
bzr commit into mysql-next-mr branch (andrei.elkin:3190) Bug#57893Andrei Elkin2 Nov
  • Re: bzr commit into mysql-next-mr branch (andrei.elkin:3190) Bug#57893Luís Soares2 Nov