From: Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Yan Shtunder <ya.shtunder@gmail.com> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH v4 net.box] Add predefined system events for pub/sub Date: Tue, 22 Mar 2022 23:55:35 +0100 [thread overview] Message-ID: <08a46abd-1fa8-8c3a-e1af-478c663d6c42@tarantool.org> (raw) In-Reply-To: <20220322115102.117727-1-ya.shtunder@gmail.com> 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.
next prev parent reply other threads:[~2022-03-22 22:55 UTC|newest] Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-03-22 11:51 Yan Shtunder via Tarantool-patches 2022-03-22 22:55 ` Vladislav Shpilevoy via Tarantool-patches [this message] -- strict thread matches above, loose matches on Subject: below -- 2022-03-24 14:01 Yan Shtunder via Tarantool-patches 2022-03-25 23:44 ` Vladislav Shpilevoy via Tarantool-patches 2022-02-28 13:32 Yan Shtunder via Tarantool-patches 2022-03-01 22:44 ` Vladislav Shpilevoy via Tarantool-patches 2022-02-22 12:56 Yan Shtunder via Tarantool-patches 2022-02-23 22:44 ` Vladislav Shpilevoy via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=08a46abd-1fa8-8c3a-e1af-478c663d6c42@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --cc=ya.shtunder@gmail.com \ --subject='Re: [Tarantool-patches] [PATCH v4 net.box] Add predefined system events for pub/sub' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox