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 D79286F879; Thu, 27 Jan 2022 03:23:21 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org D79286F879 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1643243002; bh=rAO98lyA0DNosqRo2+9NjF7xxbJy50fRpp5Zq6axtg4=; 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=FZxqZz7hHTFl+zrcO90GcuILqxBdtyGHWptR+njI89qf2RPDN5lFE5P2angKObZw2 Xm5PXaUGyCbzn8MYwQasjRGCvRFLySnhrchj+ojBqCB3oGxpTtEsJfi0lMIiXjCe6f 5gYxTdI74NY4LBOIL7CnsDsZqhS5BX0eLzXPSIGk= 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 31A656F879 for ; Thu, 27 Jan 2022 03:23:20 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 31A656F879 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1nCsZL-0005ma-D5; Thu, 27 Jan 2022 03:23:19 +0300 Message-ID: <11272290-1065-2602-4b09-ebe8102d2912@tarantool.org> Date: Thu, 27 Jan 2022 01:23:18 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.5.0 Content-Language: en-US To: Yan Shtunder Cc: tarantool-patches@dev.tarantool.org References: <20220120040640.114751-1-ya.shtunder@gmail.com> In-Reply-To: <20220120040640.114751-1-ya.shtunder@gmail.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9AA78FDF62ECAE61F574C814AB3F23F4AA01FB0D4144D4AE0182A05F53808504019CB7FF2EF50CBDAF81FB0EBC0942D1FB09129844285E644FA4A6B4D8CD83D7E X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE79683A3C835791080EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006377A58729D825217838638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8024850659D2BD38B34FFFF84A0EDC6F9117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCF1175FABE1C0F9B6A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18BDFBBEFFF4125B51D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6300D3B61E77C8D3B089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A5A17101ABDDE2B2C59470AF15562C0EF53219DB7C4E748A0BD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75438CC92D4039F4E2410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34E229BE567979C9407AAFD336A32539CEB8EC58A506F0BEFA0F9B766E5FE806D5EC2E06711F6859DC1D7E09C32AA3244C91FCC6102DA35E1948B81FAB5C98916005AB220A9D022EBC729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojUxqqD3NEEG2FLLSdBZJs3A== X-Mailru-Sender: 689FA8AB762F739339CABD9B3CA9A7D6C920082CC6107AACFB8A167D02359FC13841015FED1DE5223CC9A89AB576DD93FB559BB5D741EB963CF37A108A312F5C27E8A8C3839CE0E25FEEDEB644C299C0ED14614B50AE0675 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v2] 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! Thanks for the fixes, good job! Please, next time watch out for which button you click - 'Reply' or 'Reply all'. You need to keep the mailing list as one of the receivers. Otherwise nobody else will see your responses. I paste your response below. Lets rebase your branch on the latest master? Your base version is quite outdated - from 7 December 2021. > 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_*. > > > static void > box_broadcast_status(void); > > 2. >> >> @@ -377,9 +382,11 @@ box_set_orphan(bool orphan) >> >> if (is_orphan) { >> say_crit("entering orphan mode"); >> title("orphan"); >> + box_status_broadcast(); > > You can call this inside of title() function instead of duplicating its broadcast for each title() invocation. > > > static void title(const char *new_status) > { > snprintf(status, sizeof(status), "%s", new_status); > title_set_status(new_status); > title_update(); > systemd_snotify("STATUS=%s", status); > /* Checking box.info.status change */ > box_broadcast_status(); > } > > 3. 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. > > > I fixed this. It might be easier to return to the review context if you would keep in your response my comments together with their context. For instance, look at your response above - we can't see what was the context. Lets try to improve this part a little next time, shall we? > 4. 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. > > > https://github.com/tarantool/msgpuck/commit/7f343a40897cbf070c897886e15eb3d761d322f4 Good! Now you need to send it as another patch to the mailing list. Or as a PR to msgpuck repository. If you will use mailing list, you can look for example of formatting such patches (not in the main repository) in my vshard patches and luajit. The process is all the same. You just specify the subsystem in mail title: [Tarantool-patches] [PATCH luajit] test: adapt test checking global environment [Tarantool-patches] [PATCH vshard 0/5] Router backoff, storage disable Look - "luajit", "vshard" after the word PATCH. But if you feel a bit overwhelmed, it is ok to use PRs. > Adding predefined system event box.status and box.election See 6 comments below. 1. It might be easier for you to split these into 2 separate commits. With their own tests. To make the patch smaller and easier to review. Up to you. > Part of #6260 > --- > Issue: https://github.com/tarantool/tarantool/issues/6260 > Patch: https://github.com/tarantool/tarantool/tree/yshtunder/gh-6260-events-for-pub-sub > > src/box/box.cc | 69 +++++- > src/box/box.h | 6 + > src/box/mp_error.cc | 6 - > src/box/raft.c | 1 + > src/lib/msgpuck | 2 +- > .../gh_6260_add_builtin_events.lua | 229 ++++++++++++++++++ > test/unit/mp_error.cc | 6 - > 7 files changed, 299 insertions(+), 20 deletions(-) > create mode 100644 test/app-luatest/gh_6260_add_builtin_events.lua > > diff --git a/src/box/box.cc b/src/box/box.cc > index 0413cbf44..dc61dc77b 100644 > --- a/src/box/box.cc > +++ b/src/box/box.cc > @@ -97,13 +97,6 @@ double txn_timeout_default; > struct rlist box_on_shutdown_trigger_list = > RLIST_HEAD_INITIALIZER(box_on_shutdown_trigger_list); > > -static void title(const char *new_status) > -{ > - snprintf(status, sizeof(status), "%s", new_status); > - title_set_status(new_status); > - title_update(); > - systemd_snotify("STATUS=%s", status); > -} > > const struct vclock *box_vclock = &replicaset.vclock; > > @@ -160,6 +153,19 @@ static struct fiber_pool tx_fiber_pool; > */ > static struct cbus_endpoint tx_prio_endpoint; > > +static void > +box_broadcast_status(void); > + > +static void title(const char *new_status) > +{ > + snprintf(status, sizeof(status), "%s", new_status); > + title_set_status(new_status); > + title_update(); > + systemd_snotify("STATUS=%s", status); > + /* Checking box.info.status change */ > + box_broadcast_status(); > +} 2. Since you decided to move the function 'title', lets also make it a bit cleaner - put 'static void' on a separate line. Originally it wasn't so because the function apparently is very old. Was added before these rules. > + > void > box_update_ro_summary(void) > { > @@ -173,6 +179,9 @@ box_update_ro_summary(void) > if (is_ro_summary) > engine_switch_to_ro(); > fiber_cond_broadcast(&ro_cond); > + /* Checking box.info.ro change */ > + box_broadcast_status(); > + box_broadcast_election(); > } > > const char * > @@ -3903,3 +3912,49 @@ box_reset_stat(void) > engine_reset_stat(); > space_foreach(box_reset_space_stat, NULL); > } > + > +void 3. It should be 'static'. You never use it outside of this .cc file. > +box_broadcast_status(void) > +{ > + char buf[1024]; > + char *w = buf; > + > + 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); > + > + assert((size_t)(w - buf) < 1024); > +} > + > +void > +box_broadcast_election() > +{ > + struct raft *raft = box_raft(); > + > + char buf[1024]; > + char *w = buf; > + > + 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); > + > + assert((size_t)(w - buf) < 1024); > +} > diff --git a/src/box/box.h b/src/box/box.h > index b594f6646..2eb1ef437 100644 > --- a/src/box/box.h > +++ b/src/box/box.h > @@ -563,6 +563,12 @@ box_process_rw(struct request *request, struct space *space, > int > boxk(int type, uint32_t space_id, const char *format, ...); > > +/** > + * 4. Please, write a proper comment here. > + */ > +void > +box_broadcast_election(void); > + > #if defined(__cplusplus) > } /* extern "C" */ > #endif /* defined(__cplusplus) */ > diff --git a/src/box/mp_error.cc b/src/box/mp_error.cc > index fba562a84..006b5f2d9 100644 > --- a/src/box/mp_error.cc > +++ b/src/box/mp_error.cc > @@ -168,12 +168,6 @@ mp_sizeof_error_one(const struct error *error) > return data_size; > } > > -static inline char * > -mp_encode_str0(char *data, const char *str) > -{ > - return mp_encode_str(data, str, strlen(str)); > -} 5. This should be done in a separate commit. Together with msgpuck submodule update. > - > static char * > mp_encode_error_one(char *data, const struct error *error) > { > diff --git a/src/box/raft.c b/src/box/raft.c > index 1e360dc88..b2d4002fe 100644 > --- a/src/box/raft.c > +++ b/src/box/raft.c > @@ -170,6 +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(); > if (raft->state != RAFT_STATE_LEADER) > return 0; > /* > diff --git a/src/lib/msgpuck b/src/lib/msgpuck > index 7f95b6fd7..7f343a408 160000 > --- a/src/lib/msgpuck > +++ b/src/lib/msgpuck > @@ -1 +1 @@ > -Subproject commit 7f95b6fd7a5b928cfcbdab2d78bea88d1685821e > +Subproject commit 7f343a40897cbf070c897886e15eb3d761d322f4 > diff --git a/test/app-luatest/gh_6260_add_builtin_events.lua b/test/app-luatest/gh_6260_add_builtin_events.lua > new file mode 100644 > index 000000000..4a4e1feff > --- /dev/null > +++ b/test/app-luatest/gh_6260_add_builtin_events.lua > @@ -0,0 +1,229 @@ > +local t = require('luatest') > +local net = require('net.box') > +local cluster = require('test.luatest_helpers.cluster') > +local helpers = require('test.luatest_helpers') > + > +local g = t.group('gh_6260') > + > +g.before_test('test_box_status_event', function(cg) > + cg.cluster = cluster:new({}) > + > + local box_cfg = { > + read_only = false > + } > + > + cg.master = cg.cluster:build_server({alias = 'master', box_cfg = box_cfg}) > + cg.cluster:add_server(cg.master) > + cg.cluster:start() > +end) > + > + > +g.after_test('test_box_status_event', function(cg) > + cg.cluster.servers = nil > + cg.cluster:drop() > +end) > + > + > +g.test_box_status_event= function(cg) > + local c = net.connect(cg.master.net_box_uri) > + > + c:eval([[ > + i = '' > + box.watch('box.status', function(...) i = {...} end) > + ]]) 6. Sorry, it seems there was a misunderstanding. When I meant we need netbox tests, I meant of netbox subscriptions. You did the same as in v1: you used eval to test box.watch locally on the storages. What I would want you to do is to test `c:watch()`. Netbox connections have the same subscription API as box. box.watch() should be tested too, but we shouldn't ignore `connection:watch()`. Usage of events by remote clients like netbox is the main motivation of subscriptions.