Thanks Chuck for the review. I will make sure to address your issues.
Some comments inline.
--
Øystein
Chuck Bell wrote:
> Hi,
>
> I have comments and questions below.
>
> Chuck
>
>> 2632 Oystein Grovlen 2008-06-09
>> BUG#36586 Online backup stream library can miss end of a stream.
>> Make sure end-of-stream is detected.
>> added:
>> mysql-test/r/backup_multi_blocks.result
>> mysql-test/t/backup_multi_blocks.test
>> modified:
>> mysql-test/lib/mtr_report.pl
>> sql/backup/stream_v1_transport.c
>
> Commit email has 2 bug reports on it. Which one does this patch go with?
> Both? The scrubber didn't attach the commit to the bug report. I am
> reviewing this for BUG#36586.
Yes, the scrubber managed to pick up the wrong bug number. Both reports
are for 36586. The change for the last report was to edit the filter
for master.err.
> So when BUG#36624 is fixed the patch will correct the test, right?
Yes.
>
> All backup tests should have at least the following in the preamble. This is
> because backup is not included in embeddded.
>
> --source include/not_embedded.inc
>
> Similarly, if you need a debug build you should include this (but I don't
> think you need it for this patch).
>
> --source include/have_debug.inc
>
> Required addition: Please add the following. We do this in case another test
> (or this one) doesn't cleanup properly. Always good to start every test with
> this sort of thing.
>
> --disable_warnings
> DROP DATABASE IF EXISTS mysqltest;
> --enable_warnings
>
>> +create database mysqltest;
>> +use mysqltest;
>> +
>> +CREATE TABLE t1 (a longtext) engine=myisam;
> A word on style...once upon a time I used to type in lowercase until I was
> told all commands in tests should be UPPERCASE and only identifiers should
> be lowercase. So the above should be:
>
> CREATE DATABASE mysqltests;
> USE mysqltest;
>
> CREATE TABLE t1 (a LONGTEXT) ENGINE=MYISAM;
>
Thanks, I will follow your advice here.
>> +
>> +use mysqltest;
>> +insert into t1 values ("text");
>> +let $1=13;
>> +while ($1)
>> +{
>> + update t1 set a=concat(a,a);
>> + dec $1;
>> +}
>> +select length(a) from t1;
>> +
>> +--replace_column 1 #;
>> +BACKUP DATABASE mysqltest TO 'test.ba';
>> +
>> +DROP DATABASE mysqltest;
>> +
>> +--error 1698
>> +RESTORE FROM 'test.ba';
>> +
>> +select length(a) from t1;
>> +checksum table t1;
>> +drop database mysqltest;
>
> Please make use of the comments '#' and add, at a minimum, a short
> description of the test that includes the rationale and basic things the
> test is, er, testing.
>
> Lastly, please use the --echo command to print out steps for debugging
> purposes. For example, this is an excerpt from another test that prints out
> statements letting the person reviewing the result file to see what is about
> to happen (or should happen).
>
> # Set the breakpoint for the commit blocker.
> --echo con5: Getting lock on commit blocker.
> SELECT get_lock("backup_commit_blocker", 0);
I will make sure to document this better, and look into where it makes
sense to make some tracing. If I understand you correctly the main
reason for using --echo is to able easily map between the test file and
the result file (since comments does not appear in the result file.)
>> - as_read(&s->stream,data,buf);
>> + if (as_read(&s->stream, data, buf) == BSTREAM_EOS) s->state= EOS;
>
> Style issue: Please format the code as follows:
>
> if (something)
> do_something();
>
> Please see coding guidelines at:
>
> http://forge.mysql.com/wiki/MySQL_Internals_Coding_Guidelines
The previous projects I have worked on has discouraged this form since
it is error-prone wrt to adding more statements to the if-blocks. I
will split it across two lines, and add braces if don't mind.
--
Øystein
