Re: bzr commit into mysql-6.0-semi-sync-1.0 tree (hezx:2635) WL#4398

[Sven Sandberg]

Hi,

Patch approved.

Just some minor comments below. Also, there are still some tabs in the 
patch, which would be good to replace by spaces.

/Sven


He Zhenxing wrote:
[...]
> --- a/include/mysql/plugin.h	2008-03-27 19:02:15 +0000
> +++ b/include/mysql/plugin.h	2008-06-25 10:17:51 +0000
[...]
> @@ -610,6 +629,145 @@
[...]
> +/**
> +   Read a packet from the connection.
> +
> +   @note The packet buffer will be allocated and freed automatically,
> +   the memory pointed to by @a packet will be overwritten or freed by
> +   the next read or write using the same @a mysql connection.
> +
> +   @param mysql    mysql client connection
> +   @param packet   return the pointer to the packet read
> +   @param len      return the length of packet read
> +
> +   @retval 0 Success
> +   @retval 1 Failure
> +   @return 0 on success, 1 on failure.

Please use either one @retval for each return value, or a single @return 
describing all the return values.

[...]
> --- a/sql/replication.h	1970-01-01 00:00:00 +0000
> +++ b/sql/replication.h	2008-06-25 10:17:51 +0000
> @@ -0,0 +1,427 @@
[...]
> +  /**
> +     This callback is called before sending an event packet to slave
> +
> +     @param param Observer common parameter
> +     @param packet Binlog event packet to send
> +     @param log_file Binlog file name of the event packet to send
> +     @param log_pos Binlog position of the event packet to send
> +
> +     @retval 0 Sucess
> +     @retval 1 Failure
> +  */
> +  int (*before_send_event)(Binlog_transmit_param *param,
> +                           unsigned char *packet,

Here, we need a packet length.

Should `packet' be const?

> +                           const char *log_file, my_off_t log_pos );
> +
> +  /**
> +     This callback is called after sending an event packet to slave
> +
> +     @param param Observer common parameter
> +     @param event_buf Binlog event packet buffer sent
> +
> +     @retval 0 Sucess
> +     @retval 1 Failure
> +   */
> +  int (*after_send_event)(Binlog_transmit_param *param,
> +                          const char *event_buf);

Here, we need a packet length.

[...]
=== modified file 'sql/sql_repl.cc'
> --- a/sql/sql_repl.cc	2008-03-08 10:14:47 +0000
> +++ b/sql/sql_repl.cc	2008-06-25 10:17:51 +0000
[...]
> @@ -378,8 +379,34 @@
>    {
>      DBUG_RETURN(-1);
>    }
> +  DBUG_RETURN(0);
> +}
> +
> +/*
> +  Reset thread transmit packet buffer for event sending
> +
> +  This function allocates header bytes for event transmission, and
> +  should be called before store the event data to the packet buffer.
> +*/
> +static int reset_transmit_packet(THD *thd, ushort flags,
> +                                 ulong *ev_offset, const char **errmsg)
> +{
> +  int ret= 0;
> +  String *packet= &thd->packet;
> +
> +  /* reserve and set default header */
> +  packet->length(0);
>    packet->set("\0", 1, &my_charset_bin);
> -  DBUG_RETURN(0);
> +  *ev_offset= 1;

The above line is now unnecessary since *ev_offset is set below.

> +
> +  if (RUN_HOOK(binlog_transmit, reserve_header, thd, flags, packet))
> +  {  
> +    *errmsg= "Failed to run hook 'reserve_header'";
> +    my_errno= ER_UNKNOWN_ERROR;
> +    ret= 1;
> +  }
> +  *ev_offset= packet->length();
> +  return ret;
>  }
[...]

-- 
Sven Sandberg, Software Engineer
MySQL AB, www.mysql.com