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

[Alfranio Correia]

He Zhenxing wrote:
> Alfranio Correia wrote: 
>> 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.
>>
> 
> Good
> 
>>> 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.
>>
> 
> OK
> 
>>> 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.
>>
> 
> OK, I'm fine with this too.
> 
>>> By this way, I think you can
>>> avoid lots of the copy & paste work of the patch. 
>> Can you elaborate on this?
> 
> I think you can avoid moving init_master_info() into
> Master_info::init_info(), and flush_master_info() into
> Master_info::flush_info(), etc. Just wrapper around these functions when
> implementing the new interfaces.


I was just trying to organize the code.
Eventually, we will need to do that. :)
What do you think Luis?


>> 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.
>>
> 
> I think bzr can fail to do automatic merge if we have moved the code
> from one place to another. By reducing moving code from original
> functions to new functions, I think we can make the automatic merge work
> more smoothly.



> 
>>>
>>>
>> Cheers.
>>
> 
> 
>