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 385756EC55; Wed, 28 Jul 2021 13:03:36 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 385756EC55 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1627466616; bh=bCnz0YMeSEYM942OPa7/eExf38GfnZhR/PnmjXEVVSk=; h=To:Cc:References:Date:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=oq4lDIJgEmu8JOh0Jff/rCKeTPwp4+Txbw4FfZ/+Wy4+YRIotYquILKNNOABgg3LL w5rlEsHRJjbKcvmmSQ/5popI6qcSY6Ga2vWkEcAEqgHUuYoFHpUSzzIJnLbnv1rGwS wMBgaIfR8VVTFwJq8V7pMn0h92XSV/+IoFO0P8QE= Received: from smtp39.i.mail.ru (smtp39.i.mail.ru [94.100.177.99]) (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 1A7F46EC55 for ; Wed, 28 Jul 2021 13:03:34 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 1A7F46EC55 Received: by smtp39.i.mail.ru with esmtpa (envelope-from ) id 1m8gPT-0003rm-Is; Wed, 28 Jul 2021 13:03:32 +0300 To: Vladislav Shpilevoy , gorcunov@gmail.com Cc: tarantool-patches@dev.tarantool.org References: <20210722072002.13145-1-sergepetrenko@tarantool.org> <27aaa450-7c0c-1e5e-e912-db5bae282e16@tarantool.org> Message-ID: <81a59f0a-6703-6a18-aaaa-7ea98f4a8146@tarantool.org> Date: Wed, 28 Jul 2021 13:03:30 +0300 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.12.0 MIME-Version: 1.0 In-Reply-To: <27aaa450-7c0c-1e5e-e912-db5bae282e16@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD941C43E597735A9C36A98DBA789EBB6AE26DB9A6C1D9BF7E0182A05F538085040B475B71B7F503975B2D4D6E25F42F02C20F85679EA8723958C97CDCAA391F80D X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7EB1B9B9F97B01B97EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637A0569EA9A35E44F48638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D874ECC9BD333C233B0F522C6E84D292D6117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8BAA867293B0326636D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE0A3850AC1BE2E735C43C4D2AE8146675D8FC6C240DEA7642DBF02ECDB25306B2B78CF848AE20165D0A6AB1C7CE11FEE37812A6222701F215040F9FF01DFDA4A8C4224003CC836476EA7A3FFF5B025636E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F79006377870F476E0DB9443EFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-C1DE0DAB: 0D63561A33F958A5055F9FC4879B6C8F39E026727B0F5154BA3DAB466090AE22D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75D299BBC8C521FD19410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3433E9BC74ABA5769F8C6C9B65C4A4099A934905D71CA5A5DF51D6A3C45941C83FBC58199CC199D2CC1D7E09C32AA3244CCB5FBCC8E23B07E70188F18FD64E3E26B018FE5BB746DCD1FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojiF1u9eOpfTS39Be/FYD/DA== X-Mailru-Sender: 9482C2B233321BD23A6BA2D3EE5718956D2DAA24B516969BE553F4E31B8631A4A32FDEA59E113C286BB2E709EA627F343C7DDD459B58856F0E45BC603594F5A135B915D4279FF0579437F6177E88F7363CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH] box: introduce on_election triggers 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: Sergey Petrenko via Tarantool-patches Reply-To: Sergey Petrenko Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 26.07.2021 23:27, Vladislav Shpilevoy пишет: > Hi! Thanks for the patch! > > See 3 comments below. Thanks for the review! >> diff --git a/src/box/raft.c b/src/box/raft.c >> index 7f787c0c5..c9c3ba818 100644 >> --- a/src/box/raft.c >> +++ b/src/box/raft.c >> @@ -52,6 +52,8 @@ enum election_mode box_election_mode = ELECTION_MODE_INVALID; >> */ >> static struct trigger box_raft_on_update; >> >> +struct rlist box_raft_on_state = RLIST_HEAD_INITIALIZER(box_raft_on_state); > 1. I propose to call it 'box_raft_on_update'. We usually use 'on_' > naming pattern. box_raft_on_update is already occupied by internal triggers which can't yield. I propose box_raft_on_broadcast then. Besides, the triggers are really run on a broadcast. > >> + >> /** >> * Worker fiber does all the asynchronous work, which may need yields and can be >> * long. These are WAL writes, network broadcasts. That allows not to block the >> @@ -274,6 +276,7 @@ box_raft_broadcast(struct raft *raft, const struct raft_msg *msg) >> assert(raft == box_raft()); >> struct raft_request req; >> box_raft_msg_to_request(msg, &req); >> + trigger_run(&box_raft_on_state, NULL); > 2. Maybe call it after the state is pushed to the relays? So as it > would be sent earlier in case the triggers are going to yield. Ok, no problem. > >> replicaset_foreach(replica) >> relay_push_raft(replica->relay, &req); >> } >> diff --git a/test/replication/gh-5819-election-triggers.result b/test/replication/gh-5819-election-triggers.result >> new file mode 100644 >> index 000000000..546aac8a2 >> --- /dev/null >> +++ b/test/replication/gh-5819-election-triggers.result > 3. Hm, I think we are supposed to use gh-#### file naming only for > bugfixes. > https://github.com/tarantool/tarantool/wiki/Code-review-procedure#testing > > This is a feature, hence can have a normal name, or be in an existing > test file which is also not bound to a single ticket. Yeah, sorry. Here's the diff: ============================= diff --git a/src/box/lua/ctl.c b/src/box/lua/ctl.c index 4b2a467d8..4a9212f33 100644 --- a/src/box/lua/ctl.c +++ b/src/box/lua/ctl.c @@ -85,7 +85,7 @@ lbox_ctl_on_schema_init(struct lua_State *L)  static int  lbox_ctl_on_election(struct lua_State *L)  { -       return lbox_trigger_reset(L, 2, &box_raft_on_state, NULL, NULL); +       return lbox_trigger_reset(L, 2, &box_raft_on_broadcast, NULL, NULL);  }  static int diff --git a/src/box/raft.c b/src/box/raft.c index c9c3ba818..38cfc0cf3 100644 --- a/src/box/raft.c +++ b/src/box/raft.c @@ -52,7 +52,8 @@ enum election_mode box_election_mode = ELECTION_MODE_INVALID;   */  static struct trigger box_raft_on_update; -struct rlist box_raft_on_state = RLIST_HEAD_INITIALIZER(box_raft_on_state); +struct rlist box_raft_on_broadcast = +       RLIST_HEAD_INITIALIZER(box_raft_on_broadcast);  /**   * Worker fiber does all the asynchronous work, which may need yields and can be @@ -276,9 +277,9 @@ box_raft_broadcast(struct raft *raft, const struct raft_msg *msg)         assert(raft == box_raft());         struct raft_request req;         box_raft_msg_to_request(msg, &req); -       trigger_run(&box_raft_on_state, NULL);         replicaset_foreach(replica)                 relay_push_raft(replica->relay, &req); +       trigger_run(&box_raft_on_broadcast, NULL);  }  static void diff --git a/src/box/raft.h b/src/box/raft.h index dbe16eebf..125bc64e0 100644 --- a/src/box/raft.h +++ b/src/box/raft.h @@ -37,10 +37,10 @@ extern "C" {  #endif  /** - * A public trigger fired on Raft state change. It's allowed to yield inside - * it, and it's run asynchronously. ~/Documents/Source/tarantool/test ❯ git diff > 1.dff ~/Documents/Source/tarantool/test ❯ cat 1.dff diff --git a/src/box/lua/ctl.c b/src/box/lua/ctl.c index 4b2a467d8..4a9212f33 100644 --- a/src/box/lua/ctl.c +++ b/src/box/lua/ctl.c @@ -85,7 +85,7 @@ lbox_ctl_on_schema_init(struct lua_State *L)  static int  lbox_ctl_on_election(struct lua_State *L)  { -    return lbox_trigger_reset(L, 2, &box_raft_on_state, NULL, NULL); +    return lbox_trigger_reset(L, 2, &box_raft_on_broadcast, NULL, NULL);  }  static int diff --git a/src/box/raft.c b/src/box/raft.c index c9c3ba818..38cfc0cf3 100644 --- a/src/box/raft.c +++ b/src/box/raft.c @@ -52,7 +52,8 @@ enum election_mode box_election_mode = ELECTION_MODE_INVALID;   */  static struct trigger box_raft_on_update; -struct rlist box_raft_on_state = RLIST_HEAD_INITIALIZER(box_raft_on_state); +struct rlist box_raft_on_broadcast = +    RLIST_HEAD_INITIALIZER(box_raft_on_broadcast);  /**   * Worker fiber does all the asynchronous work, which may need yields and can be @@ -276,9 +277,9 @@ box_raft_broadcast(struct raft *raft, const struct raft_msg *msg)      assert(raft == box_raft());      struct raft_request req;      box_raft_msg_to_request(msg, &req); -    trigger_run(&box_raft_on_state, NULL);      replicaset_foreach(replica)          relay_push_raft(replica->relay, &req); +    trigger_run(&box_raft_on_broadcast, NULL);  }  static void diff --git a/src/box/raft.h b/src/box/raft.h index dbe16eebf..125bc64e0 100644 --- a/src/box/raft.h +++ b/src/box/raft.h @@ -37,10 +37,10 @@ extern "C" {  #endif  /** - * A public trigger fired on Raft state change. It's allowed to yield inside - * it, and it's run asynchronously. + * A public trigger fired on Raft state change, i.e. on a broadcast. + * It's allowed to yield inside it, and it's run asynchronously.   */ -extern struct rlist box_raft_on_state; +extern struct rlist box_raft_on_broadcast;  enum election_mode {      ELECTION_MODE_INVALID = -1, diff --git a/test/replication/election_basic.result b/test/replication/election_basic.result index b64028c60..fc7121d7f 100644 --- a/test/replication/election_basic.result +++ b/test/replication/election_basic.result @@ -275,3 +275,147 @@ test_run:cmd(string.format('start server %s', leader_name))  test_run:drop_cluster(SERVERS)   | ---   | ... + +-- gh-5819: on_election triggers, that are filed on every visible state change. +box.schema.user.grant('guest', 'replication') + | --- + | ... +test_run:cmd('create server replica with rpl_master=default,\ +          script="replication/replica.lua"') + | --- + | - true + | ... +test_run:cmd('start server replica') + | --- + | - true + | ... + +repl = test_run:eval('replica', 'return box.cfg.listen')[1] + | --- + | ... +box.cfg{replication = repl} + | --- + | ... +test_run:switch('replica') + | --- + | - true + | ... +box.cfg{election_mode='voter'} + | --- + | ... +test_run:switch('default') + | --- + | - true + | ... + +election_tbl = {} + | --- + | ... + +function trig()\ +    election_tbl[#election_tbl+1] = box.info.election\ +end + | --- + | ... + +_ = box.ctl.on_election(trig) + | --- + | ... + +box.cfg{replication_synchro_quorum=2} + | --- + | ... +box.cfg{election_mode='candidate'} + | --- + | ... + +test_run:wait_cond(function() return #election_tbl == 3 end) + | --- + | - true + | ... +assert(election_tbl[1].state == 'follower') + | --- + | - true + | ... +assert(election_tbl[2].state == 'candidate') + | --- + | - true + | ... +assert(election_tbl[2].vote == 1) + | --- + | - true + | ... +assert(election_tbl[3].state == 'leader') + | --- + | - true + | ... + +box.cfg{election_mode='voter'} + | --- + | ... +test_run:wait_cond(function() return #election_tbl == 4 end) + | --- + | - true + | ... +assert(election_tbl[4].state == 'follower') + | --- + | - true + | ... + +box.cfg{election_mode='off'} + | --- + | ... +test_run:wait_cond(function() return #election_tbl == 5 end) + | --- + | - true + | ... + +box.cfg{election_mode='manual'} + | --- + | ... +test_run:wait_cond(function() return #election_tbl == 6 end) + | --- + | - true + | ... +assert(election_tbl[6].state == 'follower') + | --- + | - true + | ... + +box.ctl.promote() + | --- + | ... + +test_run:wait_cond(function() return #election_tbl == 9 end) + | --- + | - true + | ... +assert(election_tbl[7].state == 'follower') + | --- + | - true + | ... +assert(election_tbl[7].term == election_tbl[6].term + 1) + | --- + | - true + | ... +assert(election_tbl[8].state == 'candidate') + | --- + | - true + | ... +assert(election_tbl[9].state == 'leader') + | --- + | - true + | ... + +test_run:cmd('stop server replica') + | --- + | - true + | ... +test_run:cmd('delete server replica') + | --- + | - true + | ... + +box.schema.user.revoke('guest', 'replication') + | --- + | ... diff --git a/test/replication/election_basic.test.lua b/test/replication/election_basic.test.lua index 77fdf6340..f1330d232 100644 --- a/test/replication/election_basic.test.lua +++ b/test/replication/election_basic.test.lua @@ -116,3 +116,56 @@ assert(r1_leader == r2_leader)  test_run:cmd(string.format('start server %s', leader_name))  test_run:drop_cluster(SERVERS) + +-- gh-5819: on_election triggers, that are filed on every visible state change. +box.schema.user.grant('guest', 'replication') +test_run:cmd('create server replica with rpl_master=default,\ +          script="replication/replica.lua"') +test_run:cmd('start server replica') + +repl = test_run:eval('replica', 'return box.cfg.listen')[1] +box.cfg{replication = repl} +test_run:switch('replica') +box.cfg{election_mode='voter'} +test_run:switch('default') + +election_tbl = {} + +function trig()\ +    election_tbl[#election_tbl+1] = box.info.election\ +end + +_ = box.ctl.on_election(trig) + +box.cfg{replication_synchro_quorum=2} +box.cfg{election_mode='candidate'} + +test_run:wait_cond(function() return #election_tbl == 3 end) +assert(election_tbl[1].state == 'follower') +assert(election_tbl[2].state == 'candidate') +assert(election_tbl[2].vote == 1) +assert(election_tbl[3].state == 'leader') + +box.cfg{election_mode='voter'} +test_run:wait_cond(function() return #election_tbl == 4 end) +assert(election_tbl[4].state == 'follower') + +box.cfg{election_mode='off'} +test_run:wait_cond(function() return #election_tbl == 5 end) + +box.cfg{election_mode='manual'} +test_run:wait_cond(function() return #election_tbl == 6 end) +assert(election_tbl[6].state == 'follower') + +box.ctl.promote() + +test_run:wait_cond(function() return #election_tbl == 9 end) +assert(election_tbl[7].state == 'follower') +assert(election_tbl[7].term == election_tbl[6].term + 1) +assert(election_tbl[8].state == 'candidate') +assert(election_tbl[9].state == 'leader') + +test_run:cmd('stop server replica') +test_run:cmd('delete server replica') + +box.schema.user.revoke('guest', 'replication') diff --git a/test/replication/gh-5819-election-triggers.result b/test/replication/gh-5819-election-triggers.result deleted file mode 100644 index 546aac8a2..000000000 --- a/test/replication/gh-5819-election-triggers.result +++ /dev/null @@ -1,147 +0,0 @@ --- test-run result file version 2 -test_run = require('test_run').new() - | --- - | ... - -box.schema.user.grant('guest', 'replication') - | --- - | ... -test_run:cmd('create server replica with rpl_master=default,\ -          script="replication/replica.lua"') - | --- - | - true - | ... -test_run:cmd('start server replica') - | --- - | - true - | ... - -repl = test_run:eval('replica', 'return box.cfg.listen')[1] - | --- - | ... -box.cfg{replication = repl} - | --- - | ... -test_run:switch('replica') - | --- - | - true - | ... -box.cfg{election_mode='voter'} - | --- - | ... -test_run:switch('default') - | --- - | - true - | ... - -election_tbl = {} - | --- - | ... - -function trig()\ -    election_tbl[#election_tbl+1] = box.info.election\ -end - | --- - | ... - -_ = box.ctl.on_election(trig) - | --- - | ... - -box.cfg{replication_synchro_quorum=2} - | --- - | ... -box.cfg{election_mode='candidate'} - | --- - | ... - -test_run:wait_cond(function() return box.info.election.state == 'leader' end) - | --- - | - true - | ... -assert(#election_tbl == 3) - | --- - | - true - | ... -assert(election_tbl[1].state == 'follower') - | --- - | - true - | ... -assert(election_tbl[2].state == 'candidate') - | --- - | - true - | ... -assert(election_tbl[2].vote == 1) - | --- - | - true - | ... -assert(election_tbl[3].state == 'leader') - | --- - | - true - | ... -box.cfg{election_mode='voter'} - | --- - | ... - -assert(#election_tbl == 4) - | --- - | - true - | ... -assert(election_tbl[4].state == 'follower') - | --- - | - true - | ... - -box.cfg{election_mode='off'} - | --- - | ... -assert(#election_tbl == 5) - | --- - | - true - | ... - -box.cfg{election_mode='manual'} - | --- - | ... - -assert(#election_tbl == 6) - | --- - | - true - | ... -assert(election_tbl[6].state == 'follower') - | --- - | - true - | ... - -box.ctl.promote() - | --- - | ... - -assert(#election_tbl == 9) - | --- - | - true - | ... -assert(election_tbl[7].state == 'follower') - | --- - | - true - | ... -assert(election_tbl[8].state == 'candidate') - | --- - | - true - | ... -assert(election_tbl[9].state == 'leader') - | --- - | - true - | ... - -test_run:cmd('stop server replica') - | --- - | - true - | ... -test_run:cmd('delete server replica') - | --- - | - true - | ... -box.schema.user.revoke('guest', 'replication') - | --- - | ... diff --git a/test/replication/gh-5819-election-triggers.test.lua b/test/replication/gh-5819-election-triggers.test.lua deleted file mode 100644 index 5f61a59f9..000000000 --- a/test/replication/gh-5819-election-triggers.test.lua +++ /dev/null @@ -1,53 +0,0 @@ -test_run = require('test_run').new() - -box.schema.user.grant('guest', 'replication') -test_run:cmd('create server replica with rpl_master=default,\ -          script="replication/replica.lua"') -test_run:cmd('start server replica') - -repl = test_run:eval('replica', 'return box.cfg.listen')[1] -box.cfg{replication = repl} -test_run:switch('replica') -box.cfg{election_mode='voter'} -test_run:switch('default') - -election_tbl = {} - -function trig()\ -    election_tbl[#election_tbl+1] = box.info.election\ -end - -_ = box.ctl.on_election(trig) - -box.cfg{replication_synchro_quorum=2} -box.cfg{election_mode='candidate'} - -test_run:wait_cond(function() return box.info.election.state == 'leader' end) -assert(#election_tbl == 3) -assert(election_tbl[1].state == 'follower') -assert(election_tbl[2].state == 'candidate') -assert(election_tbl[2].vote == 1) -assert(election_tbl[3].state == 'leader') -box.cfg{election_mode='voter'} - -assert(#election_tbl == 4) -assert(election_tbl[4].state == 'follower') - -box.cfg{election_mode='off'} -assert(#election_tbl == 5) - -box.cfg{election_mode='manual'} - -assert(#election_tbl == 6) -assert(election_tbl[6].state == 'follower') - -box.ctl.promote() - -assert(#election_tbl == 9) -assert(election_tbl[7].state == 'follower') -assert(election_tbl[8].state == 'candidate') -assert(election_tbl[9].state == 'leader') - -test_run:cmd('stop server replica') -test_run:cmd('delete server replica') -box.schema.user.revoke('guest', 'replication') diff --git a/test/replication/suite.cfg b/test/replication/suite.cfg index f2af00038..a51a2d51a 100644 --- a/test/replication/suite.cfg +++ b/test/replication/suite.cfg @@ -46,7 +46,6 @@      "gh-5536-wal-limit.test.lua": {},      "gh-5566-final-join-synchro.test.lua": {},      "gh-5613-bootstrap-prefer-booted.test.lua": {}, -    "gh-5819-election-triggers.test.lua": {},      "gh-6027-applier-error-show.test.lua": {},      "gh-6032-promote-wal-write.test.lua": {},      "gh-6057-qsync-confirm-async-no-wal.test.lua": {}, =============================