Jorgen,
Thanks very much for the review. Your assessment summary was excellent!
I tried to create a new patch but my bzr is crashing with a 'float division'
error. I will get a new patch tomorrow morning. I have attached a diff in
case you want to look at the changes I've made.
Responding inline.
> Requirements
> ------------
> 1. backup_errors writes warnings to var/log/warnings [1]. This warning
> should be ignored
Modified mtr_report.pl (code was there but needed altering).
> 2. Add a test for the backup id codepath that reads the if from file
Done. See backup_logs.test. Uses debug insertion to ensure deterministic
results.
> 3. Fix backup<->restore operation bug [2]
Done. Renumbered the BACKUP enum in logger.h and corrected the code a bit
(had a bug).
> 4. Test funcs_1/is_columns_mysql fails. You seem to have missed
> the latin1 -> utf8 change in the table definition that was
> pushed last week. The mysql_system_tables.sql file indicates
> that utf8 is in, so maybe you only need to do a make clean
Corrected. I had this but missed it on the bzr commit.
> 5. backup_logs_output.test: history log is on when echo says it's
> off, progress log is on when echo says it's off.
Eagle eyes! Fix for #3 above fixed this.
> 6. backup_logs_output.test: the --file_exists tests for *.log
> does not test that any information goes into the log files. If
> possible, add a test for this. Disregard otherwise
I put a grep command in. Seems to work ok. See the test for details.
> Suggestions
> -----------
> 1. backup_logs.test does a "DROP DATABASE IF EXISTS backup_logs;
> DROP TABLE backup_logs.t1_res;". Wouldn't it be better to drop
> table before database?
Removed extra DROP TABLE (not needed if you drop the database).
> 2. logger.h@236 String str is not used and can be removed.
Removed.
> 3. Since this patch touches a lot of server code, I suggest you
> do a complete test run of all suites if you haven't already.
Done. Found 1 other test: log_state.
Chuck
