Re: [Tarantool-patches] [PATCH 2.X 0/7] RFC: module api: extend for external merger Lua module

["Timur Safin" <tsafin@tarantool.org>]

Before getting into all gory details and start to actively participate in discussion 
I want to highlight some basic principles which we used while preparing these simple 
patches (some are obvious, but some not that much):

- I try to offload functionality to the code existing inside of Tarantool as much as 
possible. If there are some structures or functions - refer to them. And this principle 
better be applied not only to Tarantool core specific functionality, but also to all 3rd 
party functions it uses. E.g. small, msgpuck, or even heap.

- And current principle to compile against them is simple - we use these 3rd party headers 
for compilation of functions in the module, but ignore libraries for a moment. We delay 
binding till run-time where system loader would substitute symbol from Tarantool executable 
which eventually called us as external module. If we could use msgpuck or small in this 
manner then we almost entirely avoiding most ABI incompatibility problems. 

- The question is: where to get those 3rd party headers? We may use several approaches 
  in modules:

1. via cmake ExternalProject_Add to github repo;
2. via git submodule to github repo;
3. via systems package manager dependency and binary dependency (e.g. apt or rpm).

So happened, that in merger we used several of them, e.g. submodule for small https://github.com/tsafin/tarantool-merge/tree/master/src
tarantool-dev system package for msgpuck. And simply copy-pasted heap.h from salad.
[IMVHO, salad worth to be treated similarly as small. For better reuse we may export
It to external repository]

3 different approaches for quite similar cases of code reuse. :) Eventually we may 
develop the consistent approach here - how to reuse 3rd party Tarantool library 
in ABI stable way, which may be originating of different versions of Tarantool? 
(e.g. small in 2 its incarnations from 2.x and 1.10)

Would it be always enough to use older version of headers, and run-time linking with 
parent executable? At the moment I see no reason to not do so. As far as I can see 
small is quite similar from ABI prospective between 1.10 and 2.x at the moment, 
we could start away as is, but certainly should add binary compatibility check somewhere 
into CI (to small repo, to Tarantool repo, and/or to merger repo) for future 
compatibility guarantees. I do assume that small repository was created just for such
usage scenarios like here (module reusing some useful functionality), and it's 
correct way to use it as submodule or ExternalProject_ in cmake. We just need to
make sure it will not broken later.

Same for msgpuck - it's already created for better code reuse - we just need to 
Make sure it's possible to be used in future compatible way. (Via extra CI here 
and there).

Having said all that, now we could return to patches discussions - see my 
other emails...

Regards,
Timur


: -----Original Message-----
: From: Timur Safin <tsafin@tarantool.org>
: Sent: Thursday, September 24, 2020 8:00 PM
: To: v.shpilevoy@tarantool.org; alexander.turenko@tarantool.org
: Cc: Timur Safin <tsafin@tarantool.org>; tarantool-
: patches@dev.tarantool.org
: Subject: [PATCH 2.X 0/7] RFC: module api: extend for external merger Lua
: module
: 
: This patchset is a continuation of patch series which Alexander Turenko
: has sent
: earlier on. It extends module api with the necessary wrappers we should
: have
: in api to make external merger possible. Some internal functionality which
: was acessible to internal merger, should be wrapped via accessors if
: merger
: is becoming external.
: 
: - most of introduced functions are simple public wrappers around of
: internal
:   functions, e.g. `box_tuple_validate` is a wrapper around
:   `tuple_validate_raw`, or `bax_key_def_dup` is a wrapper for
: `key_def_dup`.
:   They were necessary to not reveal implementation specific details
:   (i.e. `tuple_data` needed in `tuple_validate_raw` should know
: `box_tuple_t`
:   layout);
: - wherever we used to use `struct tuple *` we replacing them with public
: alias
:   `box_tuple_t`. Same is for `struct tuple_format` -> `box_tuple_format_t`
: and
:   other typedefs.
: 
: - eventually merger in Tarantool core might get deprecated and all new
:   development for merger will continue in version agnostic external module
:   thus we decided that common utility functions (e.g.
: `luaT_temp_luastate`)
:   which used to reside in merger to better be sitting in `src/lua/utils.c`
:   instead (in addition to becoming public as described above).
: 
: NB! You could observe, that this part of changes for merger in module_api
: is
: much, much easier than those for key_def as sent by Sasha Turenko before.
: 
: Issue:
: * https://github.com/tarantool/tarantool/issues/5273
:   ('module api: expose everything that is needed for external key_def
: module')
: 
: Branches:
: * https://github.com/tarantool/tarantool/tree/tsafin/gh-5273-expand-
: module-api
:   (top 7 commits above of 14 @Totktonada's commits)
: * https://github.com/tarantool/tarantool/tree/tsafin/gh-5273-expand-
: module-api-1.10
:   (last 9 commits above of 16 @Totktonada's commits)
: 
: == External merger module
: 
: Currently external merger is residing here
: https://github.com/tsafin/tarantool-merge
: and it uses key_def from https://github.com/Totktonada/key_def repository
: via simple luarocks dependency (we had to introduce debugging rock-server
: to
: make this working).
: 
: CI is installed and is running on GitHub Actions, thus we know that it's
: sort of working together, but more extensive testing of merger and key_def
: yet to be proceeded. Though it might be done independently of current
: kernel patches.
: 
: 
: Timur Safin (7):
:   module api: export box_tuple_validate
:   module api: export box_key_def_dup
:   module api: luaT_newthread
:   module api: luaL_register_module & luaL_register_type
:   module api: luaT_temp_luastate & luaT_release_temp_luastate
:   module api: luaL_checkibuf & luaL_checkconstchar
:   module api: luaL_cdata_iscallable
: 
:  src/box/key_def_api.c |  6 +++
:  src/box/key_def_api.h | 10 +++++
:  src/box/lua/merger.c  | 78 -------------------------------------
:  src/box/tuple.c       |  6 +++
:  src/box/tuple.h       | 11 ++++++
:  src/exports.h         | 10 +++++
:  src/lua/utils.c       | 51 ++++++++++++++++++++++++-
:  src/lua/utils.h       | 89 ++++++++++++++++++++++++++++++++++++-------
:  8 files changed, 169 insertions(+), 92 deletions(-)
: 
: --
: 2.20.1