List:Commits« Previous MessageNext Message »
From:anders Date:February 12 2011 6:56am
Subject:Re: bzr commit into mysql-trunk branch (hezx:3481) Bug#12133
View as plain text  
Hi Zhexing,

Great work!
Please find my review comments below.

STATUS
------
  Not Approved.

REQUIRED CHANGES
----------------
RC1. 
> -  @param log_name    Filename to find in the index file.
> +  @param log_name       Filename to find in the index file.
Please don't change this line.

RC2.
> @@ -2002,9 +2002,10 @@ int MYSQL_BIN_LOG::find_log_pos(LOG_INFO linfo
>  {
linfo->log_file_name stores a value including either path or not. it
depends on the file name stored in the index file. As it is copied from
the index file. It is a bit complex to handle. So I think we should
remove the path part in linfo->log_file_name if it exists.

It is same to find_next_log().

RC3. 
> @@ -2682,7 +2691,9 @@ int MYSQL_BIN_LOG::purge_index_entry(THD
> if (!mysql_file_stat(key_file_binlog, log_info.log_file_name, &s,
MYF(0)))

log_info.log_file_name may not include path, so we need more code to add
its path.

> -      if ((error= find_log_pos(&check_log_info, log_info.log_file_name,
> need_mutex)))
> +      if ((error= find_log_pos(&check_log_info,
> +                               log_info.log_file_name +
> dirname_length(log_info.log_file_name),
if you agree above comment for find_log_pos(), log_info.log_file_name
never includes path.

> @@ -4182,6 +4196,14 @@ int MYSQL_BIN_LOG::open(const char *opt_
>        goto err;
>      }
>  
> +    if (!dirname_length(log_name))
> +    {
> +      int dir_len= dirname_length(opt_name);
> +      if (dir_len >= FN_REFLEN)
> +        dir_len=FN_REFLEN-1;
> +      strmake(log_name+dir_len, log_name, FN_REFLEN - dir_len -1);
> +      strnmov(log_name, opt_name, dir_len);
> +    }
Same to above.  "+    if (!dirname_length(log_name))" should be removed.

RC4. 
> @@ -672,7 +671,7 @@ void mysql_binlog_send(THD* thd, char* l
>      heartbeat_ts= &heartbeat_buf;
>      set_timespec_nsec(*heartbeat_ts, 0);
>      coord= &coord_buf;
> -    coord->file_name= log_file_name; // initialization basing on what slave
> remembers
> +    coord->file_name= linfo.log_file_name; // points to the current binary log
> file name
The line is too long, please move the comment above the code.

RC5.
If I understand correctly, MYSQL_BIN_LOG::log_file_name is a full path name.
So we have to remove the path before registering it into the purge index file.
  > register_create_index_entry(log_file_name)

RC6.
In show_binlogs(),
> while ((length=my_b_gets(index_file, fname, sizeof(fname))) > 1)
fname doesn't include the path. So the path should be set in before opening
the file.

RC7.
> +--echo # Move the master binlog files and the index file to a new place
> +--move_file $master_datadir/master-bin.000001 $tmpdir/master-bin.000001
> +--move_file $master_datadir/master-bin.000002 $tmpdir/master-bin.000002
> +--move_file $master_datadir/master-bin.index  $tmpdir/master-bin.index
> +
> +--echo # Restart master with log-bin option set to the new path
> +--let $rpl_server_parameters=--log-bin=$tmpdir/master-bin
> +source include/rpl_start_server.inc;
> +
> +--echo # Master has restarted successfully
Please test the following queries here.
show binary log;
show binlog events in 'a file before current file';

> +--echo # Move back the master binlog files
> +--move_file $tmpdir/master-bin.000001 $master_datadir/master-bin.000001
> +--move_file $tmpdir/master-bin.000002 $master_datadir/master-bin.000002
> +
> +--echo # Remove the unneeded master-bin.index file
> +--remove_file $tmpdir/master-bin.index
> +
> +--echo # Restart master with log-bin option set to default
> +--let $rpl_server_parameters=--log-bin=$master_datadir/master-bin
> +source include/rpl_start_server.inc;
> +
> +--echo # Master has restarted successfully
Please test the following queries here.
show binary log;
show binlog events in 'a file before current file';

RC8. I think we should ask QA to test it. 

REQUESTS
--------
                                                                                          
                                                                              

SUGGESTIONS
-----------
SU1. Would you like to do refactoring on the code?
> @@ -4173,7 +4187,7 @@ int MYSQL_BIN_LOG::open(const char *opt_
>  
>      do
>      {
> -      strmake(log_name, log_info.log_file_name, sizeof(log_name)-1);
> +      make_log_name(log_name, log_info.log_file_name);
>      } while (!(error= find_next_log(&log_info, 1)));
If I understand correctly. The code is used to find the last line of
index file. It is not need to call make_log_name for every log_name.
and we probably can find a better algorithm to find the last line.

It is really strange that I found no code calls this function.
Could you please check the code and remove this function if it is true?

SU2. Please comment that this code is for compatibility. After this
path, group_relay_log_name doesn't include the path.
> +  /* Remove the directory part */
> +  int dirlen= dirname_length(group_relay_log_name);
> +  if (dirlen)
> +  {
> +    int len= strlen(group_relay_log_name);
> +    memmove(group_relay_log_name, group_relay_log_name + dirlen, len
- dirlen);
> +    group_relay_log_name[len-dirlen]= '\0';
> +  }
> +


-- 
Your Sincerely,
Libing Song
==================================
MySQL Replication Team
Software Engineer

Email : Anders.Song@stripped
Skype : libing.song
MSN   : slb_database@stripped
Phone : +86 010-6505-4020 ext. 319
Mobile: +86 138-1144-2038
==================================


Thread
bzr commit into mysql-trunk branch (hezx:3481) Bug#12133He Zhenxing26 Jan
  • Re: bzr commit into mysql-trunk branch (hezx:3481) Bug#12133anders10 Feb
  • Re: bzr commit into mysql-trunk branch (hezx:3481) Bug#12133anders12 Feb