[Tarantool-patches] [PATCH] box: introduce on_election triggers
Sergey Petrenko
sergepetrenko at tarantool.org
Wed Jul 28 13:03:30 MSK 2021
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": {},
=============================
More information about the Tarantool-patches
mailing list