From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id EE4116ECDB; Wed, 23 Mar 2022 01:55:39 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org EE4116ECDB DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1647989740; bh=ItP9nKPL4xoDUWrcNeLJdSgtJ25+2H2tWeHaKKJ4RnA=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=CHZQvsXh2K/95s7DflRScxGtKTUqAW9uTyNwlzsmSh8vFATdBeQAMU/OVT0dW4c8R qZMy8o2xfmybzrSjDZZ4m96AFJWorrTCqtFouTNdZjScpCipRGMgcJtszdDHff+ON9 Jf2KkLucBj+NIrB+CyLj0j2qdLLaKYbY0Yfk+35M= Received: from smtpng1.i.mail.ru (smtpng1.i.mail.ru [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id C34186ECDB for ; Wed, 23 Mar 2022 01:55:37 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org C34186ECDB Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1nWnPc-0000GE-UR; Wed, 23 Mar 2022 01:55:37 +0300 Message-ID: <08a46abd-1fa8-8c3a-e1af-478c663d6c42@tarantool.org> Date: Tue, 22 Mar 2022 23:55:35 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 Content-Language: en-US To: Yan Shtunder Cc: tarantool-patches@dev.tarantool.org References: <20220322115102.117727-1-ya.shtunder@gmail.com> In-Reply-To: <20220322115102.117727-1-ya.shtunder@gmail.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD95983D7D89D92196D8C1F8FDC6468E3832CA42EF3B8AD4A16182A05F538085040AB92545D3295C32FB26159C65C5E765A23A195355C48CAE0078818BB4D2B908B X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE71EEA4C46C73542F4EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637E1D2769089B3DFB28638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D80F649C449CE0E0D8AC4B6A51F71D0915117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCF1175FABE1C0F9B6A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD186FD1C55BDD38FC3FD2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6AC294AFEFA671E80089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-8FC586DF: 6EFBBC1D9D64D975 X-C1DE0DAB: 0D63561A33F958A5CC224D7A5CE576B30FE6215FB07179081C6053818A725B3DD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75F78D6440C3F49C15410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D344E332383F80D58BB194843F7B283032DD6F1956A405EB41755BE017CBF1F2CFBB4A19F017F7BCEF11D7E09C32AA3244CF6C3D8404E71CBC6530A186120EF8FDBA95CA90A1D8AC565729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojYypBK0DWCMrTidJoKftqDw== X-Mailru-Sender: 689FA8AB762F739339CABD9B3CA9A7D656292A84ED3D8791900F58D487D1E9483841015FED1DE5223CC9A89AB576DD93FB559BB5D741EB963CF37A108A312F5C27E8A8C3839CE0E25FEEDEB644C299C0ED14614B50AE0675 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v4 net.box] Add predefined system events for pub/sub X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Vladislav Shpilevoy via Tarantool-patches Reply-To: Vladislav Shpilevoy Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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_() 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.