MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Chuck Bell Date:June 26 2007 11:51pm
Subject:RE: bk commit into 5.2 tree (rafal:1.2523)
View as plain text  
Rafal,

Patch looks good. Just some general comments.

General Comments (applies to all 4 patches)
-------------------------------------------
I've noticed you don't follow the same doxygen block format everywhere. Some
places have:

/**
  * My comment
  * My comment
  */

While others have:

/**
   My comment
   My comment
  */ 

Which is correct? Please change all comment blocks so they use the same
format. BTW: If the second one is the correct way, let me know so I can
change my code too! ;)

Some single line comments use // instead of /*   */.

Please remove extra *'s from block comments that use the format:

/***********************
  My comment
 ***********************/

There are some misspellings in the bk comments, e.g. "writting."

Concerns
--------
I was not able to apply the patch to my copy of the tree in either Windows
or Linux. I am concerned because I would have liked to see the compiler
warnings (you know there will be gobs on Windows) so that we could work on
those at each patch rather than trying to fix them all after several patches
are applied. The concern is compounded warnings that could be eliminated
more quickly using more discrete patch application.

Chuck  

> -----Original Message-----
> From: rsomla@stripped [mailto:rsomla@stripped] 
> Sent: Monday, June 25, 2007 6:24 AM
> To: commits@stripped
> Subject: bk commit into 5.2 tree (rafal:1.2523)
> 
> Below is the list of changes that have just been committed 
> into a local
> 5.2 repository of rafal. When rafal does a push these changes 
> will be propagated to the main repository and, within 24 
> hours after the push, to the public repository.
> For information on how to access the public repository see 
> http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
> 
> ChangeSet@stripped, 2007-06-25 12:23:43+02:00, rafal@quant.(none) +6 -0
>   WL#3904 (Online Backup: Error Handling):
>   
>   Define API for generating error and other types of messages during
>   backup/restore process. The API is encapsulated in 
> backup::Logger class.
> 
>   sql/CMakeLists.txt@stripped, 2007-06-25 12:23:40+02:00, 
> rafal@quant.(none) +1 -1
>     Add backup/logger.cc to mysqld sources.
> 
>   sql/backup/Makefile.am@stripped, 2007-06-25 12:23:40+02:00, 
> rafal@quant.(none) +5 -2
>     Add logger.{h,cc} to backup library.
> 
>   sql/backup/error.h@stripped, 2007-06-25 12:23:40+02:00, 
> rafal@quant.(none) +34 -0
>     Helper function report_mysql_error().
> 
>   sql/backup/error.h@stripped, 2007-06-25 12:23:40+02:00, 
> rafal@quant.(none) +0 -0
> 
>   sql/backup/logger.cc@stripped, 2007-06-25 12:23:40+02:00, 
> rafal@quant.(none) +81 -0
>     Implementation of Logger class.
> 
>   sql/backup/logger.cc@stripped, 2007-06-25 12:23:40+02:00, 
> rafal@quant.(none) +0 -0
> 
>   sql/backup/logger.h@stripped, 2007-06-25 12:23:40+02:00, 
> rafal@quant.(none) +123 -0
>     Declaration of Logger class.
> 
>   sql/backup/logger.h@stripped, 2007-06-25 12:23:40+02:00, 
> rafal@quant.(none) +0 -0
> 
>   sql/share/errmsg.txt@stripped, 2007-06-25 12:23:40+02:00, 
> rafal@quant.(none) +85 -0
>     Add backup related error messages.
> 
> # This is a BitKeeper patch.  What follows are the unified 
> diffs for the # set of deltas contained in the patch.  The 
> rest of the patch, the part # that BitKeeper cares about, is 
> below these diffs.
> # User:	rafal
> # Host:	quant.(none)
> # Root:	/ext/mysql/bk/backup/alpha
> 
> --- 1.146/sql/share/errmsg.txt	2007-06-25 12:23:48 +02:00
> +++ 1.147/sql/share/errmsg.txt	2007-06-25 12:23:48 +02:00
> @@ -6073,3 +6073,88 @@
>          eng "Cannot convert to non-transactional lock in an 
> active transaction on '%-.64s'"
>          ger "In einer laufenden Transaktion ist die 
> Umwandlung zu nicht-transaktionalen Sperren verboten. 
> Betrifft '%-.64s'"
>  
> +ER_NO_STORAGE_ENGINE
> +        eng "Can't access storage engine of table %-.64s"
> +
> +ER_BACKUP_BACKUP_START
> +        eng "Starting backup process"
> +ER_BACKUP_BACKUP_DONE
> +        eng "Backup completed"
> +ER_BACKUP_RESTORE_START
> +        eng "Starting restore process"
> +ER_BACKUP_RESTORE_DONE
> +        eng "Restore completed"
> +
> +ER_BACKUP_BACKUP
> +        eng "Error during backup operation - server's error 
> log contains more information about the error"
> +ER_BACKUP_RESTORE
> +        eng "Error during restore operation - server's error 
> log contains more information about the error"
> +ER_BACKUP_BACKUP_PREPARE
> +        eng "Error when preparing for backup operation"
> +ER_BACKUP_RESTORE_PREPARE
> +        eng "Error when preparing for restore operation"
> +ER_BACKUP_INVALID_LOC
> +        eng "Invalid backup location '%-.64s'"
> +ER_BACKUP_READ_LOC
> +        eng "Can't read backup location '%-.64s'"
> +ER_BACKUP_WRITE_LOC
> +        eng "Can't write to backup location '%-.64s'"
> +ER_BACKUP_LIST_DBS
> +        eng "Can't enumerate server databases"
> +ER_BACKUP_LIST_TABLES
> +        eng "Can't enumerate server tables"
> +ER_BACKUP_LIST_DB_TABLES
> +        eng "Can't enumerate tables in database %-.64s"
> +ER_BACKUP_READ_HEADER
> +        eng "Can't read backup archive preamble"
> +ER_BACKUP_WRITE_HEADER
> +        eng "Can't write backup archive preamble"
> +ER_BACKUP_NO_BACKUP_DRIVER
> +        eng "Can't find backup driver for table %-.64s"
> +ER_BACKUP_NOT_ACCEPTED
> +        eng "%-.64s backup driver was selected for table 
> %-.64s but it rejects to handle this table"
> +ER_BACKUP_CREATE_BACKUP_DRIVER
> +        eng "Can't create %-.64s backup driver"
> +ER_BACKUP_CREATE_RESTORE_DRIVER
> +        eng "Can't create %-.64s restore driver"
> +ER_BACKUP_TOO_MANY_IMAGES
> +        eng "Found %d images in backup archive but maximum 
> %d are supported"
> +ER_BACKUP_WRITE_META
> +        eng "Error when saving meta-data of %-.64s"
> +ER_BACKUP_READ_META
> +        eng "Error when reading meta-data list"
> +ER_BACKUP_CREATE_META
> +        eng "Can't create %-.64s"
> +ER_BACKUP_GET_BUF
> +        eng "Can't allocate buffer for image data transfer"
> +ER_BACKUP_WRITE_DATA
> +        eng "Error when writing %-.64s backup image data 
> (for table #%d)"
> +ER_BACKUP_READ_DATA
> +        eng "Error when reading data from backup stream"
> +ER_BACKUP_NEXT_CHUNK
> +        eng "Can't go to the next chunk in backup stream"
> +
> +ER_BACKUP_INIT_BACKUP_DRIVER
> +        eng "Can't initialize %-.64s backup driver"
> +ER_BACKUP_INIT_RESTORE_DRIVER
> +        eng "Can't initialize %-.64s restore driver"
> +ER_BACKUP_STOP_BACKUP_DRIVER
> +        eng "Can't shut down %-.64s backup driver"
> +ER_BACKUP_STOP_RESTORE_DRIVERS
> +        eng "Can't shut down %-.64s backup driver(s)"
> +ER_BACKUP_PREPARE_DRIVER
> +        eng "%-.64s backup driver can't prepare for synchronization"
> +ER_BACKUP_CREATE_VP
> +        eng	"%-.64s backup driver can't create its image 
> validity point"
> +ER_BACKUP_UNLOCK_DRIVER
> +        eng "Can't unlock %-.64s backup driver after 
> creating the validity point"
> +ER_BACKUP_CANCEL_BACKUP
> +        eng "%-.64s backup driver can't cancel its backup operation"
> +ER_BACKUP_CANCEL_RESTORE
> +        eng "%-.64s restore driver can't cancel its restore 
> operation"
> +ER_BACKUP_GET_DATA
> +        eng "Error when polling %-.64s backup driver for its 
> image data"
> +ER_BACKUP_SEND_DATA
> +        eng "Error when sending image data (for table #%d) 
> to %-.64s restore driver"
> +ER_BACKUP_SEND_DATA_RETRY
> +        eng "After %d attempts %-.64s restore driver still 
> can't accept next block of data"
> 
> --- 1.2/sql/backup/Makefile.am	2007-06-25 12:23:48 +02:00
> +++ 1.3/sql/backup/Makefile.am	2007-06-25 12:23:48 +02:00
> @@ -24,13 +24,16 @@
>  	-I$(top_srcdir)/regex
>  
>  libbackup_la_SOURCES = \
> -	stream.cc
> +	stream.cc \
> +	logger.cc
>  
>  noinst_HEADERS = \
>  	api_types.h \
>  	backup_engine.h \
>      debug.h \
> -    stream.h 
> +    error.h \
> +    stream.h \
> +    logger.h
>  
>  DEFS = \
>  	-DMYSQL_SERVER \
> --- New file ---
> +++ sql/backup/error.h	07/06/25 12:23:40
> #ifndef _BACKUP_ERROR_H
> #define _BACKUP_ERROR_H
> 
> namespace util {
> 
> /**
>   Report error stored in MYSQL_ERROR structure to a client.
> 
>   If @c err doesn't contain any error code, the given error 
> code is reported.
> 
>   @returns 0 if error was reported, non-zero otherwise.
>  */
> inline
> int report_mysql_error(THD*, MYSQL_ERROR *err, int code= 0) {
>   DBUG_ASSERT(err);
> 
>   if (err->level == MYSQL_ERROR::WARN_LEVEL_END
>       && !err->msg && !err->code ) // err doesn't store any
> error
>     return -1;
> 
>   switch (err->level) {
> 
>   case MYSQL_ERROR::WARN_LEVEL_ERROR:
>     return my_printf_error(err->code ? err->code : code, 
> err->msg, MYF(0));
> 
>   default: // Q: What to do with warnings and notes? push them... ?
>     return -1;
>   }
> }
> 
> } // util namespace
> 
> #endif
> 
> --- New file ---
> +++ sql/backup/logger.cc	07/06/25 12:23:40
> #include "../mysql_priv.h"
> 
> #include "logger.h"
> 
> namespace backup {
> 
> 
> /**
>   Output message on a given level.
> 
>   This is the low-level method used by all other methods to 
> output messages.
>   It's implementation determines how messages are delivered 
> to the user.
>   Currently they are appended to the server's error log.
> 
>   @param level       level of the message (INFO,WARNING,ERROR)
>   @param error_code  optional code for message coming from 
> errmsg.txt database -
>                      for other messages set to 0
>   @param msg         message text
> 
>   @returns 0 on success.
>  */
> int Logger::write_message(log_level::value level, int error_code,
>                           const char *msg) {
>    switch (level) {
>    case log_level::ERROR:
>      if (m_save_errors)
>        errors.push_front(new MYSQL_ERROR(::current_thd,error_code,
>                                          
> MYSQL_ERROR::WARN_LEVEL_ERROR,msg));
>      sql_print_error(msg);
>      DBUG_PRINT("backup log",("[ERROR] %s",msg));
>      return 0;
> 
>    case log_level::WARNING:
>      sql_print_warning(msg);
>      DBUG_PRINT("backup log",("[Warning] %s",msg));
>      return 0;
> 
>    case log_level::INFO:
>      sql_print_information(msg);
>      DBUG_PRINT("backup log",("[Info] %s",msg));
>      return 0;
> 
>    default: return -1;
>    }
> }
> 
> /**
>   Output message registered in errmsg.txt database.
> 
>   @param level       level of the message (INFO,WARNING,ERROR)
>   @param error_code  code assigned to the message in errmsg.txt
> 
>   If the message contains placeholders, additional arguments provide
>   values to be put there.
> 
>   @returns 0 on success.
>  */
> int Logger::v_report_error(log_level::value level, int 
> error_code, va_list args) {
>   return v_write_message(level,error_code,ER_SAFE(error_code),args);
> }
> 
> /**
>   Output unregistered message.
> 
>   Format string is given explicitly as an argument.
> 
>   Note: no localization support.
>  */
> int Logger::v_write_message(log_level::value level, int error_code,
>                             const char *format, va_list args) {
>   char buf[ERRMSGSIZE + 20];
> 
>   my_vsnprintf(buf,sizeof(buf),format,args);
>   return write_message(level,0,buf);
> }
> 
> 
> } // backup namespace
> 
> --- New file ---
> +++ sql/backup/logger.h	07/06/25 12:23:40
> #ifndef _BACKUP_LOGGER_H
> #define _BACKUP_LOGGER_H
> 
> #include <backup/error.h>
> 
> 
> namespace backup {
> 
> /// Logging levels for messages generated by backup system 
> struct log_level {  enum value {
>     INFO=    MYSQL_ERROR::WARN_LEVEL_NOTE,
>     WARNING= MYSQL_ERROR::WARN_LEVEL_WARN,
>     ERROR=   MYSQL_ERROR::WARN_LEVEL_ERROR
>  };
> };
> 
> 
> /**
>   This class exposes API used to output various messages 
> during backup/restore
>   process.
> 
>   Destination of the messages is determined by the 
> implementation. Currently
>   messages are output the the servers error log.
> 
>   Messages are intended for a DBA and thus should be 
> localized. A message should
>   be registered in errmsg.txt database and have assigned an 
> error code. Printing
>   message corresponding to an error code is done using @c 
> report_error() methods.
>  */
> class Logger
> {
>  public:
> 
>    Logger(): m_save_errors(FALSE)
>    {}
> 
>    int write_message(log_level::value level , int error_code, 
> const char *msg);
> 
>    int write_message(log_level::value level, const char *msg, ...)
>    {
>      va_list args;
> 
>      va_start(args,msg);
>      int res= v_write_message(level, 0, msg, args);
>      va_end(args);
> 
>      return res;
>    }
> 
>    /// Reports error with log_level::ERROR.
>    int report_error(int error_code, ...)
>    {
>      va_list args;
> 
>      va_start(args,error_code);
>      int res= v_report_error(log_level::ERROR, error_code, args);
>      va_end(args);
> 
>      return res;
>    }
> 
>    /// Reports error with registered error description string.
>    int report_error(log_level::value level, int error_code, ...)
>    {
>      va_list args;
> 
>      va_start(args,error_code);
>      int res= v_report_error(level, error_code, args);
>      va_end(args);
> 
>      return res;
>    }
> 
>    /// Reports error with given description.
>    int report_error(const char *format, ...)
>    {
>      va_list args;
> 
>      va_start(args,format);
>      int res= v_write_message(log_level::ERROR, 0, format, args);
>      va_end(args);
> 
>      return res;
>    }
> 
>    ///  Request that all reported errors are saved in the logger.
>    void save_errors()
>    {
>      if (m_save_errors)
>        return;
>      clear_saved_errors();
>      m_save_errors= TRUE;
>    }
> 
>    /// Stop saving errors.
>    void stop_save_errors()
>    {
>      if (!m_save_errors)
>        return;
>      m_save_errors= FALSE;
>    }
> 
>    /// Delete all saved errors to free resources.
>    void clear_saved_errors()
>    { errors.delete_elements(); }
> 
>    /// Return a pointer to most recent saved error.
>    MYSQL_ERROR *last_saved_error()
>    { return errors.head(); }
> 
>  private:
> 
>   List<MYSQL_ERROR> errors;  ///< Used to store saved errors
>   bool m_save_errors;        ///< Flag telling if errors 
> should be saved
> 
>   int v_report_error(log_level::value,int,va_list);
>   int v_write_message(log_level::value,int, const char*,va_list); };
> 
> 
> } // backup namespace
> 
> #endif
> 
> 
> --- 1.40/sql/CMakeLists.txt	2007-06-25 12:23:48 +02:00
> +++ 1.41/sql/CMakeLists.txt	2007-06-25 12:23:48 +02:00
> @@ -73,7 +73,7 @@
>                 partition_info.cc rpl_utility.cc 
> rpl_injector.cc sql_locale.cc
>                 rpl_rli.cc rpl_mi.cc sql_servers.cc
>                 sql_connect.cc scheduler.cc
> -               backup/stream.cc
> +               backup/stream.cc backup/logger.cc
>                 ${PROJECT_SOURCE_DIR}/sql/sql_yacc.cc
>    			   ${PROJECT_SOURCE_DIR}/sql/sql_yacc.h
>  			   ${PROJECT_SOURCE_DIR}/include/mysqld_error.h
> 
> --
> MySQL Code Commits Mailing List
> For list archives: http://lists.mysql.com/commits
> To unsubscribe:    
> http://lists.mysql.com/commits?unsub=1
> 

Thread
bk commit into 5.2 tree (rafal:1.2523)rsomla25 Jun
  • RE: bk commit into 5.2 tree (rafal:1.2523)Chuck Bell27 Jun