MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Charles Bell Date:November 25 2009 6:55pm
Subject:Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2895)
Bug#47940
View as plain text  
STATUS
------
Undecided.

REQUESTS
--------
1. Please explain why the code is duplicating the error. I don't think 
it should be doing this.
2. Spelling errors.
3. Too many characters on one line.

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