maria-developers team mailing list archive

[Jan Lindström <jan.lindstrom@xxxxxxxxxxx>]

Hi Alexander and thank you for your review.

On Tue, Nov 27, 2018 at 2:18 PM Alexander Barkov <bar@xxxxxxxxxxx> wrote:

> Hi Jan,
>
>
> I checked changes to the /sql directory so far.
> Sorry, I'm completely out of this topic.
> So I have only general suggestions.
>
>
>
> 1. General code structure.
>
> There are a lot of new
>
> #ifdef WITH_WRESP
>   do_something_with_wresp();
> #else
>   do_something_without_wresp();
> #endif
>

This is true, as do_something_with_wsrep() contains variables and/or
methods that are not available when compiling !WITH_WSREP as we do
in case of embedded server and in Windows. Some of them could be avoided by
adding empty macros for functions and letting wsrep variables be
there even when you do not use them ever. Some of those are like
if(wsrep_on(thd)) do_something_with_wsrep() that is naturally dynamic.


>
> This make the server code harder to follow.
> I'd prefer a more modular way.
> Would it be possible to introduce some abstract class with virtual
> methods? One instantiable class would implement behavior for wresp,
> and other class would implement behavior for "without wresp".
>

Not sure if I agree on this readability, virtual methods just hide this
wsrep behavior somewhere else (I must admit I'm not fan of C++).


>
> Instead #ifdefs, we would do:
>
>   behavour->do_something();
>
>
>
> 4. Some code was commented. Why not just remove it?
>

I agree on this, I do not see point keeping commented or #ifdef lines that
are newer used

R: Jan