From: Sergey Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>, gorcunov@gmail.com Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH] box: introduce on_election triggers Date: Wed, 28 Jul 2021 13:03:30 +0300 [thread overview] Message-ID: <81a59f0a-6703-6a18-aaaa-7ea98f4a8146@tarantool.org> (raw) In-Reply-To: <27aaa450-7c0c-1e5e-e912-db5bae282e16@tarantool.org> 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_<action>' > 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": {}, =============================
next prev parent reply other threads:[~2021-07-28 10:03 UTC|newest] Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-07-22 7:20 Serge Petrenko via Tarantool-patches 2021-07-26 20:27 ` Vladislav Shpilevoy via Tarantool-patches 2021-07-28 10:03 ` Sergey Petrenko via Tarantool-patches [this message] 2021-07-29 21:45 ` Vladislav Shpilevoy via Tarantool-patches 2021-08-06 9:49 ` Vitaliia Ioffe 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=81a59f0a-6703-6a18-aaaa-7ea98f4a8146@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=gorcunov@gmail.com \ --cc=sergepetrenko@tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH] box: introduce on_election triggers' \ /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