MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:He Zhenxing Date:January 26 2011 7:54am
Subject:Re: bzr commit into mysql-trunk branch (hezx:3481) Bug#12133
View as plain text  
He Zhenxing wrote: 
> anders wrote:
> > Great work!
> > Please find my review comments below.
> > 
> 
> Thank you for the nice comments, I have changed accordingly, please look
> for the reply below and review the new patch.
> 
> > STATUS
> > ------
> >  
> >   Not Approved.
> > 
> > REQUIRED CHANGES
> > ----------------
> > RC1. A little change on find_log_pos can make the code simple.
> > 
> > int MYSQL_BIN_LOG::find_log_pos(LOG_INFO *linfo, const char *log_name,          
>                                                                                           
>        
> >                                 bool need_lock).
> > The argument 'log_name' should be defined as file name without path.
> > It is reasonable as on most of the occasions the callers
> > know the file name, but not the path. They don't care the path.
> > eg. slaves send only the log file name to master when connecting.
> >     Only binary log file name is need in PURGE LOGS, SHOW BINLOG
> > EVENTS and SHOW RELAYLOG EVENTS statements.
> >     
> 
> There are cases that the log_name passed in will have the directory
> path, Although it's possible to change all those cases to strip the
> directory path before calling this function, but I think it's better to
> make this function can handle both cases.
> 
> > And This will simplify the code. I found some cases that make_log_name()
> > is called to expand the file name to 'path+name' before calling find_log_pos().
> > This code can be removed if find_log_pos is refactored.
> > 
> 
> Yes, these should be removed.
> 
> > It is better to define the log file names in relay_log_info as file name without
> path. 
> > relay_log_info.group_relay_log_name relay_log_info.event_relay_log_name.
> > 
> 
> I think this should already be done by the patch, if not, please point
> out, I'll have a look.
> 

I think I have found the places, I'll fix all of them, and also I think
now it's OK to make find_log_pos() assume log_name have not directory
part.

Will commit a new patch soon that will address all your comments.



Thread
bzr commit into mysql-trunk branch (hezx:3481) Bug#12133He Zhenxing17 Jan
  • Re: bzr commit into mysql-trunk branch (hezx:3481) Bug#12133anders19 Jan
    • Re: bzr commit into mysql-trunk branch (hezx:3481) Bug#12133He Zhenxing26 Jan
      • Re: bzr commit into mysql-trunk branch (hezx:3481) Bug#12133He Zhenxing26 Jan
  • Re: bzr commit into mysql-trunk branch (hezx:3481) Bug#12133Luís Soares19 Jan