Brandon Nesterenko <[email protected]> writes:
> diff --git a/sql/log.h b/sql/log.h
> index deb3c306d0c..6423610c061 100644
> --- a/sql/log.h
> +++ b/sql/log.h
> +/* 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
Ack.
> +/* 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).
Ok.
> 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.
Like this?
/*
Given a slave connection position (containing potentially multiple
GTIDs), return the last node in the list of pending semi-sync transactions
that is contained in the slave position, if any.
*/
Tranx_node *
Active_tranx::find_latest_semi_sync_trx(slave_connection_state *state)
> @@ -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?
I don't think so.
This code is specifically for the initial slave connect. IIUC, the code
wants to make it so that when the slave connects starting after a specific
transaction, the connect will implicitly ack that transaction (as well as
any prior transactions). I suppose the reasoning is that if the slave
receives transaction T, but then the connection is lost before receiving the
ack, then we want the ack to be recovered immediately at reconnect, and not
have to wait until another transaction gets sent and ack'ed.
That seems to be needed independently of master_wait_no_slave.
However, this check is specifically because when get_master_enabled()
returns false, the m_active_tranxs pointer is NULL (it could also have been
an explicit NULL check). If the Active_tranx class was folded into
Repl_semi_sync_master, then this check would not be needed at all, the list
of nodes would always be available (though empty when semi-sync is
disabled).
- Kristian.
_______________________________________________
developers mailing list -- [email protected]
To unsubscribe send an email to [email protected]
