Re: bzr commit into mysql-6.0 branch (ogrovlen:2632) Bug#36586, Bug#36624

[Øystein Grøvlen]

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