[Tarantool-patches] [PATCH] net.box: add predefined system events for pub/sub
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Sat Dec 18 02:31:25 MSK 2021
Hi! Thanks for the patch!
On 16.12.2021 12:22, Yan Shtunder wrote:
> Adding predefined system event box.status
Please, add ticket reference here as
Part of #6260
On a new line. With an empty line before it. Like this:
Your message.
Line,
More lines.
Part of #6260
See 11 comments below.
> diff --git a/src/box/box.cc b/src/box/box.cc
> index 0413cbf44..8529e6925 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -160,6 +160,9 @@ static struct fiber_pool tx_fiber_pool;
> */
> static struct cbus_endpoint tx_prio_endpoint;
>
> +void
> +box_status_broadcast(void);
1. The function is never used outside of box.cc. You can make it 'static'.
Also lets better call it box_broadcast_status(). It would look better
if all built-in events would start with the same box_broadcast_*.
> @@ -377,9 +382,11 @@ box_set_orphan(bool orphan)
> if (is_orphan) {
> say_crit("entering orphan mode");
> title("orphan");
> + box_status_broadcast();
2. You can call this inside of title() function instead of
duplicating its broadcast for each title() invocation.
> } else {
> say_crit("leaving orphan mode");
> title("running");
> + box_status_broadcast();
> }
> }
> @@ -3800,6 +3810,8 @@ box_cfg_xc(void)
> assert(box_is_ro());
> /* If anyone is waiting for ro mode. */
> fiber_cond_broadcast(&ro_cond);
> + /* Checking box.cfg.read_only change */
> + box_status_broadcast();
3. The place didn't change is_ro_summary, so a broadcast here is not needed.
> /*
> * Yield to let ro condition waiters to handle the event.
> * Without yield it may happen there won't be a context
> @@ -3903,3 +3915,32 @@ box_reset_stat(void)
> engine_reset_stat();
> space_foreach(box_reset_space_stat, NULL);
> }
> +
> +void
> +box_status_broadcast(void)
> +{
> + size_t size = 0;
> +
> + size += mp_sizeof_map(3);
> + size += mp_sizeof_str(strlen("is_ro"));
> + size += mp_sizeof_bool(box_is_ro());
> + size += mp_sizeof_str(strlen("is_ro_cfg"));
> + size += mp_sizeof_bool(cfg_geti("read_only"));
> + size += mp_sizeof_str(strlen("status"));
> + size += mp_sizeof_str(strlen(box_status()));
> +
> + char buf[size];
4. Usage of non-constant values as a buffer size is not used in Tarantool code.
Lets better avoid it. You can just use a big enough buffer like
char buf[1024];
and add an assertion in the end of encodes below that you didn't go beyond
the border.
> + char *w = buf;
> +
> + w = mp_encode_map(w, 3);
> + w = mp_encode_str(w, "is_ro", strlen("is_ro"));
> + w = mp_encode_bool(w, box_is_ro());
> + w = mp_encode_str(w, "is_ro_cfg", strlen("is_ro_cfg"));
> + w = mp_encode_bool(w, cfg_geti("read_only"));
> + w = mp_encode_str(w, "status", strlen("status"));
> + w = mp_encode_str(w, box_status(), strlen(box_status()));
5. It looks like a quite repetitive thing. mp_error.cc had the same problem
and introduced mp_encode_str0(). I suggest you to make a preparatory commit
which would move mp_encode_str0() from mp_error.cc into a common place from
where you could reuse it.
One option would be to make a patch for https://github.com/tarantool/msgpuck
to add this new function. Or we could add it to trivia/util.h and src/util.c.
The first option is better IMO.
> +
> + box_broadcast("box.status", strlen("box.status"), buf, w);
> +
> + assert(size == (size_t)(w - buf));
> +}
> diff --git a/test/app-luatest/gh_6260_add_predefined_events_test.lua b/test/app-luatest/gh_6260_add_predefined_events_test.lua
> new file mode 100644
> index 000000000..e6ff0e94e
> --- /dev/null
> +++ b/test/app-luatest/gh_6260_add_predefined_events_test.lua
6. You don't need to use imperative in file names. They are not commit titles.
A simple gh_6260_builtin_events would be enough.
> @@ -0,0 +1,61 @@
> +local t = require('luatest')
> +local cluster = require('test.luatest_helpers.cluster')
> +local helpers = require('test.luatest_helpers')
> +
> +local g = t.group('gh_6260', {{engine = 'memtx'}, {engine = 'vinyl'}})
7. Why do you need different engines? The feature does not seem to be
engine-dependent. You don't even use spaces in the test.
> +
> +g.before_each(function(cg)
> + local engine = cg.params.engine
> +
> + cg.cluster = cluster:new({})
> +
> + local box_cfg = {
> + replication = {
> + helpers.instance_uri('master')
> + },
> + replication_timeout = 1,
> + read_only = false
> + }
8. Why do you need replication and replication_timeout from the
beginning? It seems for your case it would be enough to start an
instance without any particular options.
> +g.test_box_status_event = function(cg)
> + cg.master:eval([[
> + i = ''
> + box.watch('box.status', function(...) i = {...} end)
> + ]])
9. Please, use :exec() function. It looks much better than :eval().
> + t.assert_equals(cg.master:eval("return i"), {"box.status", {is_ro = false, is_ro_cfg = false, status = "running"}})
10. Try to keep line widths <= 80 symbols.
> +
> + cg.master:eval(([[
> + box.cfg{
> + replication = {
> + "%s",
> + "%s"
> + },
> + replication_connect_timeout = 0.001,
> + }
> + ]]):format(helpers.instance_uri('master'), helpers.instance_uri('replica')))
> + t.assert_equals(cg.master:eval("return i"), {"box.status", {is_ro = true, is_ro_cfg = false, status = "orphan"}})
> +
> + cg.master:eval(([[
> + box.cfg{
> + replication = {
> + "%s",
> + },
> + }
> + ]]):format(helpers.instance_uri('master')))
> + t.assert_equals(cg.master:eval("return i"), {"box.status", {is_ro = false, is_ro_cfg = false, status = "running"}})
> +
> + cg.master:eval("box.cfg{read_only = true}")
> + t.assert_equals(cg.master:eval("return i"), {"box.status", {is_ro = true, is_ro_cfg = true, status = "running"}})
> +end
11. Please, at lest one test case for netbox. That a remote subscription also works.
More information about the Tarantool-patches
mailing list