Jonas Oreland <jonaso@google.com> 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.