Rafal,
Since this is a very large patch, it would be inefficient for me to make my
comments interdelinia. I have prepared the following lists instead.
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/42571 only.
Chuck
Minor changes needed (may be placed as low priority)
----------------------------------------------------
* We determined in Orlando that this is correct. Please strike comment.
+ TODO: check if this is correct
+ */
+ DBUG_PRINT("backup",("version %d",MYSQL_VERSION_ID));
+ server_version.major= MYSQL_VERSION_ID / 10000;
+ server_version.minor= (MYSQL_VERSION_ID % 10000) / 100;
+ server_version.release= MYSQL_VERSION_ID % 100;
+ server_version.extra.begin= (byte*)MYSQL_SERVER_VERSION;
+ server_version.extra.end= server_version.extra.begin
* Here's another hard coded seemingly arbitrary value. Please use #define or
similar.
+ for (uint no=0; no<256; ++no)
* Spacing violations for operators.
+ return no+1;
Changes needed (med/high priority)
----------------------------------
* The changes to the cmake file and the makefile do not agree. Please make
sure all changes to the makefile are reflected in the cmake file. It appears
that the restore_info is different. I could verify this if I could compile.
* Please make version number a #define or something similar which can be
changed without respect to hard coded numbers in the code itself.
* You're using uint for counts but ulong for position. Please change all to
ulong.
+ uint table_count() const;
+ uint db_count() const;
Questions
---------
* Why is this comment in the code? Please remove.
+ field_list.push_back(new
Item_empty_string(STRING_WITH_LEN("backup_id")));
//op_str.c_ptr(), op_str.length()));
+ protocol->send_fields(&field_list, Protocol::SEND_NUM_ROWS |
Concerns (require addressing as condition of approval if given)
---------------------------------------------------------------
* Are we finished with the code shuffling? This sort of massive
reorganization causes a great deal of problems for simultaneous work. I
would recommend the existing kernel and associated drivers et. al. be frozen
for any changes other than adding new functionality (like dependency
checking, synch with replication, etc.) and bug fixes.
