maria-developers team mailing list archive

[Nirbhay Choubey <nirbhay@xxxxxxxxxxx>]

Hi Serg,

On Thu, May 28, 2015 at 5:56 PM, Sergei Golubchik <serg@xxxxxxxxxxx> wrote:

> Hi, Nirbhay!
>
> On May 09, Nirbhay Choubey wrote:
> > revision-id: 8928497acbe3e17a105897f405d5e5138d57a249
> > parent(s): 3674c363a7e77e534c3e0d28659dc614c53fbcbc
> > committer: Nirbhay Choubey
> > branch nick: 10.1-wsrep
> > timestamp: 2015-05-09 16:11:59 -0400
> > message:
> >
> > MDEV-7067: Server outputs Galera (WSREP) information, even if Galera is
> disabled
> >
> > * mysqld_safe: Since wsrep_on variable is mandatory in 10.1, skip wsrep
> > position recovery if its OFF.
> > * mysqld: Remove "-wsrep" from server version
> > * mysqld: Remove wsrep patch version from @@version_comment
> > * mysqld: Introduce @@wsrep_patch_version
>
> Looks ok, I just didn't like the wsrep.cmake change. See below.
>
> > diff --git a/cmake/wsrep.cmake b/cmake/wsrep.cmake
> > index c37cf74..305ab27 100644
> > --- a/cmake/wsrep.cmake
> > +++ b/cmake/wsrep.cmake
> > @@ -28,29 +28,22 @@ OPTION(WITH_WSREP "WSREP replication API (to use,
> e.g. Galera Replication librar
> >  # Set the patch version
> >  SET(WSREP_PATCH_VERSION "10")
> >
> > -# MariaDB addition: Revision number of the last revision merged from
> > -# codership branch visible in @@version_comment.
> > -# Branch : codership-mysql/5.6
> > -SET(WSREP_PATCH_REVNO "4144")  # Should be updated on every merge.
> > -
> > -# MariaDB addition: Revision number of the last revision merged from
> > -# Branch : lp:maria/maria-10.0-galera
> > -SET(WSREP_PATCH_REVNO2 "3919")  # Should be updated on every merge.
> > -
> > -# MariaDB: Obtain patch revision number:
> > -# Update WSREP_PATCH_REVNO if WSREP_REV environment variable is set.
> > -IF (DEFINED ENV{WSREP_REV})
> > -  SET(WSREP_PATCH_REVNO $ENV{WSREP_REV})
> > -ENDIF()
> >
> > -SET(WSREP_INTERFACE_VERSION 25)
> > +# Obtain wsrep API version
> > +EXECUTE_PROCESS(
> > +  COMMAND sh -c "grep WSREP_INTERFACE_VERSION
> ${MySQL_SOURCE_DIR}/wsrep/wsrep_api.h | cut -d '\"' -f 2"
> > +  OUTPUT_VARIABLE WSREP_API_VERSION
> > +  RESULT_VARIABLE RESULT
> > +)
>
> I don't like calling grep|cut from cmake files. If you want to extract
> the version from wsrep_api.h, use cmake commands, e.g.
>
>   FILE(STRINGS "${MySQL_SOURCE_DIR}/wsrep/wsrep_api.h" WSREP_API_H
>        LIMIT_COUNT 1 REGEX "WSREP_INTERFACE_VERSION")
>   STRING(REGEX MATCH ....
>

Done.


>
> > +STRING(REGEX REPLACE "(\r?\n)+$" "" WSREP_API_VERSION
> "${WSREP_API_VERSION}")
> >
> > -SET(WSREP_VERSION
> > -
> "${WSREP_INTERFACE_VERSION}.${WSREP_PATCH_VERSION}.r${WSREP_PATCH_REVNO}")
> > +SET(WSREP_VERSION "${WSREP_API_VERSION}.${WSREP_PATCH_VERSION}"
> > +  CACHE STRING "WSREP version")
>
> make it INTERNAL, not STRING
>

Done.


>
> >  SET(WSREP_PROC_INFO ${WITH_WSREP})
> >
> >  IF(WITH_WSREP)
> > -  SET(COMPILATION_COMMENT "${COMPILATION_COMMENT},
> wsrep_${WSREP_VERSION}")
> > +  SET(WSREP_PATCH_VERSION "wsrep_${WSREP_VERSION}")
> >  ENDIF()
> >
> > diff --git a/mysql-test/suite/sys_vars/r/sysvars_wsrep.result
> b/mysql-test/suite/sys_vars/r/sysvars_wsrep.result
> > index 9c392b1..1763078 100644
> > --- a/mysql-test/suite/sys_vars/r/sysvars_wsrep.result
> > +++ b/mysql-test/suite/sys_vars/r/sysvars_wsrep.result
> > @@ -365,6 +365,20 @@ NUMERIC_BLOCK_SIZE       NULL
> >  ENUM_VALUE_LIST      TOI,RSU
> >  READ_ONLY    NO
> >  COMMAND_LINE_ARGUMENT        OPTIONAL
> > +VARIABLE_NAME        WSREP_PATCH_VERSION
> > +SESSION_VALUE        NULL
> > +GLOBAL_VALUE wsrep_25.10
> > +GLOBAL_VALUE_ORIGIN  COMPILE-TIME
> > +DEFAULT_VALUE        NULL
> > +VARIABLE_SCOPE       GLOBAL
> > +VARIABLE_TYPE        VARCHAR
> > +VARIABLE_COMMENT     wsrep patch version
> > +NUMERIC_MIN_VALUE    NULL
> > +NUMERIC_MAX_VALUE    NULL
> > +NUMERIC_BLOCK_SIZE   NULL
> > +ENUM_VALUE_LIST      NULL
> > +READ_ONLY    YES
> > +COMMAND_LINE_ARGUMENT        NULL
> >  VARIABLE_NAME        WSREP_PROVIDER
> >  SESSION_VALUE        NULL
> >  GLOBAL_VALUE none
> > diff --git
> a/mysql-test/suite/sys_vars/r/wsrep_patch_version_basic.result
> b/mysql-test/suite/sys_vars/r/wsrep_patch_version_basic.result
> > new file mode 100644
> > index 0000000..80f29e6
> > --- /dev/null
> > +++ b/mysql-test/suite/sys_vars/r/wsrep_patch_version_basic.result
>
> I've actually stopped creating sys_var tests for every new variable
> (even though I was the one who introduced sys_vars.all_vars test in the
> first place :).
>
> With the new I_S.SYSTEM_VARIABLES table we have all variables listed
> there anyway, with all properties. So there's no need to copy these
> boilerplate tests over and over.
>

Such tests can be useful too, for instance, sometime back I found quite a
number of
crashing issues (assigning 'default') while adding basic tests for wsrep
variables.
But, since its a RO variables, I have removed the basic test.


> > diff --git a/mysql-test/suite/wsrep/suite.pm b/mysql-test/suite/wsrep/
> suite.pm
> > index 02dff24..33744a6 100644
> > --- a/mysql-test/suite/wsrep/suite.pm
> > +++ b/mysql-test/suite/wsrep/suite.pm
> > @@ -27,6 +27,7 @@ push @::global_suppressions,
> >       qr(WSREP: Could not open saved state file for reading: ),
> >       qr(WSREP: option --wsrep-casual-reads is deprecated),
> >       qr(WSREP: --wsrep-casual-reads=ON takes precedence over
> --wsrep-sync-wait=0),
> > +     qr|WSREP: access file\(gvwstate.dat\) failed\(No such file or
> directory\)|,
>
> why?
>

Its an additional warning from newer versions of galera library, but I will
remove it in the
next commit as it has already been added.


> >     );
> >
> > diff --git a/sql/mysqld.cc b/sql/mysqld.cc
> > index 14d904b..49a639b 100644
> > --- a/sql/mysqld.cc
> > +++ b/sql/mysqld.cc
> > @@ -1902,13 +1902,15 @@ static void __cdecl kill_server(int sig_ptr)
> >    }
> >  #endif
> >
> > -  if (WSREP_ON)
> > +  /* Stop wsrep threads in case they are running. */
> >      wsrep_stop_replication(NULL);
>
> Does this print any messages?
>

Yes.

..snip..
void wsrep_stop_replication(THD *thd)
{
  WSREP_INFO("Stop replication");
  if (!wsrep)
  {
    WSREP_INFO("Provider was not loaded, in stop replication");
    return;
  }
..snip..

BTW, it actually fixes MDEV-7798. The fix has not been upmerged yet, so I
thought of
including it here.

Best,
Nirbhay


> Regards,
> Sergei
>