MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Rafal Somla Date:December 2 2009 9:02am
Subject:Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2895)
Bug#47940
View as plain text  
Hi Ingo,

Thaks for your review.

Ingo Strüwing wrote:
> Hi Rafal,
> 
> Status: Approved.
> 
> Suggestions:
> 1) Update function header of save_vp_info().

I've chosen to leave it as it is (see below).

> 3) Move type conversion operators implementations together.
> 

Done.

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

See below.

...
>> === 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().
> 

Hmm, I think given function description:

  "Store information about validity point in @c Backup_info structure"

it is obvious that the info parameter refers to the Backup_info structure in 
which information will be stored:

  static int save_vp_info(Backup_info &info)

Thus I'll leave it as it is.

> ...
>> +    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?
> 

Sorry, I don't remember the lock_tables() discussion. I think one good 
reason for doing cleanup in a destructor is to have a safer code with 
respect to interrupts, early termination etc. I mean, if the code is like this:

int foo()
{
   <allocate resources>;
   <do something>;
   <free resources>;
}

then we have a problem of freeing resources if e.g. we do early return 
inside <do something>. If <do something> is long and complicated, it is very 
easy to make this mistake. This is always handled correctly if resources are 
allocated inside an object instance and freed in this object destructor. 
This is one reason I wanted to use e.g. Backup_restore_ctx class, which does 
all necessary clean-ups in the destructor.

However, in the above code it is critical to unlock() drivers as early as 
possible. Since not much happens (and not much should happen) between lock() 
and unlock() calls, I think the above structure of the code is most 
appropriate for this place.

Trying to answer your more general question about cleanup paradigm, I can 
only tell what are my own preferences. I never formulated them precisely, 
but I think they are something like:

- The allocator of resources should free them if possible. The allocator 
could be a function or an object.

- Whenever possible/appropriate, use objects for allocating resources which 
then will be automagically freed upon object's destruction.

> ...
>>  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.
>

OK, I moved it next to operator bool().

> (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.

Hmm, I'm not sure what you want to criticize. I read it as an expression of 
your general dislike for iterator concept.

For me the main advantage of this concept is that it leaves bigger freedom 
of implementation. An iterator can be implemented for sequential (e.g. 
linked list) as well as random access containers (e.g. array). This allows 
designing interfaces which do not exclude either possibility.

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