Bug #47897 Unnecessary, non-public debug definitions in MGMAPI
Submitted: 7 Oct 2009 15:30 Modified: 9 Oct 2009 9:56
Reporter: Martin Zaun Email Updates:
Status: Patch approved Impact on me:
None 
Category:MySQL Cluster: NDB API Severity:S4 (Feature request)
Version:mysql-5.1-telco-6.2 OS:Any
Assigned to: Assigned Account CPU Architecture:Any
Tags: cleanup, ndb
Triage: Triaged: D5 (Feature request) / R1 (None/Negligible) / E1 (None/Negligible)

[7 Oct 2009 15:30] Martin Zaun
Description:
This is not a bug but an NDB MGM API code cleanup item.

Wrapping the MGMAPI with a JTie Java interface has revealed some unnecessary definitions in the .h files that are not meant to be part of the public API.

1) These two files are for internal debugging purposes:
     mgmapi_config_parameters_debug.h
     mgmapi_debug.h

   Move these files from include/mgmapi/ to /src/mgmapi/.

2) Delete duplicate definitions in mgmapi_config_parameters.h:

#define CFG_DB_DISK_PAGE_BUFFER_MEMORY 160 /* used from 5.1 */
#define CFG_DB_STRING_MEMORY          161 /* used from 5.1 */

#define CFG_DB_DISK_PAGE_BUFFER_MEMORY 160
#define CFG_DB_STRING_MEMORY          161 

3) Move internal definitions in mgmapi_config_parameters.h to  mgmapi_config_parameters_debug.h:

/**
 * Internal
 */
#define CFG_DB_STOP_ON_ERROR_INSERT   1

#define CFG_TYPE_OF_SECTION           999
#define CFG_SECTION_SYSTEM            1000
#define CFG_SECTION_NODE              2000
#define CFG_SECTION_CONNECTION        3000

#define NODE_TYPE_DB                  0
#define NODE_TYPE_API                 1
#define NODE_TYPE_MGM                 2

#define CONNECTION_TYPE_TCP           0
#define CONNECTION_TYPE_SHM           1
#define CONNECTION_TYPE_SCI           2
#define CONNECTION_TYPE_OSE           3 /* Removed. */

4) Consider demarcating deprecated field in log_event.h with flag DOXYGEN_SHOULD_SKIP_DEPRECATED:

  struct ndb_logevent_MemoryUsage {
    int      gth;
    /* union is for compatibility backward.                                     
     * page_size_kb member variable should be removed in the future             
     */
    union {
      unsigned page_size_kb;
      unsigned page_size_bytes;
    };
    ...
  };

How to repeat:
Check storage/ndb/include/mgmapi/*.h.

Suggested fix:
Move definitions+files under src/mgmapi/ for clean include/mgmapi/*.h files.
[8 Oct 2009 16:47] Bugs System
A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/86225

3093 Martin Zaun	2009-10-08
      ndb - bug#47897 - minor clean up actions in include/mgmapi.
[8 Oct 2009 17:00] Martin Zaun
Hi Magnus,

the patch differs slightly from our initial email discussion:

1) straight forward
     not needed: mgmapi_config_parameters_debug.h
     moved:      mgmapi_debug.h

2) fixed

3) turns out rather impractical:
   - the NODE_TYPE... defs are used right there in mgmapi.h
   - the CFG_ defs semantically belong into mgmapi_config_parameters.h
   - moving CONNECTION_TYPE defs to mgmapi_internal.h (or _debug.h)
     would require creating new include dependencies upon src/mgmapi
     in these files
       src/common/mgmcommon/ConfigRetriever.cpp
       src/common/mgmcommon/IPCConfig.cpp
       src/mgmapi/mgmapi_internal.h
       src/mgmsrv/ConfigInfo.cpp
       tools/ndb_config.cpp
     This just does not "feel" right to me.

   So, I reverted to the original idea leaving them in place but
   flagging them with DOXYGEN_SHOULD_SKIP_INTERNAL.  Minimal intrusion.

4) I'd prefer the DOXYGEN_SHOULD_SKIP_DEPRECATED flag on the deprecated
   field even if it means a bit more clutter.  I'll undo, however, if
   you object.

Thanks,
Martin
[8 Oct 2009 17:02] Martin Zaun
P.S.: Compiled and tested in 6.3 with mtr --suite=ndb,rpl_ndb,ndb_binlog.
[9 Oct 2009 9:56] Magnus Blåudd
Suggest you do a "make distcheck" as well. It will detect if you missed any Makefile updates.

In 7.0, the new include paths will need to be added to CMakeLists.txt as well.
[9 Oct 2009 17:11] Martin Zaun
Hi Magnus,

on my system, 'make distcheck' aborts with many errors, already on a *clean* ndb-6.3 branch:

gcc -DHAVE_CONFIG_H -I. -I../../include -I../../../cmd-line-utils/libedit -I../../include -I../../../include    -O    -D_P1003_1B_VISIBLE -DSIGNAL_WITH_VIO_CLOSE -DSIGNALS_DONT_BREAK_READ -DIGNORE_SIGHUP_SIGQUIT  -DDONT_DECLARE_CXA_PURE_VIRTUAL -MT help.o -MD -MP -MF .deps/help.Tpo -c -o help.o help.c
In file included from ../../../cmd-line-utils/libedit/el.h:96,
                 from help.c:3:
../../../cmd-line-utils/libedit/tty.h:459: error: syntax error before ‘int’
../../../cmd-line-utils/libedit/tty.h:460: error: syntax error before ‘void’
../../../cmd-line-utils/libedit/tty.h:461: error: syntax error before ‘int’
../../../cmd-line-utils/libedit/tty.h:462: error: syntax error before ‘int’
../../../cmd-line-utils/libedit/tty.h:463: error: syntax error before ‘int’
../../../cmd-line-utils/libedit/tty.h:464: error: syntax error before ‘int’
../../../cmd-line-utils/libedit/tty.h:465: error: syntax error before ‘int’
../../../cmd-line-utils/libedit/tty.h:466: error: syntax error before ‘void’
In file included from ../../../cmd-line-utils/libedit/el.h:97,
...

In my patched work branch, 'make distclean' aborts with the same error; so, it's not related to my patch.

Please, advise.

Will add include changes to CMakeLists.txt in >= 7.0.
[16 Oct 2009 8:21] Magnus Blåudd
Seems like the compiler error you get is in our bundled version of libedit. We have the option to use bundled libedit, bundled readline or system installed one. Looks like "make distcheck" always want to compile bundled libedit. Ignore it and go ahead with your push.
[28 Oct 2009 15:30] Magnus Blåudd
BUG#24809 contains a fix to the "make distcheck" problem