[Tarantool-patches] [PATCH v4 net.box] Add predefined system events for pub/sub
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Wed Mar 23 01:55:35 MSK 2022
Hi! Good job on the fixes!
See 3 comments below, almost finished!
> Built-in events can't be override. Meaning, users can't be able to call
> box.broadcast(‘box.id’, any_data) etc.
>
> The events are available from the very beginning as not MP_NIL. It will come
> in handy when local subscriptions will be supported (not via the network).
1. 'when local subscriptions will be supported (not via the network).' - they
are already supported. I wrote this sentence before Vova managed to implement
`box.watch()`.
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 45a6b7f41..342f893b8 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -57,9 +57,17 @@
> #include "sequence.h"
> #include "sql.h"
> #include "constraint_id.h"
> +#include "box.h"
>
> /* {{{ Auxiliary functions and methods. */
>
> +static void
> +box_schema_version_bump(void)
> +{
> + ++schema_version;
> + box_broadcast_schema(false);
2. The main problem with adding flags as booleans in function arguments is
that when you call the function, you can't tell from the call what does the
argument mean. For instance, if I don't know/remember the function's signature,
I can't tell what 'false' stands for.
Normally such cases are ironed out by either splitting the function in 2
(inside they can call some third function with this flag as an argument), or
by just not using the flag.
Here, for example, the following can be done - just use these functions only
after box.cfg. Before that, as a first step, you broadcast the sys events
explicitly, manually:
====================
diff --git a/src/box/alter.cc b/src/box/alter.cc
index 342f893b8..a7598a88f 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -65,7 +65,7 @@ static void
box_schema_version_bump(void)
{
++schema_version;
- box_broadcast_schema(false);
+ box_broadcast_schema();
}
static int
@@ -4264,7 +4264,7 @@ on_replace_dd_schema(struct trigger * /* trigger */, void *event)
if (tuple_field_uuid(new_tuple, BOX_CLUSTER_FIELD_UUID, &uu) != 0)
return -1;
REPLICASET_UUID = uu;
- box_broadcast_id(false);
+ box_broadcast_id();
say_info("cluster uuid %s", tt_uuid_str(&uu));
} else if (strcmp(key, "version") == 0) {
if (new_tuple != NULL) {
diff --git a/src/box/box.cc b/src/box/box.cc
index 9be1a5bbf..62693b3db 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -155,7 +155,7 @@ static struct fiber_pool tx_fiber_pool;
static struct cbus_endpoint tx_prio_endpoint;
static void
-box_broadcast_status(bool is_outside_box_cfg);
+box_broadcast_status(void);
static void
title(const char *new_status)
@@ -164,7 +164,7 @@ title(const char *new_status)
title_set_status(new_status);
title_update();
systemd_snotify("STATUS=%s", status);
- box_broadcast_status(false);
+ box_broadcast_status();
}
void
@@ -180,8 +180,8 @@ box_update_ro_summary(void)
if (is_ro_summary)
engine_switch_to_ro();
fiber_cond_broadcast(&ro_cond);
- box_broadcast_status(false);
- box_broadcast_election(false);
+ box_broadcast_status();
+ box_broadcast_election();
}
const char *
@@ -3367,7 +3367,7 @@ bootstrap(const struct tt_uuid *instance_uuid,
else
tt_uuid_create(&INSTANCE_UUID);
- box_broadcast_id(false);
+ box_broadcast_id();
say_info("instance uuid %s", tt_uuid_str(&INSTANCE_UUID));
/*
@@ -3646,10 +3646,15 @@ box_init(void)
sequence_init();
box_raft_init();
box_watcher_init();
- box_broadcast_id(true);
- box_broadcast_status(true);
- box_broadcast_election(true);
- box_broadcast_schema(true);
+
+ /*
+ * Default built-in events to help users distinguish an event being not
+ * supported from box.cfg not being called yet.
+ */
+ box_broadcast_fmt("box.status", "{}");
+ box_broadcast_fmt("box.election", "{}");
+ box_broadcast_fmt("box.id", "{}");
+ box_broadcast_fmt("box.schema", "{}");
}
bool
@@ -3952,22 +3957,17 @@ box_reset_stat(void)
}
void
-box_broadcast_id(bool is_outside_box_cfg)
+box_broadcast_id(void)
{
char buf[1024];
char *w = buf;
-
- if (is_outside_box_cfg)
- w = mp_encode_map(w, 0);
- else {
- w = mp_encode_map(w, 3);
- w = mp_encode_str0(w, "id");
- w = mp_encode_uint(w, instance_id);
- w = mp_encode_str0(w, "instance_uuid");
- w = mp_encode_uuid(w, &INSTANCE_UUID);
- w = mp_encode_str0(w, "replicaset_uuid");
- w = mp_encode_uuid(w, &REPLICASET_UUID);
- }
+ w = mp_encode_map(w, 3);
+ w = mp_encode_str0(w, "id");
+ w = mp_encode_uint(w, instance_id);
+ w = mp_encode_str0(w, "instance_uuid");
+ w = mp_encode_uuid(w, &INSTANCE_UUID);
+ w = mp_encode_str0(w, "replicaset_uuid");
+ w = mp_encode_uuid(w, &REPLICASET_UUID);
box_broadcast("box.id", strlen("box.id"), buf, w);
@@ -3975,22 +3975,17 @@ box_broadcast_id(bool is_outside_box_cfg)
}
static void
-box_broadcast_status(bool is_outside_box_cfg)
+box_broadcast_status(void)
{
char buf[1024];
char *w = buf;
-
- if (is_outside_box_cfg)
- w = mp_encode_map(w, 0);
- else {
- w = mp_encode_map(w, 3);
- w = mp_encode_str0(w, "is_ro");
- w = mp_encode_bool(w, box_is_ro());
- w = mp_encode_str0(w, "is_ro_cfg");
- w = mp_encode_bool(w, cfg_geti("read_only"));
- w = mp_encode_str0(w, "status");
- w = mp_encode_str0(w, box_status());
- }
+ w = mp_encode_map(w, 3);
+ w = mp_encode_str0(w, "is_ro");
+ w = mp_encode_bool(w, box_is_ro());
+ w = mp_encode_str0(w, "is_ro_cfg");
+ w = mp_encode_bool(w, cfg_geti("read_only"));
+ w = mp_encode_str0(w, "status");
+ w = mp_encode_str0(w, box_status());
box_broadcast("box.status", strlen("box.status"), buf, w);
@@ -3998,26 +3993,21 @@ box_broadcast_status(bool is_outside_box_cfg)
}
void
-box_broadcast_election(bool is_outside_box_cfg)
+box_broadcast_election(void)
{
struct raft *raft = box_raft();
char buf[1024];
char *w = buf;
-
- if (is_outside_box_cfg)
- w = mp_encode_map(w, 0);
- else {
- w = mp_encode_map(w, 4);
- w = mp_encode_str0(w, "term");
- w = mp_encode_uint(w, raft->term);
- w = mp_encode_str0(w, "role");
- w = mp_encode_str0(w, raft_state_str(raft->state));
- w = mp_encode_str0(w, "is_ro");
- w = mp_encode_bool(w, box_is_ro());
- w = mp_encode_str0(w, "leader");
- w = mp_encode_uint(w, raft->leader);
- }
+ w = mp_encode_map(w, 4);
+ w = mp_encode_str0(w, "term");
+ w = mp_encode_uint(w, raft->term);
+ w = mp_encode_str0(w, "role");
+ w = mp_encode_str0(w, raft_state_str(raft->state));
+ w = mp_encode_str0(w, "is_ro");
+ w = mp_encode_bool(w, box_is_ro());
+ w = mp_encode_str0(w, "leader");
+ w = mp_encode_uint(w, raft->leader);
box_broadcast("box.election", strlen("box.election"), buf, w);
@@ -4025,18 +4015,13 @@ box_broadcast_election(bool is_outside_box_cfg)
}
void
-box_broadcast_schema(bool is_outside_box_cfg)
+box_broadcast_schema(void)
{
char buf[1024];
char *w = buf;
-
- if (is_outside_box_cfg)
- w = mp_encode_map(w, 0);
- else {
- w = mp_encode_map(w, 1);
- w = mp_encode_str0(w, "version");
- w = mp_encode_uint(w, box_schema_version());
- }
+ w = mp_encode_map(w, 1);
+ w = mp_encode_str0(w, "version");
+ w = mp_encode_uint(w, box_schema_version());
box_broadcast("box.schema", strlen("box.schema"), buf, w);
diff --git a/src/box/box.h b/src/box/box.h
index 41f60c0ea..766568368 100644
--- a/src/box/box.h
+++ b/src/box/box.h
@@ -567,19 +567,19 @@ boxk(int type, uint32_t space_id, const char *format, ...);
* Broadcast the identification of the instance
*/
void
-box_broadcast_id(bool is_outside_box_cfg);
+box_broadcast_id(void);
/**
* Broadcast the current election state of RAFT machinery
*/
void
-box_broadcast_election(bool is_outside_box_cfg);
+box_broadcast_election(void);
/**
* Broadcast the current schema version
*/
void
-box_broadcast_schema(bool is_outside_box_cfg);
+box_broadcast_schema(void);
#if defined(__cplusplus)
} /* extern "C" */
diff --git a/src/box/raft.c b/src/box/raft.c
index 259be639e..efc7e1038 100644
--- a/src/box/raft.c
+++ b/src/box/raft.c
@@ -170,7 +170,7 @@ box_raft_on_update_f(struct trigger *trigger, void *event)
* writable only after it clears its synchro queue.
*/
box_update_ro_summary();
- box_broadcast_election(false);
+ box_broadcast_election();
if (raft->state != RAFT_STATE_LEADER)
return 0;
/*
diff --git a/src/box/replication.cc b/src/box/replication.cc
index 732fe6211..0da256721 100644
--- a/src/box/replication.cc
+++ b/src/box/replication.cc
@@ -247,7 +247,7 @@ replica_set_id(struct replica *replica, uint32_t replica_id)
/* Assign local replica id */
assert(instance_id == REPLICA_ID_NIL);
instance_id = replica_id;
- box_broadcast_id(false);
+ box_broadcast_id();
} else if (replica->anon) {
/*
* Set replica gc on its transition from
@@ -288,7 +288,7 @@ replica_clear_id(struct replica *replica)
/* See replica_check_id(). */
assert(replicaset.is_joining);
instance_id = REPLICA_ID_NIL;
- box_broadcast_id(false);
+ box_broadcast_id();
}
replica->id = REPLICA_ID_NIL;
say_info("removed replica %s", tt_uuid_str(&replica->uuid));
====================
Now on each box_broadcast_<sys_event>() call you can tell that it
just broadcasts the event. No flags, not other parameters, simple to
read.
> diff --git a/test/box-luatest/gh_6260_add_builtin_events_test.lua b/test/box-luatest/gh_6260_add_builtin_events_test.lua
> new file mode 100644
> index 000000000..4170ce420
> --- /dev/null
> +++ b/test/box-luatest/gh_6260_add_builtin_events_test.lua
<...>
> +g.test_sys_events_no_override = function(cg)
> + local sys_events = {'box.id', 'box.status', 'box.election', 'box.schema'}
> +
> + for i = 1, #sys_events do
> + t.assert_error_msg_content_equals("System event can't be override",
> + function()
> + cg.master:exec(function(sys_events, i)
> + box.broadcast(sys_events[i], 'any_data')
> + end, {sys_events, i})
> + end)
> + end
3. Lua supports range loops. You can do this:
====================
@@ -39,12 +39,12 @@ end)
g.test_sys_events_no_override = function(cg)
local sys_events = {'box.id', 'box.status', 'box.election', 'box.schema'}
- for i = 1, #sys_events do
+ for _, key in pairs(sys_events) do
t.assert_error_msg_content_equals("System event can't be override",
function()
- cg.master:exec(function(sys_events, i)
- box.broadcast(sys_events[i], 'any_data')
- end, {sys_events, i})
+ cg.master:exec(function(key)
+ box.broadcast(key, 'any_data')
+ end, {key})
end)
end
====================
Up to you.
Technically everything looks good except for the nits above. Now you also
should comb the patch to make the linters happy: luacheck and checkpatch.
To run luacheck you need to install it. How - depends on your system. In
Linux I think it would be something like
sudo apt install -y lua5.1 luarocks
sudo luarocks install luacheck
Or
tarantoolctl rocks install luacheck (I didn't validate this)
Then you run it with our config. This is how I do it in tarantool's root
dir (when I want to check 'test/' folder):
luacheck --codes --config .luacheckrc test/
Then for checkpatch you need to clone the repository:
https://github.com/tarantool/checkpatch
For instance, I cloned it just in a neighbour folder near my tarantool/
repo. Then from inside of tarantool/ I do this:
../checkpatch/checkpatch.pl --color=always --git head
to check the latest commit. I suppose you can also specify head~1 for a
previous one, head~2 for pre-previous and so on. Maybe it also supports
git hash ranges, I didn't check yet. On your commit this linter raises some
errors.
More information about the Tarantool-patches
mailing list