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 0D8636E454; Thu, 24 Feb 2022 01:44:21 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 0D8636E454 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1645656261; bh=iEU+X+hvjRtvo+f9fz5ValgC2wvKDNe5PaNuWdHu6Ek=; 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=TR2tPDdTXfxqkJap7QpME9Tw6rdvcMjfekJqgJsT1KXe79VbmWeBwkaj7VvmBKpyK x+Y/3TrLQQafesxxSPEHXLc/VjAcGCHWvUpqkaIAMTLdshywAVB2XI49L3acJtNDMr S+HIdG7fkilXFDADluFWjbkaqM5KfsMa+jff9x4c= 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 C01636E454 for ; Thu, 24 Feb 2022 01:44:18 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org C01636E454 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1nN0Ms-0000NR-07; Thu, 24 Feb 2022 01:44:18 +0300 Message-ID: Date: Wed, 23 Feb 2022 23:44:17 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.6.1 Content-Language: en-US To: Yan Shtunder Cc: tarantool-patches@dev.tarantool.org References: <20220222125643.42705-1-ya.shtunder@gmail.com> In-Reply-To: <20220222125643.42705-1-ya.shtunder@gmail.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD95E12778296193B1FE89A4E5A38A44C937401DB1D34488C7B00894C459B0CD1B9878CAD55573DDEB3A9870BEAB7FD829A85E89376E9CC4ADB45849E6DFAE0F427 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7EC0B1A4921CAE631EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006378CCCB41504E044EE8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D89C023C27DEBA657764362140BC240C7A117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC2EE5AD8F952D28FBA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F44604297287769387670735201E561CDFBCA1751F2CC0D3CB04F14752D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B66F6A3E018CF4DC80089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-8FC586DF: 6EFBBC1D9D64D975 X-C1DE0DAB: 0D63561A33F958A5FEDF0DAF03E8A62D6545EAA4181F92D10588D13FC4F8BCAFD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75EEFE94893D2DF0F4410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D345B8C2BEC800D10210222601E43086C0C582CD6C34B920FC3D28163EC8DDC21E172506FF1001E10A61D7E09C32AA3244C258CCCBE3A02C7BBB7BB013683E18E8AF94338140B71B8EE729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojiJrs9LMZ5DjJl5MBYKlfog== X-Mailru-Sender: 689FA8AB762F739339CABD9B3CA9A7D6F5C28651F9A9E5D6EA9AD8CAB467CFF13841015FED1DE5223CC9A89AB576DD93FB559BB5D741EB963CF37A108A312F5C27E8A8C3839CE0E25FEEDEB644C299C0ED14614B50AE0675 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! Thanks for the patch, good job! See some comments below. On 22.02.2022 13:56, Yan Shtunder wrote: > Added predefined system events: box.status, box.id, box.election and > box.schema. > > Closes #6260 > > @TarantoolBot document > Title: Built-in events for pub/sub > > Built-in events are needed, first of all, in order to learn who is the > master, unless it is defined in an application specific way. Knowing who > is the master is necessary to send changes to a correct instance, and > probably make reads of the most actual data if it is important. Also > defined more built-in events for other mutable properties like replication > list of the server, schema version change and instance state. 1. There is no an event about replication list of the server. Also you need to provide more details. Which events are introduced? You didn't give names. Which fields each of them has? There are names in the example below, but it does not reveal the events' content. > Example usage: > > * Client: > ```lua > conn = net.box.connect(URI) > -- Subscribe to updates of key 'box.id' > w = conn:watch('box.id', function(key, value) > assert(key == 'box.id') > -- do something with value > end) > -- or to updates of key 'box.status' > w = conn:watch('box.status', function(key, value) > assert(key == 'box.status') > -- do something with value > end) > -- or to updates of key 'box.election' > w = conn:watch('box.election', function(key, value) > assert(key == 'box.election') > -- do something with value > end) > -- or to updates of key 'box.schema' > w = conn:watch('box.schema', function(key, value) > assert(key == 'box.schema') > -- do something with value > end) > -- Unregister the watcher when it's no longer needed. > w:unregister() > ``` > --- > Issue: https://github.com/tarantool/tarantool/issues/6260 > Patch: https://github.com/tarantool/tarantool/tree/yshtunder/gh-6260-events-v4 > > diff --git a/src/box/alter.cc b/src/box/alter.cc > index b85d279e3..cd48cd6aa 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -57,6 +57,14 @@ > #include "sequence.h" > #include "sql.h" > #include "constraint_id.h" > +#include "box.h" > + > +static void > +box_schema_version_bump(void) > +{ > + ++schema_version; > + box_broadcast_schema(); > +} 2. Please, move it below the comment on the next line. To keep the functions grouped. > /* {{{ Auxiliary functions and methods. */ > > 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..c11d79e30 > --- /dev/null > +++ b/test/box-luatest/gh_6260_add_builtin_events_test.lua > @@ -0,0 +1,294 @@ > +local t = require('luatest') > +local net = require('net.box') > +local cluster = require('test.luatest_helpers.cluster') > +local server = require('test.luatest_helpers.server') > +local fiber = require('fiber') > + > +local g = t.group('gh_6260') > + > +g.before_test('test_box_status', function(cg) > + cg.cluster = cluster:new({}) > + cg.master = cg.cluster:build_server({alias = 'master'}) > + cg.cluster:add_server(cg.master) > + cg.cluster:start() > +end) > + > +g.after_test('test_box_status', function(cg) > + cg.cluster.servers = nil > + cg.cluster:drop() > +end) > + > +g.test_box_status = function(cg) > + local c = net.connect(cg.master.net_box_uri) > + > + local result = {} > + local result_no = 0 > + local watcher = c:watch('box.status', > + function(name, state) > + assert(name == 'box.status') > + result = state > + result_no = result_no + 1 > + end) > + > + -- initial state should arrive > + t.helpers.retrying({}, function() > + while result_no < 1 do fiber.sleep(0.000001) end > + end) 3. With retrying() you are not supposed to have 'while' loop. It is done inside of the function for a limited time. The problem with custom 'while' loops is that people usually do not add any timeout and do not handle exceptions. Please, take a look again at other usages of retrying() for a reference. The same in the similar places below. > + > + t.assert_equals(result, > + {is_ro = false, is_ro_cfg = false, status = 'running'}) > + > + -- test orphan status appearance > + cg.master:exec(function(repl) > + box.cfg{ > + replication = repl, > + replication_connect_timeout = 0.001, 4. Indentation is broken. > + replication_timeout = 0.001, > + } > + end, {{server.build_instance_uri('master'), server.build_instance_uri('replica')}}) 5. You are out of 80 symbols. > + -- here we have 2 notifications: entering ro when can't connect > + -- to master and the second one when going orphan > + t.helpers.retrying({}, function() > + while result_no < 3 do fiber.sleep(0.000001) end > + end) > + t.assert_equals(result, > + {is_ro = true, is_ro_cfg = false, status = 'orphan'}) <...> > +g.test_box_election = function(cg) > + local c = {} > + c[1] = net.connect(cg.instance_1.net_box_uri) > + c[2] = net.connect(cg.instance_2.net_box_uri) > + c[3] = net.connect(cg.instance_3.net_box_uri) > + > + local res = {} > + local res_n = {0, 0, 0} > + > + for i = 1, 3 do > + c[i]:watch('box.election', > + function(n, s) > + t.assert_equals(n, 'box.election') > + res[i] = s > + res_n[i] = res_n[i] + 1 > + end) > + end > + t.helpers.retrying({}, function() > + while res_n[1] + res_n[2] + res_n[3] < 3 do fiber.sleep(0.00001) end > + end) > + > + -- verify all instances are in the same state > + t.assert_equals(res[1], res[2]) > + t.assert_equals(res[1], res[3]) > + > + -- wait for elections to complete, verify leader is the instance_1 > + -- trying to avoid the exact number of term - it can vary 6. You just said "trying to avoid the exact number of term" and used an exact number of term a few lines below. Lets better not rely on an exact term number. It is enough to test that it exists and is the same in all 3 events. t.assert_covers() allows to omit certain fields. Because it is not 'equal', it is 'covers'. A subset of fields is enough. The others can be checked in a special way. Like term. > + local instance1_id = cg.instance_1:instance_id() > + > + cg.instance_1:exec(function() box.cfg{election_mode='candidate'} end) > + cg.instance_2:exec(function() box.cfg{election_mode='voter'} end) > + cg.instance_3:exec(function() box.cfg{election_mode='voter'} end) > + > + cg.instance_1:wait_election_leader_found() > + cg.instance_2:wait_election_leader_found() > + cg.instance_3:wait_election_leader_found() > + > + t.assert_covers(res, > + { > + {leader = instance1_id, is_ro = false, role = 'leader', term = 2}, > + {leader = instance1_id, is_ro = true, role = 'follower', term = 2}, > + {leader = instance1_id, is_ro = true, role = 'follower', term = 2} > + }) <...> > Hi! Thank you for the review! Lets next time put the comment responses before the new patch. I noticed this part of the email only when finished the new review.