Re: bzr commit into mysql-5.1-rep+3 branch (alfranio.correia:3123) WL#5125

[Alfranio Correia]

Hi Jasonh,

Thank you for the review.
See some comments in-line.

He Zhenxing wrote:
> Hi, Alfranio
> 
> Thank you for the nice work!
> 
> STATUS
> ______
> 
> NOT APPROVED
> 
> REQUIRED
> ________
> 
> 1) The 'rollback' interface of the 2pc protocol is missing.

Sure. Silly me.

> 
> 2) The 'prepare' and 'rollback' should be called in appropriate
> places (e.g. call prepare before writing events, call commit after
> successfully written, and call rollback if there is error in writing),
> although they will be empty functions right now, but they will
> have definition if we implemented WL#2275/WL#3970. So that when later
> implementing WL#2275/WL#3970, we do not need to change the logic flow
> any more, just implementing the strategies in the corresponding classes,
> and then tell the server to use those objects.

I thought it would be better to postpone this to the WL#3970 but
I will do it as part of the current patch.

> 3) The copyright information in the newly added files are
> incorrect, I think the following should be used:
> 
>   Copyright (C) 2009 Sun Microsystems, Inc.

Ok.

> 
> 4) Please add Doxygen commments for the interfaces added.
> 
> http://forge.mysql.com/wiki/MySQL_Internals_Coding_Guidelines#Commenting_Code

Ok.

> REQUESTS
> ________
> 
> 1) Please considering use aggregation instead of inherite. 

I and Luis also share the same idea and I only implemented it because Mats has
requested inheritance.

Now we are three requesting aggregation, so I will do it.

> Do not
> change the hierachy of Master_info and Relay_log_info, make
> Master_info/Relay_log_info a member of
> Master_info_file/Relay_log_info_file. 

I don't think this should be done like this.
I think we should "inject" the *_file classes into the *_info classes.


> By this way, I think you can
> avoid lots of the copy & paste work of the patch. 

Can you elaborate on this?

I can only think about what follows:

private:
  virtual int do_check()= 0;
  virtual int do_init_info()= 0;
  virtual int do_prepare_info()= 0;
  virtual int do_flush_info(const bool force)= 0;
  virtual void do_end_info()= 0;
  virtual int do_reset_info()= 0;

  Rpl_info& operator=(const Rpl_info& info);
  Rpl_info(const Rpl_info& info);
};

How could I avoid that?

> The patch would be
> much simpler and easier to understand, and meanwhile this can help
> when we merge patches from older versions that does not include this
> re-factor work. 

Can you elaborate on this?
I am wondering what we can do avoid merge problems.

> 
> 
> 

Cheers.