MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Charles Bell Date:December 1 2009 4:57pm
Subject:Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2895)
Bug#47940
View as plain text  
STATUS
------
Approved pending changes from previous email:

> REQUESTS
> --------
> 1. Please explain why the code is duplicating the error. I don't think 
> it should be doing this.

I am ok with the explanation, thanks.

> 2. Spelling errors.

Please fix this.

> 3. Too many characters on one line.

Please fix this.

> DETAILS
> -------
>>       Bug#47940 - Backup: Implementation of save_vp_info() violates 
>> its specification.
>>             This patch fixes problems in the backup synchronization 
>> logic:
>>             - In case of error, all drivers which have ben locked, 
>> should be 
> 
> [2] been
> 
>>         unlocked before we abort operation.
>>             - Potentially time consuming error reporting should not be 
>> done inside
>>         the synchronization phase, but only after all drivers have been
>>         unlocked.
>>      @ 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.
> 
>>      @ 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.
> 
>>
>>     modified:
>>       mysql-test/suite/backup/r/backup_errors_debug_3.result
>>       sql/backup/data_backup.cc
>> === modified file 
>> 'mysql-test/suite/backup/r/backup_errors_debug_3.result'
>> --- a/mysql-test/suite/backup/r/backup_errors_debug_3.result    
>> 2009-11-24 16:47:23 +0000
>> +++ b/mysql-test/suite/backup/r/backup_errors_debug_3.result    
>> 2009-11-25 10:37:26 +0000
>> @@ -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
>>  PURGE BACKUP LOGS;
>>  #
>>  # Turn off debug SESSION.
>>
>> === modified file 'sql/backup/data_backup.cc'
>> --- a/sql/backup/data_backup.cc    2009-10-21 13:32:24 +0000
>> +++ b/sql/backup/data_backup.cc    2009-11-25 10:37:26 +0000
>> @@ -42,6 +42,7 @@ struct backup_state {
>>                WAITING,    ///< Waiting for others to finish init ph. 
>> (phase 2).
>>                PREPARING,  ///< Preparing for @c lock() call (phase 3).
>>                READY,      ///< Ready for @c lock() call (phase 4)
>> +              LOCKED,     ///< After @c lock() and before @c unlock().
>>                FINISHING,  ///< Final data transfer (phase 7).
>>                DONE,       ///< Backup complete.
>>                SHUT_DOWN,  ///< After @c end() call.
>> @@ -67,6 +68,7 @@ struct backup_state {
>>        name[WAITING]=    "WAITING";
>>        name[PREPARING]=  "PREPARING";
>>        name[READY]=      "READY";
>> +      name[LOCKED]=     "LOCKED";
>>        name[FINISHING]=  "FINISHING";
>>        name[DONE]=       "DONE";
>>        name[SHUT_DOWN]=  "SHUT DOWN";
>> @@ -256,6 +258,7 @@ public:
>>    int prepare();
>>    int lock();
>>    int unlock();
>> +  int report_vp_errors();
>>  
>>    uint  init_count;     ///< Nr of drivers sending init data.
>>    uint  prepare_count;  ///< Nr of drivers preparing for lock.
>> @@ -286,11 +289,15 @@ private:
>>    Output_stream &m_str; ///< Stream to which we write.
>>    bool   cancelled;     ///< True if backup process was cancelled.
>>  
>> +  Backup_pump *m_failed_lock; ///< If not NULL, points at driver 
>> which failed in lock() call.
>> +  bool   m_vp_info_error;  ///< TRUE if errors were detected while 
>> collecting VP info in the synchronization phase.
> 
> [3] Long line please fix.
> 
> ...
> 
> Chuck
> 

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