Andrei Elkin wrote:
> Zhen Xing, hello.
>
> I have not completed the entire review, still I have some comments and
> open (probably just to me) issues.
>
OK, I think we do not need to solve all problems right now, we only need
to address the critical issues, and can handle other non-critical issues
after push.
> 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.
>
The convention for MySQL is that system variables are in lower-case, and
for status variables, the first letter is in upper-case. I think we
should follow that convention.
There is only one status variable on slave: Rpl_semi_sync_slave_status,
which the first letter is in upper-case, all others are system
variables.
About the use prefix 'rplug_' or 'rpl_plug_', I think it's not necessary
to mention 'plug' in the name, because:
1) we have storage plug-ins, and do not have 'plug' in those variable
names
2) I don't think we need to distinct variables defined by plug-in and
sever kernel. If we later move a feature out of the kernel and make it a
plug-in, I don't think we should also change the variable names.
> 2. rpl_semi_sync_master_timeout == 0 | > 0 could mean
> rpl_semi_sync_master_enabled == FALSE | TRUE
>
I think using two variable is more explicit.
> 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'.
>
I don't think these components added any new parameter for CHANGE
MASTER.
> 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).
>
Maybe we could make rpl_semi_sync_master_timeout == 0 means wait for
ever.
> 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.
>
Yes, this is a problem, but since it's a general problem, I think it
should be handled as a server bug.
> 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.
>
These components are supposed to be independent from server, and should
not use any server internal structures(THD, MASTER_INFO, etc). only the
replication interface defined by WL#4398, interfaces defined in plugin.h
and cient API should be used.
>
> 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.
>
You're right, that is only a simple feature test, and WL#4873 is
supposed to test this more completely.
>
> Open issues:
>
> O1. How about rpl_semi_sync.test with engine= FALCON?
I have not did a lot of test, the patch is supposed to be engine
independent.
> 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 don't see any problem with rpl hb, the event generated by hb is not
written into binary log, and will not cause a wait on master and there
will be not reply from slave for hb events.
>
> I am going to work hard tomorrow morning to finish it up.
>
Thanks!
> 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
> > ...
