* [Tarantool-patches] [PATCH] net.box: add predefined system events for pub/sub
@ 2021-12-16 11:22 Yan Shtunder via Tarantool-patches
2021-12-17 23:31 ` Vladislav Shpilevoy via Tarantool-patches
0 siblings, 1 reply; 2+ messages in thread
From: Yan Shtunder via Tarantool-patches @ 2021-12-16 11:22 UTC (permalink / raw)
To: v.shpilevoy; +Cc: tarantool-patches, Yan Shtunder
Adding predefined system event box.status
---
Issue: https://github.com/tarantool/tarantool/issues/6260
Patch: https://github.com/tarantool/tarantool/tree/yshtunder/gh-6260-add-predefined-system-events
src/box/box.cc | 41 +++++++++++++
.../gh_6260_add_predefined_events_test.lua | 61 +++++++++++++++++++
2 files changed, 102 insertions(+)
create mode 100644 test/app-luatest/gh_6260_add_predefined_events_test.lua
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);
+
void
box_update_ro_summary(void)
{
@@ -173,6 +176,8 @@ box_update_ro_summary(void)
if (is_ro_summary)
engine_switch_to_ro();
fiber_cond_broadcast(&ro_cond);
+ /* Checking box.info.ro change */
+ box_status_broadcast();
}
const char *
@@ -377,9 +382,11 @@ box_set_orphan(bool orphan)
if (is_orphan) {
say_crit("entering orphan mode");
title("orphan");
+ box_status_broadcast();
} else {
say_crit("leaving orphan mode");
title("running");
+ box_status_broadcast();
}
}
@@ -3508,6 +3515,7 @@ local_recovery(const struct tt_uuid *instance_uuid,
*/
if (wal_dir_lock < 0) {
title("hot_standby");
+ box_status_broadcast();
say_info("Entering hot standby mode");
engine_begin_hot_standby_xc();
recovery_follow_local(recovery, &wal_stream.base, "hot_standby",
@@ -3643,6 +3651,7 @@ box_cfg_xc(void)
}
title("loading");
+ box_status_broadcast();
struct tt_uuid instance_uuid, replicaset_uuid;
box_check_instance_uuid(&instance_uuid);
@@ -3766,6 +3775,7 @@ box_cfg_xc(void)
diag_raise();
title("running");
+ box_status_broadcast();
say_info("ready to accept requests");
if (!is_bootstrap_leader) {
@@ -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();
/*
* 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];
+ 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()));
+
+ 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
@@ -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'}})
+
+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
+ }
+
+ cg.master = cg.cluster:build_server({alias = 'master', engine = engine, box_cfg = box_cfg})
+ cg.cluster:add_server(cg.master)
+ cg.cluster:start()
+end)
+
+
+g.after_each(function(cg)
+ cg.cluster.servers = nil
+ cg.cluster:drop()
+end)
+
+
+g.test_box_status_event = function(cg)
+ cg.master:eval([[
+ i = ''
+ box.watch('box.status', function(...) i = {...} end)
+ ]])
+ t.assert_equals(cg.master:eval("return i"), {"box.status", {is_ro = false, is_ro_cfg = false, status = "running"}})
+
+ 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
--
2.25.1
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [Tarantool-patches] [PATCH] net.box: add predefined system events for pub/sub
2021-12-16 11:22 [Tarantool-patches] [PATCH] net.box: add predefined system events for pub/sub Yan Shtunder via Tarantool-patches
@ 2021-12-17 23:31 ` Vladislav Shpilevoy via Tarantool-patches
0 siblings, 0 replies; 2+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-12-17 23:31 UTC (permalink / raw)
To: Yan Shtunder; +Cc: tarantool-patches
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.
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-12-17 23:31 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-16 11:22 [Tarantool-patches] [PATCH] net.box: add predefined system events for pub/sub Yan Shtunder via Tarantool-patches
2021-12-17 23:31 ` Vladislav Shpilevoy via Tarantool-patches
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox