Code review comment for lp:~sergei.glushchenko/percona-server/disable-flashcache

Revision history for this message
Alexey Kopytov (akopytov) wrote :

Sergei,

   - since after this fix flashcache is disabled by default, failure to
     set it up when it's _explicitly_ enabled should be a fatal error
   - the above also means the error should be printed with
     sql_print_error() rather than sql_print_information()
   - that also means the code printing the "Flashcache bypass" message
     does not have to check if initialization was successful (it should
     just say "flashcache support initialized successfully")
   - strerror() is not portable. which is not that important in this
     case, of course, but I would use sql_perror() for consistency.
   - the command line option is available only on Linux, whereas the
     test case includes "not_windows.inc". Which is not the same set of
     platforms, it will fail on all non-Windows platforms except Linux
   - the test case dependency on external utilities like grep and sed is
     redundant, even though it might work in most cases (since this is a
     Linux-specific test).

   I think a clean way to implement the test case would be to add
   'include/have_flashcache.inc' (which would also require a separate
   status variable). So you can easily check if the code you are going
   to test is present in the binary. You should also test only the
   --flashcache=0 case (i.e. the default) and make sure the
   initialization message is not present in the error log. Then you only
   need a few lines in Perl to implement that (see
   percona_log_warnings_suppress.test for an example).

review: Needs Fixing

« Back to merge proposal