Hi Rafal,
OK. I think the use you describe is an exception, but please make the
Rows_log_event_old classes pure virtual to prevent accidental misuse.
/Matz
Rafal Somla wrote:
> Mats Kindahl wrote:
>> Hi Rafal!
>>
>> I have looked at the code, and it appears to be correct, albeit hard
>> to follow. I have some suggestions to improve the overall structure
>> of the code and make it easier to maintain.
>>
>> In general, the principle of using a mixin (they are called that) is
>> sound, but there are a few guidelines to following when using a
>> mixin. They are not written in stone, but one should not deviate from
>> them unless there is a good reason:
>
> I think there is a good reason for deviating from "no virtual
> functions in mixin" guideline. Here it goes:
>
> The main purpose of introducing my mixin Rows_log_event_old, is to
> move here old implementation of do_apply_event() method to be used by
> derived {Write,Update,Delete}_rows_log_event_old classes. This old
> implementation uses do_xxxx() primitives, for example do_exec_row().
>
> If I want to use the old code as implementation of
> Rows_log_event_old::do_apply_event(), I must declare do_exec_row() and
> other primitives in this class. And I must declare them as virtual
> since they will be defined only in the derived classes
> {Write,Update,Delete}_rows_log_event_old.
> Lets look at my example again:
>
> 1) Write_rows_log_event_old ev;
>
> 2) ev->do_apply_event(...)
> 3) Rows_log_event_old::do_apply_event(ev,...)
> 4) ev->do_exec_row(...) // this will call version defined in
> // Write_rows_log_event_old
>
> In line 4, correct implementation will be called only if
> do_exec_row(...) is virtual. If it were not, then it would have been
> defined inside Rows_log_event_old, but this is not possible since
> implementation depends on the type of event.
>
>>> Rows_log_event_old
>>> /
>>> Write_rows_log_event_old
>>> \
>>> Write_rows_log_event - Rows_log_event
>>>
>>>
>>> But do_apply_event() operates on instances of Rows_log_event class.
>>> Therefore I must pass pointer to Rows_log_event instance to the
>>> method and make Rows_log_event_old a friend of Rows_log_event so
>>> that it can access all members.
>>> This leads to the following solutions:
>>>
>>> Rows_log_event_old::do_apply_event(Rows_log_event *,...)
>>>
>>> Write_rows_log_event_old::do_apply_event(...)
>>> {
>>> Rows_log_event_old::do_apply_event(this,...)
>>> }
>>>
>>
>> Since this is not the same as the do_apply_event() in Rows_log_event,
>> I suggest changing the name.
>>
>
> Ok, I can easily change it to e.g. do_apply_event_old() and will make
> it non-virtual.
>
>>> And here we come to the most suspicious part of my code. For example
>>> let's take do_exec_row() primitive redefined in
>>> Write_rows_log_event_old. The class inherits this method from both
>>> Rows_log_event_old and Write_rows_log_event and in both these
>>> classes the method is declared virtual. But, AFAIK, this should not
>>> be a problem. After redefining do_exec_row() in
>>> Write_rows_log_event_old, this definition will be always used. For
>>> example:
>>
>> Then you don't need to have do_exec_row() in Rows_log_event_old
>> virtual either. Also, since this again is not the same as
>> do_exec_row() in Rows_log_event, I suggest you rename it to something
>> else.
>>
>
> I think it must be virtual as I explained above. Changing its name
> would require renaming all uses of it in the old code, instead of just
> cutting-and-pasting it from log_event.cc to log_event_old.cc - an
> error prone procedure. Do you really insist on that? In my opinion,
> distinction between Write_rows_log_event::do_exec_row() and
> Write_rows_log_event_old::do_exec_row() is as good as the one between
> do_exec_row() and do_exec_row_old().
>
>
> Rafal
--
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com
