List:Commits« Previous MessageNext Message »
From:He Zhenxing Date:June 11 2009 5:11am
Subject:Re: bzr commit into mysql-6.0-rpl branch (zhenxing.he:2858) WL#1720
View as plain text  
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
> >     ...

Thread
bzr commit into mysql-6.0-rpl branch (zhenxing.he:2858) WL#1720He Zhenxing31 May
  • Re: bzr commit into mysql-6.0-rpl branch (zhenxing.he:2858) WL#1720Alfranio Correia7 Jun
    • Re: bzr commit into mysql-6.0-rpl branch (zhenxing.he:2858) WL#1720Alfranio Correia9 Jun
      • Re: bzr commit into mysql-6.0-rpl branch (zhenxing.he:2858) WL#1720Alfranio Correia10 Jun
        • Re: bzr commit into mysql-6.0-rpl branch (zhenxing.he:2858) WL#1720He Zhenxing10 Jun
          • Re: bzr commit into mysql-6.0-rpl branch (zhenxing.he:2858) WL#1720He Zhenxing11 Jun
    • Re: bzr commit into mysql-6.0-rpl branch (zhenxing.he:2858) WL#1720He Zhenxing10 Jun
      • Re: bzr commit into mysql-6.0-rpl branch (zhenxing.he:2858) WL#1720Alfranio Correia10 Jun
        • Re: bzr commit into mysql-6.0-rpl branch (zhenxing.he:2858) WL#1720Mats Kindahl10 Jun
          • Re: bzr commit into mysql-6.0-rpl branch (zhenxing.he:2858) WL#1720Alfranio Correia10 Jun
            • Re: bzr commit into mysql-6.0-rpl branch (zhenxing.he:2858) WL#1720Mats Kindahl10 Jun
              • Re: bzr commit into mysql-6.0-rpl branch (zhenxing.he:2858) WL#1720Alfranio Correia10 Jun
                • Re: bzr commit into mysql-6.0-rpl branch (zhenxing.he:2858) WL#1720Mats Kindahl10 Jun
  • Re: bzr commit into mysql-6.0-rpl branch (zhenxing.he:2858) WL#1720Andrei Elkin10 Jun
    • Re: bzr commit into mysql-6.0-rpl branch (zhenxing.he:2858) WL#1720Paul DuBois10 Jun
      • Re: bzr commit into mysql-6.0-rpl branch (zhenxing.he:2858) WL#1720He Zhenxing11 Jun
    • Re: bzr commit into mysql-6.0-rpl branch (zhenxing.he:2858) WL#1720He Zhenxing11 Jun
      • Re: bzr commit into mysql-6.0-rpl branch (zhenxing.he:2858) WL#1720Andrei Elkin11 Jun
        • Re: bzr commit into mysql-6.0-rpl branch (zhenxing.he:2858) WL#1720He Zhenxing11 Jun
          • Re: bzr commit into mysql-6.0-rpl branch (zhenxing.he:2858) WL#1720Andrei Elkin11 Jun
            • Re: bzr commit into mysql-6.0-rpl branch (zhenxing.he:2858) WL#1720He Zhenxing12 Jun