Zhen Xing, hello.
I have not completed the entire review, still I have some comments and
open (probably just to me) issues.
I would rather value 1,3 as important to fix, although not critical.
2 is not critical and might be the beginning of a discussion.
1. status vs variables name convention on the master:
E.g Rpl_semi_sync_master_status rpl_semi_sync_master_enabled
On the slave the status and the var names are in lower-case.
I suggest to stay with the lower-case convention on either side.
Generally about the new names arrived with the sync/2 plugin,
I think `rplug_' or `rpl_plug_' as a prefix to denote a replication plugin
would be a better choice. That would facilitate to comfortable
selecting with `show status like `rplug_%' etc to get out stuff
belonging to plugins, a plugin separately from "static"
rpl_ - features.
2. rpl_semi_sync_master_timeout == 0 | > 0 could mean
rpl_semi_sync_master_enabled == FALSE | TRUE
and the slave's intent to connect semi-syncronously to be
express via CHANGE MASTER's new parameter having the meaning
of `rpl_semi_sync_slave_enabled'.
That would make `rpl_semi_sync_{master,slave}_enabled' unnecessary
(and therefore release from a dependency like
rpl_semi_sync_master_timeout == 0 => rpl_semi_sync_master_enabled == FALSE
(Just have tested: timeout == 0 and enable == TRUE can be at the
same time).
3. Attempt to set an out-of-range value succeeds.
set global rpl_semi_sync_master_timeout= -1;
show warnings >
Warning | 1292 | Truncated incorrect timeout value: '18446744073709551615'
show variables like >
rpl_semi_sync_master_timeout | 4294967295
This might be a separate and rather general issue as many other
variables tolerate an out-of-range assignments. However when some
change the current value to accept an incorrect (the out-of-range)
that is an issue.
I think there should be an error thrown as out-of-range input
value, and at least not to accept an incorrect value.
It's safer to be conservative to assume Warnings are not always read.
The same applies
rpl_semi_sync_slave_trace_level, rpl_semi_sync_master_trace_level
that must accept only positive integer values.
4. It makes sense to group the two members of
struct TranxNode {
char *log_name_;
my_off_t log_pos_;
into an existing in 6.0 struct event_coordinates (added by hb)
because the two go together most of the time, which is naturally.
5. A first look on testing makes me wondering if ratio of the new
feature code lines per the only test is fine...
Sorry, that does not sound like creative, I'll figure out the rest
tomorrow.
Open issues:
O1. How about rpl_semi_sync.test with engine= FALCON?
O2. I think there should be some interleaving with the repl heartbeat;
Have you thought on any compliction in this area? I need some more
time of digging into the sync/2 impl to be sure everything is
safe.
I am going to work hard tomorrow morning to finish it up.
cheers,
Andrei
> #At file:///media/sdb2/hezx/work/mysql/bzrwork/semisync/6.0-rpl/ based on
> revid:zhenxing.he@stripped
>
> 2858 He Zhenxing 2009-05-31
> WL#1720 Semi-synchronous replication
>
> Rewrite semi-sync patch from google with the semi-synchronous
> replication interface.
>
> A mysql-test/suite/rpl/r/rpl_semi_sync.result
> A mysql-test/suite/rpl/t/rpl_semi_sync-master.opt
> A mysql-test/suite/rpl/t/rpl_semi_sync-slave.opt
> A mysql-test/suite/rpl/t/rpl_semi_sync.test
> A plugin/semisync/
> A plugin/semisync/Makefile.am
> A plugin/semisync/configure.in
> A plugin/semisync/plug.in
> A plugin/semisync/semisync.cc
> A plugin/semisync/semisync.h
> A plugin/semisync/semisync_master.cc
> A plugin/semisync/semisync_master.h
> A plugin/semisync/semisync_master_plugin.cc
> A plugin/semisync/semisync_slave.cc
> A plugin/semisync/semisync_slave.h
> A plugin/semisync/semisync_slave_plugin.cc
> ...
