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

[He Zhenxing]

Hi,

Thanks!

Sven Sandberg wrote:
> 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.
> 

right, forget to delete

> [...]
> > --- 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.
> 

agree

> Should `packet' be const?
> 

It should not, plugins can modify the packet content

> > +                           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.
> 

agree

> [...]
> === 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.
> 

right

> > +
> > +  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;
> >  }
> [...]
>