MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Ingo Strüwing Date:November 28 2009 6:50pm
Subject:Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2895)
Bug#47940
View as plain text  
Hi Rafal,

Status: Approved.

Suggestions:
1) Update function header of save_vp_info().
3) Move type conversion operators implementations together.

Commentary:
2) Which cleanup paradigm do we use in the backup module?

Details:
Rafal Somla, 25.11.2009 11:37:
...

>  2895 Rafal Somla	2009-11-25
>       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 
>         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.
...
> === modified file 'sql/backup/data_backup.cc'
...
> @@ -481,12 +488,7 @@ int save_vp_info(Backup_info &info)
>  


1) Would be nice to have a "@param" line in the function header of
save_vp_info().

...
> +    if(!sch.lock())
> +    {
> +      if(save_vp_info(info))
> +        sch.m_vp_info_error= TRUE;
> +      DEBUG_SYNC(thd, "before_backup_data_unlock");
> +      sch.unlock();
> +    }
>  


2) I like the paradigm that a function cleans up behind itself, if it
fails. sch.lock() unlocks all drivers if any one lock fails. But at
another place (lock_tables()) you wanted me, not to clean up within the
function, but to leave it to the destructor. Is there a good reason to
implement different paradigms of cleanup in one module?

...
>  class Scheduler::Pump_iterator
>  {
> +  LIST  *pumps;  ///< The list of pumps.
> +
>  public:
>  
> -  LIST  *pumps;  ///< The list of pumps.
> +  /// Return pointer to the current pump.
> +  operator Pump*()
> +  {
> +    return pumps? static_cast<Pump*>(pumps->data) : NULL;
> +  }


3) Would be nice to have the type conversion operators near by each
other (see also 'bool') and not scattered among the other operators.

(unnumbered commentary) BTW, I learned that iterators abstract the
"container" implementation from the application. But if we need to
change the iterator, when we change the application, this concept
doesn't make much sense to me.

...

Regards
Ingo
-- 
Ingo Strüwing, Database Group
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schröder,   Wolfgang Engels,   Wolf Frenkel
Vorsitzender des Aufsichtsrates: Martin Häring   HRB München 161028

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