Hi Kristian,
This is the review for your GTID extensions on top of your semi-sync
refactoring. Generally, this patch is much more straight-forward and I have
less comments here.
diff --git a/sql/log.h b/sql/log.h
index deb3c306d0c..6423610c061 100644
--- a/sql/log.h
+++ b/sql/log.h
@@ -543,6 +543,8 @@ class Cache_flip_event_log: public Event_log {
#define SEMI_SYNC_SLAVE_DELAY_SYNC 1
/* Tell the io thread if the current event needs a ack. */
#define SEMI_SYNC_NEED_ACK 2
+/* Tell the io thread if the ack should use new-style GTID format. */
"new-style" will eventually be no longer new :) Perhaps just GTID-style
format
diff --git a/sql/rpl_mi.h b/sql/rpl_mi.h
index 83fb7c5b2d4..f3816201f20 100644
--- a/sql/rpl_mi.h
+++ b/sql/rpl_mi.h
@@ -310,6 +310,11 @@ class Master_info : public Slave_reporting_capability
(Full slave stop/start does not use it, as it resets the relay logs).
*/
slave_connection_state gtid_current_pos;
+ /*
+ The GTID of the event group currently being received from the master
+ (ie. the last GTID event seen).
+ */
This comment wasn't that helpful, I still had to look up where it is
updated.
Perhaps make it a bit more specific (who updates it and when).
diff --git a/sql/semisync.h b/sql/semisync.h
index 44f236606fd..011d2a692aa 100644
--- a/sql/semisync.h
+++ b/sql/semisync.h
@@ -54,9 +54,10 @@ class Repl_semi_sync_base
/* Constants in network packet header. */
static const unsigned char k_packet_magic_num;
static const unsigned char k_packet_flag_sync;
+ static const unsigned char k_packet_flag_gtid;
};
-/* The layout of a semisync slave reply packet:
+/* The layout of a semisync slave reply packet, old-style filename/offset:
1 byte for the magic num
8 bytes for the binlog positon
n bytes for the binlog filename, terminated with a '\0'
@@ -70,4 +71,14 @@ class Repl_semi_sync_base
#define REPLY_MESSAGE_MAX_LENGTH \
(REPLY_MAGIC_NUM_LEN + REPLY_BINLOG_POS_LEN + REPLY_BINLOG_NAME_LEN)
+/* The layout of a semisync slave reply packet, new-style GTID:
+ 1 byte for the magic num
+ 4 bytes for the GTID domain_id, little-endian
+ 4 bytes for the server_id
+ 8 bytes for the domain_id
+*/
This would be good in the git commit message (i.e. that there is a new
semi-sync magic flag and what it looks like).
diff --git a/sql/semisync_master.h b/sql/semisync_master.h
index ae48ec85bf6..1578dd7257b 100644
--- a/sql/semisync_master.h
+++ b/sql/semisync_master.h
@@ -655,8 +659,13 @@ class Repl_semi_sync_master
const rpl_gtid *gtid);
int dump_start(THD* thd,
- const char *log_file,
- my_off_t log_pos);
+ const char *log_file,
+ my_off_t log_pos,
+ slave_connection_state *gtid_state);
+ virtual void dump_start_inner(THD* thd,
+ const char *log_file,
+ my_off_t log_pos,
+ slave_connection_state *gtid_state) = 0;
Your previous patch uses *_sub as a convention, it'd be good to be
consistent.
diff --git a/sql/semisync_master.cc b/sql/semisync_master.cc
index dc5d49c892e..0a24aa180ee 100644
--- a/sql/semisync_master.cc
+++ b/sql/semisync_master.cc
@@ -335,6 +335,27 @@ void Active_tranx::clear_active_tranx_nodes(const
Repl_semi_sync_trx_info *inf)
DBUG_VOID_RETURN;
}
+
+Tranx_node *
+Active_tranx::find_latest(slave_connection_state *state)
This function name is vague. Both Active_trans and slave_connection_state
are
searchable, so "find_latest" finds what in which? I could easily see someone
breaking this function because of a lack of understanding what it is
supposed
to be doing.
@@ -947,20 +1034,90 @@ int Repl_semi_sync_master::dump_start(THD* thd,
...
+bool
+Repl_semi_sync_master_gtid::latest_gtid(slave_connection_state *gtid_state,
+ rpl_gtid *out_gtid)
+{
+ bool found_any_gtid= false;
+ rpl_gtid *gtid;
+ Tranx_node *tranx_node;
+
+ lock();
+
+ if (m_commit_inf_inited)
+ {
+ /* Check if the slave is connecting at our current commit point. */
+ gtid= gtid_state->find(m_commit_inf.gtid.domain_id);
+ if (gtid &&
+ gtid->server_id == m_commit_inf.gtid.server_id &&
+ gtid->seq_no == m_commit_inf.gtid.seq_no)
+ {
+ *out_gtid= m_commit_inf.gtid;
+ found_any_gtid= true;
+ goto l_found;
+ }
+ }
+
+ if (get_master_enabled())
Should rpl_semi_sync_master_wait_no_slave be in this check?
_______________________________________________
developers mailing list -- [email protected]
To unsubscribe send an email to [email protected]
