STATUS
------
Patch rejected.
REQUIRED
--------
1. Insufficient testing. Test case does nothing.
2. I think the purge command should also be included along with
appropriate tests.
REQUESTS
--------
3. Comply with code formatting issues cited by Rafal.
4. More explanatory comments please.
DETAILS
-------
> BUG#47493: add 'flush-backup-logs' feature to mysqladmin
[4] This patch comment is weak and isn't even a complete sentence. You
should state the problem and its solution in this section.
> @ client/mysqladmin.cc
> Used the existing SQL command 'flush backup logs'.
[4] What does this mean?
> @ mysql-test/suite/backup/t/backup_logs_output.test
> Irrespective of log_backup_output options log files are created.
> So I have tested only for TABLE,FILE option.
> The command doesn't return anything when it succeeds.
[1] True, but the test does nothing as well (see below).
...
> === modified file 'client/mysqladmin.cc'
> --- a/client/mysqladmin.cc 2009-10-12 09:08:34 +0000
> +++ b/client/mysqladmin.cc 2009-10-21 14:16:30 +0000
> @@ -97,7 +97,7 @@ enum commands {
> ADMIN_FLUSH_HOSTS, ADMIN_FLUSH_TABLES, ADMIN_PASSWORD,
> ADMIN_PING, ADMIN_EXTENDED_STATUS, ADMIN_FLUSH_STATUS,
> ADMIN_FLUSH_PRIVILEGES, ADMIN_START_SLAVE, ADMIN_STOP_SLAVE,
> - ADMIN_FLUSH_THREADS, ADMIN_OLD_PASSWORD
> + ADMIN_FLUSH_THREADS, ADMIN_OLD_PASSWORD, ADMIN_FLUSH_BACKUP_LOGS
> };
> static const char *command_names[]= {
> "create", "drop", "shutdown",
> @@ -107,7 +107,7 @@ static const char *command_names[]= {
> "flush-hosts", "flush-tables", "password",
> "ping", "extended-status", "flush-status",
> "flush-privileges", "start-slave", "stop-slave",
> - "flush-threads","old-password",
> + "flush-threads", "old-password", "flush-backup-logs",
[2] What about purge? A complete solution for mysqladmin should include
the purge-backup-logs which is a companion command for flush-backup-logs.
...
> === modified file 'mysql-test/suite/backup/r/backup_logs_output.result'
> --- a/mysql-test/suite/backup/r/backup_logs_output.result 2009-02-06 08:28:24 +0000
> +++ b/mysql-test/suite/backup/r/backup_logs_output.result 2009-10-21 14:16:30 +0000
> @@ -237,6 +237,7 @@ SELECT count(*) FROM mysql.backup_progre
> count(*)
> 6
> FLUSH BACKUP LOGS;
> +Test flush-backup-logs from mysqladmin
[1] This result is unacceptable. There is a way to test the flush.
...
> === modified file 'mysql-test/suite/backup/t/backup_logs_output.test'
> --- a/mysql-test/suite/backup/t/backup_logs_output.test 2009-02-24 20:57:21 +0000
> +++ b/mysql-test/suite/backup/t/backup_logs_output.test 2009-10-21 14:16:30 +0000
> @@ -252,6 +252,18 @@ SELECT count(*) FROM mysql.backup_histor
> SELECT count(*) FROM mysql.backup_progress;
>
> FLUSH BACKUP LOGS;
> +
> +# BUG#47493 - add 'flush backup logs' to mysqladmin.
> +#
> +# 'flush backup logs' reopens log files.
> +# Log files are created irrespective of log_backup_output options.
> +# Test 'flush-backup-logs' from mysqladmin for log_backup_ouput=TABLE,FILE.
> +#
> +--echo Test flush-backup-logs from mysqladmin
> +
> +--exec $MYSQLADMIN --no-defaults --default-character-set=latin1 -S $MASTER_MYSOCK -P
> $MASTER_MYPORT -u root --password= flush-backup-logs 2>&1
> +
> +
[1] This test does nothing. It runs the code, yes, but it isn't being
run when the logs have anything to flush so it won't execute the 'flush'
part of the command. The flush is write out any buffered data. This
happens for the FILE output option but may be less effective for TABLE
option.
I think there are at least two solutions here.
1. The 'hard' way but most effective method:
You must construct a meaningful test case to show this code is working
correctly. To do this, you're going to need to use debug sync points to
halt the code in the middle of writing to the file.
This will require adding sync point(s) to the log writing method(s) in
log.cc and/or logger.cc.
The concept is:
1. set the sync point
2. start a backup operation that will stop in the middle of writing the log
3. dump the log contents
4. call mysqladmin to flush the logs.
5. dump the log contents (should show more data)
I admit this could be a tricky test to write especially since you are
new to the code, however, it is not impossible to write such a test.
I would begin by investigating how other backup tests (especially the
log tests) use sync points and search the tests in the main suite for
how test cases were written for flushing the query and slow logs. This
should help in writing a test case to show the flush backup logs is working.
2. The 'easy' way but less effective method:
I am not sure why this wasn't considered, but why not copy the test case
for flushing the backup logs and simply repeat it for mysqladmin? Note:
You must purge the logs before restarting the test case -- don't just
run it again like the patch does. That doesn't test anything -- there's
nothing to purge.
[2] To test the purge operation, simply conduct one or more backup or
restore operations, show the contents, execute mysqladmin to purge the
logs, then show the contents again (should be empty).
