Hi Alexander and thank you for your review. On Tue, Nov 27, 2018 at 2:18 PM Alexander Barkov <bar@mariadb.com> 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