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

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

[Yan Shtunder via Tarantool-patches]

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

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

[Vladislav Shpilevoy via 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.