Rafal,
Here are my comments. I would like you to respond to my comments. I think I
should also see new patches with the corrections. I have one overriding
request: please make all patches compatible with the current backup tree so
that I can compile and test the code.
Note: This review is for patch http://lists.mysql.com/commits/42481 only.
Chuck
Minor changes needed (may be placed as low priority)
----------------------------------------------------
* Please don't place case statements on a single line. They are too hard to
read this way.
+ case BSTREAM_IT_DB: meta_err= ER_BACKUP_GET_META_DB; break;
+ case BSTREAM_IT_TABLE: meta_err= ER_BACKUP_GET_META_TABLE; break;
* Please combine asserts into one call. This will help greatly when
debugging on Windows.
+ DBUG_ASSERT(obj);
DBUG_ASSERT(obj->m_obj_ptr);
Changes needed (med/high priority)
----------------------------------
* I really don't like this rename. We have a real problem server-wide with
poorly worded error messsages. This change exasperates this. A user will
ask, "what's a execution context? I'm trying to backup!" Furthermore, the
backup progress facility is going to change. I recommend reversing this
change for this particular error message and let the rework of the progress
tables handle any changes needed.
-Error 1651 Can't open the online backup progress tables. Check
'mysql.online_backup' and 'mysql.online_backup_progress'.
+Error 1675 Cannot create backup/restore execution context
* Mistyped error messages. The second one needs to be changed to reflect the
type of error -- tables instead of databases.
+ER_BACKUP_CANT_RESTORE_DB
+ eng "Could not restore database %-.64s"
+ER_BACKUP_CANT_RESTORE_TABLE
+ eng "Could not restore database %-.64s"
Questions
---------
* Why are all of these TODO's removed? Are the errors being handled
somewhere else? Please explain.
if (m_dbs.insert(pos,db))
- {
- // TODO: report error
return NULL;
- }
* Why is there now a fatal_error() method? Is there an error method that is
not fatal? I don't understand the reasons for changing the name --
report_error() seemed a much better choice.
Concerns (require addressing as condition of approval if given)
---------------------------------------------------------------
* Please use this error message for logging concerns and restore the
original backup progress error message (see above).
+ER_BACKUP_LOGGER_INIT
+ eng "Could not initialize logging and reporting services"
* This error message needs mending. It is too confusing.
+ER_BACKUP_CREATE_BE
+ eng "Cannot create backup engine for storage engine %-.64s"
I recomment this version:
+ER_BACKUP_CREATE_BE
+ eng "Cannot create backup engine instance for table referenced by
storage engine %-.64s"
