Re: [Tarantool-patches] [PATCH] net.box: add predefined system events for pub/sub

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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.