maria-developers team mailing list archive

[Kristian Nielsen <knielsen@xxxxxxxxxxxxxxx>]

Jonas Oreland <jonaso@xxxxxxxxxx> writes:

Thanks for the MDEV-162 patch! Here is my review.

I think the patch looks fine, and we should take it. Just a few questions
below, and one minor comment:


> === modified file 'mysql-test/suite/rpl/t/rpl_semi_sync.test'
> --- mysql-test/suite/rpl/t/rpl_semi_sync.test	2014-08-07 16:06:56 +0000
> +++ mysql-test/suite/rpl/t/rpl_semi_sync.test	2014-10-20 10:52:49 +0000
> @@ -59,7 +59,6 @@
>  echo [ status of semi-sync on master should be ON even without any semi-sync slaves ];
>  show status like 'Rpl_semi_sync_master_clients';
>  show status like 'Rpl_semi_sync_master_status';
> ---replace_result 305 304
>  show status like 'Rpl_semi_sync_master_yes_tx';

I'm curious why you decided to remove this --replace_result?
I see in bzr history that Monty added this --replace_result without any
explanation why... :-/


> === added file 'mysql-test/suite/rpl/t/rpl_semi_sync_wait_point.opt'
> --- mysql-test/suite/rpl/t/rpl_semi_sync_wait_point.opt	1970-01-01 00:00:00 +0000
> +++ mysql-test/suite/rpl/t/rpl_semi_sync_wait_point.opt	2014-10-20 09:25:13 +0000
> @@ -0,0 +1,1 @@
> +--log_bin

Any reason not to use --source include/have_log_bin.inc instead?


> === added file 'mysql-test/suite/rpl/t/rpl_semi_sync_wait_point.test'
> --- mysql-test/suite/rpl/t/rpl_semi_sync_wait_point.test	1970-01-01 00:00:00 +0000
> +++ mysql-test/suite/rpl/t/rpl_semi_sync_wait_point.test	2014-10-20 09:25:13 +0000

> +--echo # Kill the waiting thread; it should die immediately.
> +KILL @other_connection_id;
> +
> +--echo # Collect the error from the INSERT thread; it should be disconnected.
> +connection other;
> +--error 2013

Please use here:

  --error 2013,ER_CONNECTION_KILLED

to avoid a rare test failure (apparently the ER_CONNECTION_KILLED can
occasionally reach the client before the socket is closed, I've seen it in our
Buildbot).

> +--echo # Collect the error from the INSERT thread; it should be disconnected.
> +connection other;
> +--error 2013

Also here use --error 2013,ER_CONNECTION_KILLED


> === modified file 'sql/replication.h'
> --- sql/replication.h	2014-08-07 16:06:56 +0000
> +++ sql/replication.h	2014-10-20 09:25:13 +0000

> @@ -114,7 +117,13 @@
>  */
>  enum Binlog_storage_flags {
>    /** Binary log was sync:ed */
> -  BINLOG_STORAGE_IS_SYNCED = 1
> +  BINLOG_STORAGE_IS_SYNCED = 1,
> +
> +  /** First(or alone) in a group commit */
> +  BINLOG_GROUP_COMMIT_LEADER = 2,
> +
> +  /** Last(or alone) in a group commit */
> +  BINLOG_GROUP_COMMIT_TRAILER = 4
>  };

What is the reason for introducing these flags?
As far as I can see from the patch, they are set, but never read?


Thanks,

 - Kristian.