Re: bzr commit into mysql-6.0-backup branch (cbell:2638) Bug#33364, Bug#35230, WL#4296

[Øystein Grøvlen]

Chuck Bell wrote:
> Hi,
> 
> I plan to do a new, restructured implementation of this worklog. Alas, but
> this is one of my works that has fallen victim to many plagues including
> diverging development trees. :|
> 
> I have supplied some answers to your queries in the interim.

Thanks, for responding to my comments.  I will await your next patch. 
Some comments inline.

...

>>  > +void Logger::report_engine(const char *engine_name)
>>  > +{
>>  > +  String str;    // engines string
>>  > +  DBUG_ASSERT(m_state == READY || m_state == RUNNING);
>>  > +
>>  > +  str.length(0);
>>  > +  if (m_op_hist->engine_name &&
>>  > +      (strlen(m_op_hist->engine_name) > 0) &&
>>  > +      (strlen(engine_name) > 0))
>>  > +    str.append(", ");
>>  > +  if (strlen(engine_name) > 0)
>>  > +    str.append(engine_name);
>>  > +  if (str.length() > 0)
>>  > +  {
>>  > +    if (m_op_hist->engine_name)
>>  > +      delete m_op_hist->engine_name;
>>  > +    m_op_hist->engine_name = new char[str.length()+1];
>>  > +    strcpy(m_op_hist->engine_name, str.ptr());
>>  > +  }
>>  > +}
>>
>> Oystein: I do not think the method above works as intended. 
>> It seems the old
>> content of m_op_hist->engine_name is overwritten, not appended to.
>> Did you intend to copy old name into str at the beginning?
> 
> Actually, it does work as intended and engine (driver) names are appended.

I have been staring at this code for a while, and I still do not 
understand this.  I assume what you want to achieve is to get a new 
em_op_hist->engine_name where engine_name is appended to the existing 
contents.  However, as far as I can tell, the old contents of 
em_op_hist->engine_name is not copied into str, and the new content is 
entirely based on the content of str.  Am I missing something here?
(I admit that looking at the  test result file, this seems to work.)

>>  > +  as the starting backup_id.
>>  > +
>>  > +  @todo Do we need a mutex to protect this call internally?
>>  > +*/
>>
>> Oystein: Why use a file and not the database when persisting 
>> backup id? 
>>   Seems
>> like we should eat our own dog food here.
> 
> It was decided that a file would be easier to work with -- faster, no
> locking to contend with, etc..
> 

I see the locking infrastructure as one of the advantages of using a db. 
     It should make it easier to guarantee consistency.  And I do not 
think speed is a major issue here.

> ...
> 
>> Oystein: Irregular indentation.
> 
> Left over from old code -- not much I can do except run it through a filter.
> I will try to remove them.

I was just talking about two lines of code in a function introduced in 
this patch which a 3 spaces of indent instead of 2:

+      if (hist_file > 0)
+      {
+         my_fstat(hist_file, &fstate, MYF(0));
+         my_close(hist_file, MYF(MY_WME));
+        id= fstate.st_size + 1;
+      }


> 
> ...
> 
>> Oystein: Mixing tabs and spaces for indentation here.  That should be
>> avoided.
> 
> Same as above.
> 

I am only referring to two tabs in logger.h that was added by this patch:

+	 int write_progress(THD *thd,

+	void get_state_string(enum_backup_state state, String *str);

...

> 
>>  > +class MYSQL_BACKUP_LOG: public MYSQL_LOG
>>
>> Oystein: What does this inheritance actually give that could not be
>> achieved by encapsulation?  As far as I can see, MYSQL_BACKUP_LOG does
>> not override any methods from MYSQL_LOG.
> 
> It keeps the backup-specific code out of the MYSQL_LOG class which is
> something else entirely. I sacrificed some clarity for separation.

I was not suggesting putting backup specific code into MYSQL_LOG but to 
instead of inheriting MYSQL_LOG consider whether to create an MYSQL_LOG 
instance and call methods on it from the backup specific code.  I wonder 
whether the relationship here is more of a "is-implemented-in-terms-of" 
than a "is-a" relationship (Ref. Scott Meyers, More Effective C++).

Awaiting your next patch,

-- 
Øystein