Chuck Bell wrote:
> Rafal,
>
> The new patch works much better now on Windows. There is just one issue
> we should discuss before I approve the patch.
>
> >>> === added file
> >>> 'mysql-test/suite/backup_engines/r/backup_interruption.result'
> >>> --- a/mysql-test/suite/backup_engines/r/backup_interruption.result
> >>> 1970-01-01 00:00:00 +0000
> >>> +++ b/mysql-test/suite/backup_engines/r/backup_interruption.result
> >>> 2008-12-05 07:38:01 +0000
> >>
> >> I don't think this test needs to be in backup_engines. Please explain
> >> why you think it should be there.
> >>
> >
> > To run it with all possible backup/restore drivers, which is the main
> > idea of backup_engines test suite.
>
> I still don't see why it needs to run against all storage engines. All
> of the code being exercised is above the storage engine layer and
> therefore of no consequence. Granted there are some minor things that
> are specific to some of the engines that occur above that layer, but for
> backup or restore it isn't related to the quality of the backup code.
>
I don't think your premise that all happens above the storage engine level is
true. Or, at least, we can not be sure. We are checking interruptions and we
don't know where exactly they will be caught. For example, one of the points
where RESTORE is stopped before interruption is "restore_before_sending_data"
which is this place in data_backup.cc:
DEBUG_SYNC(thd, "restore_before_sending_data");
ret= drvr->send_data(buf);
if (log.report_killed())
goto error;
Thus the interruption will be intercepted inside driver's send_data() method and
I want to see that it is correctly handled regardless of which driver is used.
Therefore I want to keep this test in backup_engines suite.
> Please move the test to the backup suite.
>
> The other things...
>
>>>> === added file
>>>> 'mysql-test/suite/backup_engines/include/backup_restore_interrupt.inc'
>>>> ---
>>>> a/mysql-test/suite/backup_engines/include/backup_restore_interrupt.inc
>
>>>> 1970-01-01 00:00:00 +0000
>>>> +++
>>>> b/mysql-test/suite/backup_engines/include/backup_restore_interrupt.inc
>
>>>> 2008-12-05 07:38:01 +0000
>>>
>>> ...
>>>
>>>> + --connection killer
>>>> +
>>>> + --echo #
>>>> + --echo # Wait for the command to reach its synchronization point,
>>>> + --echo # then kill it.
>>>> + --echo #
>>>> + SET DEBUG_SYNC='now WAIT_FOR here';
>>>> + --replace_regex /id=[0-9]+/id=<query id>/
>>>> + eval SELECT state FROM INFORMATION_SCHEMA.PROCESSLIST WHERE id=$id;
>>>> + --replace_regex /QUERY [0-9]+/QUERY <query id>/
>>>> + eval KILL QUERY $id;
>>>> + SET DEBUG_SYNC='now SIGNAL go';
>>>> +
>>>
>>> [4] Why is this indented? It shouldn't be.
>>>
>>
>> Why do you say it should not be? I think it should to make it more
>> clear that these lines are executed from a different connection.
>
> I will not press the indentation for this review. I think we need
> clear(er) guidelines.
>
With this indentation it is much easier to see which test statements are
executed from which connection. I don't understand what problem do you have with
this...
Rafal
