Re: bzr commit into mysql-6.0-backup branch (thavamuni.alagu:2888) Bug#47994

[Thava Alagu]

Hi Rafal,

     Please refer to a new patch committed now:
          http://lists.mysql.com/commits/89721


Rafal Somla wrote:
> Hi Thava,
>
> STATUS
> ------
> Not approved.
>
> REQUIRED
> --------
> 1. Forward to user errors/warnings (other than ER_QUERY_INTERRUPTED) 
> which
>    could happen when a statement is interrupted.
>
    See below.
> 2. Do not add Logger::report_state_change() method - the
>    Logger::report_state() one should do the same task.
   Agreed and done.
>
> SUGGESTIONS
> -----------
> 3. Do not add new Backup_log::set/get_state() methods. Instead fix the
>    implementation of Backup_log::state() to follow the specification 
> (report
>    only state changes) or implement logic for detecting state changes in
>    Logger::report_state().
   report_state() now implements the logic of detecting state changes.
>
> COMMENT
> -------
> I am not convinced by your argumentation that reporting additional 
> errors/warnings after query interruption is not needed or even 
> confusing. If a user hits Ctrl+C to interrupt an operation, depending 
> on the operation a complex shutdown sequence might be executed. During 
> this shutdown sequence errors might happen or warnings be generated 
> and I think the user should see them. This might require more work on 
> our side but is not impossible to implement. E.g., instead of calling 
> copy_warnings() do an explicit loop iterating over the warnings in 
> ed_connection and add only these which are different from 
> ER_QUERY_INTERRUPTED.
>
   My initial impression about this is that it is unnecessary. We run 
service interface
   queries mainly in the initial stages to obtain the metadata information.
   When these queries are interrupted, we already give high level warning
   information about what stage the interruption has occured.
   IMHO, that should be sufficient. Ofcourse, if there is
   software bug, these low level warnings will be useful for 
troubleshooting,
   but that is the job for the debug trace not user visible warnings.
   I am bit concerned about raising false alarms for the user who can not
   make much out of the low level warnings.

       Having said that I have not looked at many real test cases where 
these
   low level warnings will be really useful or raising unnecessary alarms.
   So it is possible I am overlooking few possibilities.
   If we do decide to forward the lowlevel warnings to user, this will need
   extending ed_connection with new method like append_unique_warnings()
   May be part of another patch? Let us discuss if this is needed.

-thava

> DETAILS
> -------
> Thava Alagu wrote:
>> #At file:///home/thava/mysql/repo/backup/
>>
>>  2888 Thava Alagu    2009-11-06
>>       BUG#47994 : Interruption of BACKUP command reported multiple 
>> times.
>>             When backup session is interrupted at certain stages, 
>> multiple duplicate
>>       warning messages are generated.
>>             Added additional checks to catch interruption of the 
>> command earlier
>>       to stop the backup kernel from executing further queries.
>>       Removed some duplicate pushing of warnings.       modified:
>>         mysql-test/suite/backup/r/backup_errors.result
>>         mysql-test/suite/backup/t/backup_errors.test
>>         mysql-test/suite/backup_engines/r/backup_interruption.result
>>         sql/backup/backup_info.cc
>>         sql/backup/backup_kernel.h
>>         sql/backup/kernel.cc
>>         sql/backup/logger.h
>>         sql/share/errmsg-utf8.txt
>>         sql/share/errmsg.txt
>>         sql/si_logs.cc
>>         sql/si_logs.h
>>         sql/si_objects.cc
>>         sql/si_objects.h
>>
> ...
.........
>  
> Rafal
>
>
>