MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Rafal Somla Date:November 26 2009 6:31am
Subject:Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2895)
Bug#47940
View as plain text  
Hi Chuck,

Thanks for review. I'll prepare new patch with fixes after Ingo's review.

Charles Bell wrote:
> STATUS
> ------
> Undecided.
> 
> REQUESTS
> --------
> 1. Please explain why the code is duplicating the error. I don't think 
> it should be doing this.

The error is not exactly duplicated. There is one error per each failing 
backup driver and each error contains driver name. We have two errors only 
because two drivers are failing. A single error would mean that only one 
driver has failed.

> 2. Spelling errors.

Will be fixed.

> 3. Too many characters on one line.
> 

Will be fixed.

...
>>      @ mysql-test/suite/backup/r/backup_errors_debug_3.result
>>         This test case triggers errors during unlock() of backup 
>> drivers. Since two
>>         drivers are used, the unlock error should be reported twice. 
>> This patch
>>         fixes it.
> 
> [1] I don't think we should report the same error twice. I understand 
> this comment and after reading the code it is fine, however, I think we 
> should be generating an error that includes the driver name or some 
> such. That is, so the user knows these errors came from two different 
> places. Either that or only report the error once.
> 

This is actually the case: the errors contain driver name.

>>      @ sql/backup/data_backup.cc
>>         - Add new LOCKED driver state.
>>         - Add Scheduler::report_vp_errors() method.
>>         - Add Scheduler members m_failed_lock and m_vp_info_error which
>>           store information about errors encountered in the 
>> synchronization
>>           phase.
>>         - Remove error reporting from time critical methods.
>>         - Refactoring of the synchronization phase.
>>         - Add new operator and fix comments in Pump_iterator class.
>>         - Refactor Pump_iterator::lock()/unlock() methods: make sure 
>> all           drivers which have been locked are unlocked in case of 
>> error.
>>         - Refactor Backup_pump methods: remove error reporting and 
>>           handle the new LOCKED state.
> 
> Commentary: This makes me nervous when we include so much refactoring in 
> a bug report. But I don't see any logic errors, just questioning the 
> need to do this.
>

Hmm, but I think this is the gist of the problem: error reporting can 
potentially involve time consuming I/O and thus should not be done inside 
the critical section. The refactoring was done mostly to remove error 
reporting from these methods and then to make everything work together as 
expected.

>> @@ -626,6 +626,7 @@ ERROR HY000: Can't unlock DEBUG TEST bac
>>  SELECT * FROM mysql.backup_progress WHERE error_num <> 0;
>>  backup_id    object    error_num    notes
>>  #        ####    Can't unlock DEBUG TEST backup driver after creating 
>> the validity point
>> +#        ####    Can't unlock DEBUG TEST backup driver after creating 
>> the validity point

Note that in reality these errors read like this:

  Can't unlock MyISAM backup driver after creating the validity point
  Can't unlock Default backup driver after creating the validity point

The "DEBUG TEST" replacement is introduced in the test script to make 
results independent of the order of messages (I guess).

Rafal
Thread
bzr commit into mysql-6.0-backup branch (Rafal.Somla:2895) Bug#47940Rafal Somla25 Nov
  • Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2895)Bug#47940Charles Bell25 Nov
    • Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2895)Bug#47940Rafal Somla26 Nov
    • Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2895)Bug#47940Charles Bell1 Dec
  • Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2895)Bug#47940Ingo Strüwing28 Nov
    • Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2895)Bug#47940Rafal Somla2 Dec
      • Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2895)Bug#47940Ingo Strüwing2 Dec