[Tarantool-patches] [PATCH v2] qsync: provide box.info interface for monitoring

[Tarantool-patches] [PATCH v2] qsync: provide box.info interface for monitoring

[Cyrill Gorcunov via Tarantool-patches]

Since commit 14fa5fd82 we support symbolic evaluation of
`replication_synchro_quorum` parameter and there is no easy
way to obtain it current run-time value, ie evaluated one.
Moreover we would like to fetch queue length on transaction
limbo for tests and extend this statistics in future.

Thus we introduce the "synchro" leaf in box.info interface.
For now only a few entries are printed out

 | tarantool> box.info.synchro
 | ---
 | - queue:
 |     1:
 |       len: 0
 |   quorum: 1
 | ...

The `queue` represents limbo instances (since we support only
one limbo for now the sole entry is printed) and `len` member
shows the number of entries in the queue.

The `quorum` member shows the evaluated value of
`replication_synchro_quorum` parameter.

Closes #5191

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>

@TarantoolBot document
Title: Provice `box.info.synchro` interface

The `box.info.synchro` leaf provides information about details of
synchronous replication.

In particular `quorum` represent the current value of synchronous
replication quorum defined by `replication_synchro_quorum`
configuration parameter since it can be set as dynamic formula
such as `N/2+1` and the value depends on current number
of replicas.

Since synchronous replication does not commit data immediately
but waits for its propagation to replicas such data sits in
a queue gathering `commit` responses from remote nodes. Current
number of entries sitting in the queue is shown by `queue.len`
member.

A typical output is the following

``` Lua
tarantool> box.info.synchro
---
- queue:
    1:
      len: 0
  quorum: 1
...
```

For now only one `queue` is supported so the output is filled with
one array entry. In future the multiple queues might be implemented.
---

v2:
 - reformat output
 - update test

issue https://github.com/tarantool/tarantool/issues/5191
branch gorcunov/gh-5191-qsync-stat-2

 src/box/lua/info.c                            | 43 +++++++++++++
 src/box/txn_limbo.c                           |  5 +-
 src/box/txn_limbo.h                           |  4 ++
 .../gh-5446-qsync-eval-quorum.result          | 64 +++++++++++++++----
 .../gh-5446-qsync-eval-quorum.test.lua        | 33 ++++++++--
 5 files changed, 129 insertions(+), 20 deletions(-)

diff --git a/src/box/lua/info.c b/src/box/lua/info.c
index 8cd379756..18b18cc90 100644
--- a/src/box/lua/info.c
+++ b/src/box/lua/info.c
@@ -50,6 +50,7 @@
 #include "version.h"
 #include "box/box.h"
 #include "box/raft.h"
+#include "box/txn_limbo.h"
 #include "lua/utils.h"
 #include "fiber.h"
 #include "sio.h"
@@ -599,6 +600,47 @@ lbox_info_election(struct lua_State *L)
 	return 1;
 }
 
+static void
+lbox_push_synchro_queue(struct lua_State *L)
+{
+	/*
+	 * For fancy formatting.
+	 */
+	lua_newtable(L);
+	lua_pushliteral(L, "mapping");
+	lua_setfield(L, -2, "__serialize");
+	lua_setmetatable(L, -2);
+
+	struct txn_limbo *queue = &txn_limbo;
+	const int queue_nr = 1;
+
+	lua_createtable(L, 0, 1);
+	lua_pushstring(L, "len");
+	lua_pushnumber(L, queue->len);
+	lua_settable(L, -3);
+	lua_rawseti(L, -2, queue_nr);
+}
+
+static int
+lbox_info_synchro(struct lua_State *L)
+{
+	lua_newtable(L);
+
+	/* Quorum value may be evaluated via formula */
+	lua_pushinteger(L, replication_synchro_quorum);
+	lua_setfield(L, -2, "quorum");
+
+	/*
+	 * Queue information.
+	 */
+	lua_newtable(L);
+	lbox_push_synchro_queue(L);
+	lua_setfield(L, -2, "queue");
+
+	return 1;
+}
+
+
 static const struct luaL_Reg lbox_info_dynamic_meta[] = {
 	{"id", lbox_info_id},
 	{"uuid", lbox_info_uuid},
@@ -618,6 +660,7 @@ static const struct luaL_Reg lbox_info_dynamic_meta[] = {
 	{"sql", lbox_info_sql},
 	{"listen", lbox_info_listen},
 	{"election", lbox_info_election},
+	{"synchro", lbox_info_synchro},
 	{NULL, NULL}
 };
 
diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
index cf0ad9350..a22e0861a 100644
--- a/src/box/txn_limbo.c
+++ b/src/box/txn_limbo.c
@@ -41,6 +41,7 @@ static inline void
 txn_limbo_create(struct txn_limbo *limbo)
 {
 	rlist_create(&limbo->queue);
+	limbo->len = 0;
 	limbo->owner_id = REPLICA_ID_NIL;
 	fiber_cond_create(&limbo->wait_cond);
 	vclock_create(&limbo->vclock);
@@ -118,6 +119,7 @@ txn_limbo_append(struct txn_limbo *limbo, uint32_t id, struct txn *txn)
 	e->is_commit = false;
 	e->is_rollback = false;
 	rlist_add_tail_entry(&limbo->queue, e, in_queue);
+	limbo->len++;
 	/*
 	 * We added new entries from a remote instance to an empty limbo.
 	 * Time to make this instance read-only.
@@ -132,8 +134,8 @@ txn_limbo_remove(struct txn_limbo *limbo, struct txn_limbo_entry *entry)
 {
 	assert(!rlist_empty(&entry->in_queue));
 	assert(txn_limbo_first_entry(limbo) == entry);
-	(void) limbo;
 	rlist_del_entry(entry, in_queue);
+	limbo->len--;
 }
 
 static inline void
@@ -144,6 +146,7 @@ txn_limbo_pop(struct txn_limbo *limbo, struct txn_limbo_entry *entry)
 	assert(entry->is_rollback);
 
 	rlist_del_entry(entry, in_queue);
+	limbo->len--;
 	++limbo->rollback_count;
 }
 
diff --git a/src/box/txn_limbo.h b/src/box/txn_limbo.h
index af0addf8d..f2a98c8bb 100644
--- a/src/box/txn_limbo.h
+++ b/src/box/txn_limbo.h
@@ -94,6 +94,10 @@ struct txn_limbo {
 	 * them LSNs in the same order.
 	 */
 	struct rlist queue;
+	/**
+	 * Number of entries in limbo queue.
+	 */
+	int64_t len;
 	/**
 	 * Instance ID of the owner of all the transactions in the
 	 * queue. Strictly speaking, nothing prevents to store not
diff --git a/test/replication/gh-5446-qsync-eval-quorum.result b/test/replication/gh-5446-qsync-eval-quorum.result
index 1d13f26db..33d64b5b1 100644
--- a/test/replication/gh-5446-qsync-eval-quorum.result
+++ b/test/replication/gh-5446-qsync-eval-quorum.result
@@ -2,6 +2,9 @@
 test_run = require('test_run').new()
  | ---
  | ...
+fiber = require('fiber')
+ | ---
+ | ...
 engine = test_run:get_cfg('engine')
  | ---
  | ...
@@ -123,6 +126,11 @@ s:insert{3} -- should pass
  | - [3]
  | ...
 
+assert(box.info.synchro.quorum == 2)
+ | ---
+ | - true
+ | ...
+
 -- 6 replicas, 7 nodes -> replication_synchro_quorum = 7/2 + 1 = 4
 test_run:cmd('create server replica2 with rpl_master=default,\
               script="replication/replica-quorum-2.lua"')
@@ -174,6 +182,11 @@ test_run:cmd('start server replica6 with wait=True, wait_load=True')
  | - true
  | ...
 
+assert(box.info.synchro.quorum == 4)
+ | ---
+ | - true
+ | ...
+
 -- All replicas are up and running
 s:insert{4} -- should pass
  | ---
@@ -254,49 +267,76 @@ s:insert{11} -- should pass
  | - [11]
  | ...
 
--- cleanup
-
+-- To test queue lentgh tracking we should enter a state
+-- where attempt to write data stucks waiting for replication
+-- to timeout. For this sake we stop all replicas and try
+-- to insert a new record. This record queued into the limbo.
+-- Note the replication timeout set to a reasonably small value
+-- just to not wait too long and still be able to detect the length
+-- change.
 test_run:cmd('stop server replica1')
  | ---
  | - true
  | ...
-test_run:cmd('delete server replica1')
+test_run:cmd('stop server replica2')
  | ---
  | - true
  | ...
-test_run:cmd('stop server replica2')
+test_run:cmd('stop server replica3')
  | ---
  | - true
  | ...
-test_run:cmd('delete server replica2')
+test_run:cmd('stop server replica4')
  | ---
  | - true
  | ...
-test_run:cmd('stop server replica3')
+test_run:cmd('stop server replica5')
  | ---
  | - true
  | ...
-test_run:cmd('delete server replica3')
+test_run:cmd('stop server replica6')
  | ---
  | - true
  | ...
-test_run:cmd('stop server replica4')
+
+assert(box.info.synchro.queue[1].len == 0)
  | ---
  | - true
  | ...
-test_run:cmd('delete server replica4')
+box.cfg{replication_synchro_timeout = 2}
+ | ---
+ | ...
+f = fiber.new(function() s:insert{12} end)
+ | ---
+ | ...
+test_run:wait_cond(function() return box.info.synchro.queue[1].len == 1 end)
  | ---
  | - true
  | ...
-test_run:cmd('stop server replica5')
+test_run:wait_cond(function() return box.info.synchro.queue[1].len == 0 end)
  | ---
  | - true
  | ...
-test_run:cmd('delete server replica5')
+
+-- Cleanup
+
+test_run:cmd('delete server replica1')
  | ---
  | - true
  | ...
-test_run:cmd('stop server replica6')
+test_run:cmd('delete server replica2')
+ | ---
+ | - true
+ | ...
+test_run:cmd('delete server replica3')
+ | ---
+ | - true
+ | ...
+test_run:cmd('delete server replica4')
+ | ---
+ | - true
+ | ...
+test_run:cmd('delete server replica5')
  | ---
  | - true
  | ...
diff --git a/test/replication/gh-5446-qsync-eval-quorum.test.lua b/test/replication/gh-5446-qsync-eval-quorum.test.lua
index 62d87ddcb..929571798 100644
--- a/test/replication/gh-5446-qsync-eval-quorum.test.lua
+++ b/test/replication/gh-5446-qsync-eval-quorum.test.lua
@@ -1,4 +1,5 @@
 test_run = require('test_run').new()
+fiber = require('fiber')
 engine = test_run:get_cfg('engine')
 
 box.schema.user.grant('guest', 'replication')
@@ -46,6 +47,8 @@ cfg_set_pass_tmo()
 test_run:cmd('start server replica1 with wait=True, wait_load=True')
 s:insert{3} -- should pass
 
+assert(box.info.synchro.quorum == 2)
+
 -- 6 replicas, 7 nodes -> replication_synchro_quorum = 7/2 + 1 = 4
 test_run:cmd('create server replica2 with rpl_master=default,\
               script="replication/replica-quorum-2.lua"')
@@ -67,6 +70,8 @@ test_run:cmd('create server replica6 with rpl_master=default,\
               script="replication/replica-quorum-6.lua"')
 test_run:cmd('start server replica6 with wait=True, wait_load=True')
 
+assert(box.info.synchro.quorum == 4)
+
 -- All replicas are up and running
 s:insert{4} -- should pass
 
@@ -92,19 +97,33 @@ s:insert{10} -- should pass
 test_run:cmd('start server replica1 with wait=True, wait_load=True')
 s:insert{11} -- should pass
 
--- cleanup
-
+-- To test queue lentgh tracking we should enter a state
+-- where attempt to write data stucks waiting for replication
+-- to timeout. For this sake we stop all replicas and try
+-- to insert a new record. This record queued into the limbo.
+-- Note the replication timeout set to a reasonably small value
+-- just to not wait too long and still be able to detect the length
+-- change.
 test_run:cmd('stop server replica1')
-test_run:cmd('delete server replica1')
 test_run:cmd('stop server replica2')
-test_run:cmd('delete server replica2')
 test_run:cmd('stop server replica3')
-test_run:cmd('delete server replica3')
 test_run:cmd('stop server replica4')
-test_run:cmd('delete server replica4')
 test_run:cmd('stop server replica5')
-test_run:cmd('delete server replica5')
 test_run:cmd('stop server replica6')
+
+assert(box.info.synchro.queue[1].len == 0)
+box.cfg{replication_synchro_timeout = 2}
+f = fiber.new(function() s:insert{12} end)
+test_run:wait_cond(function() return box.info.synchro.queue[1].len == 1 end)
+test_run:wait_cond(function() return box.info.synchro.queue[1].len == 0 end)
+
+-- Cleanup
+
+test_run:cmd('delete server replica1')
+test_run:cmd('delete server replica2')
+test_run:cmd('delete server replica3')
+test_run:cmd('delete server replica4')
+test_run:cmd('delete server replica5')
 test_run:cmd('delete server replica6')
 
 s:drop()
-- 
2.30.2

Re: [Tarantool-patches] [PATCH v3] qsync: provide box.info interface for monitoring

[Cyrill Gorcunov via Tarantool-patches]

Guys, here is an updated version. Take a look please.
Vlad, lets stick with simple version and will add
`queues` array if we ever really need to.

issue https://github.com/tarantool/tarantool/issues/5191
branch gorcunov/gh-5191-qsync-stat-3
---
Since commit 14fa5fd82 we support symbolic evaluation of
`replication_synchro_quorum` parameter and there is no easy
way to obtain it current run-time value, ie evaluated one.
Moreover we would like to fetch queue length on transaction
limbo for tests and extend this statistics in future.

Thus we introduce the "synchro" leaf in box.info interface.
For now only a few entries are printed out

 | tarantool> box.info.synchro
 | ---
 | - queue:
 |     len: 0
 |   quorum: 1
 | ...

The `queue` represents limbo instance and `len` member
shows the number of entries in the queue. Note that if
we gonna support multiple queues then we might create
a separate `queues` array to cover all possible instances.

The `quorum` member shows the evaluated value of
`replication_synchro_quorum` parameter.

Closes #5191

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>

@TarantoolBot document
Title: Provide `box.info.synchro` interface

The `box.info.synchro` leaf provides information about details of
synchronous replication.

In particular `quorum` represent the current value of synchronous
replication quorum defined by `replication_synchro_quorum`
configuration parameter since it can be set as dynamic formula
such as `N/2+1` and the value depends on current number
of replicas.

Since synchronous replication does not commit data immediately
but waits for its propagation to replicas such data sits in
a queue gathering `commit` responses from remote nodes. Current
number of entries sitting in the queue is shown by `queue.len`
member.

A typical output is the following

``` Lua
tarantool> box.info.synchro
---
 changelogs/unreleased/box-info-limbo.md       |  4 ++
 src/box/lua/info.c                            | 25 ++++++++
 src/box/txn_limbo.c                           |  5 +-
 src/box/txn_limbo.h                           |  4 ++
 test/box/info.result                          |  1 +
 .../gh-5446-qsync-eval-quorum.result          | 64 +++++++++++++++----
 .../gh-5446-qsync-eval-quorum.test.lua        | 33 ++++++++--
 7 files changed, 116 insertions(+), 20 deletions(-)
 create mode 100644 changelogs/unreleased/box-info-limbo.md

diff --git a/changelogs/unreleased/box-info-limbo.md b/changelogs/unreleased/box-info-limbo.md
new file mode 100644
index 000000000..0f75a911d
--- /dev/null
+++ b/changelogs/unreleased/box-info-limbo.md
@@ -0,0 +1,4 @@
+## feature/core
+
+* Provide information about state of synchronous replication via
+  `box.info.synchro` interface (gh-5191).
diff --git a/src/box/lua/info.c b/src/box/lua/info.c
index 8cd379756..a2ffff4b5 100644
--- a/src/box/lua/info.c
+++ b/src/box/lua/info.c
@@ -50,6 +50,7 @@
 #include "version.h"
 #include "box/box.h"
 #include "box/raft.h"
+#include "box/txn_limbo.h"
 #include "lua/utils.h"
 #include "fiber.h"
 #include "sio.h"
@@ -599,6 +600,29 @@ lbox_info_election(struct lua_State *L)
 	return 1;
 }
 
+static int
+lbox_info_synchro(struct lua_State *L)
+{
+	lua_newtable(L);
+
+	/* Quorum value may be evaluated via formula */
+	lua_pushinteger(L, replication_synchro_quorum);
+	lua_setfield(L, -2, "quorum");
+
+	/*
+	 * Queue information.
+	 */
+	struct txn_limbo *queue = &txn_limbo;
+	lua_createtable(L, 0, 1);
+	lua_pushstring(L, "len");
+	lua_pushnumber(L, queue->len);
+	lua_settable(L, -3);
+	lua_setfield(L, -2, "queue");
+
+	return 1;
+}
+
+
 static const struct luaL_Reg lbox_info_dynamic_meta[] = {
 	{"id", lbox_info_id},
 	{"uuid", lbox_info_uuid},
@@ -618,6 +642,7 @@ static const struct luaL_Reg lbox_info_dynamic_meta[] = {
 	{"sql", lbox_info_sql},
 	{"listen", lbox_info_listen},
 	{"election", lbox_info_election},
+	{"synchro", lbox_info_synchro},
 	{NULL, NULL}
 };
 
diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
index cf0ad9350..a22e0861a 100644
--- a/src/box/txn_limbo.c
+++ b/src/box/txn_limbo.c
@@ -41,6 +41,7 @@ static inline void
 txn_limbo_create(struct txn_limbo *limbo)
 {
 	rlist_create(&limbo->queue);
+	limbo->len = 0;
 	limbo->owner_id = REPLICA_ID_NIL;
 	fiber_cond_create(&limbo->wait_cond);
 	vclock_create(&limbo->vclock);
@@ -118,6 +119,7 @@ txn_limbo_append(struct txn_limbo *limbo, uint32_t id, struct txn *txn)
 	e->is_commit = false;
 	e->is_rollback = false;
 	rlist_add_tail_entry(&limbo->queue, e, in_queue);
+	limbo->len++;
 	/*
 	 * We added new entries from a remote instance to an empty limbo.
 	 * Time to make this instance read-only.
@@ -132,8 +134,8 @@ txn_limbo_remove(struct txn_limbo *limbo, struct txn_limbo_entry *entry)
 {
 	assert(!rlist_empty(&entry->in_queue));
 	assert(txn_limbo_first_entry(limbo) == entry);
-	(void) limbo;
 	rlist_del_entry(entry, in_queue);
+	limbo->len--;
 }
 
 static inline void
@@ -144,6 +146,7 @@ txn_limbo_pop(struct txn_limbo *limbo, struct txn_limbo_entry *entry)
 	assert(entry->is_rollback);
 
 	rlist_del_entry(entry, in_queue);
+	limbo->len--;
 	++limbo->rollback_count;
 }
 
diff --git a/src/box/txn_limbo.h b/src/box/txn_limbo.h
index af0addf8d..f2a98c8bb 100644
--- a/src/box/txn_limbo.h
+++ b/src/box/txn_limbo.h
@@ -94,6 +94,10 @@ struct txn_limbo {
 	 * them LSNs in the same order.
 	 */
 	struct rlist queue;
+	/**
+	 * Number of entries in limbo queue.
+	 */
+	int64_t len;
 	/**
 	 * Instance ID of the owner of all the transactions in the
 	 * queue. Strictly speaking, nothing prevents to store not
diff --git a/test/box/info.result b/test/box/info.result
index c8037818b..3ee653773 100644
--- a/test/box/info.result
+++ b/test/box/info.result
@@ -89,6 +89,7 @@ t
   - signature
   - sql
   - status
+  - synchro
   - uptime
   - uuid
   - vclock
diff --git a/test/replication/gh-5446-qsync-eval-quorum.result b/test/replication/gh-5446-qsync-eval-quorum.result
index 1d13f26db..ff2660a72 100644
--- a/test/replication/gh-5446-qsync-eval-quorum.result
+++ b/test/replication/gh-5446-qsync-eval-quorum.result
@@ -2,6 +2,9 @@
 test_run = require('test_run').new()
  | ---
  | ...
+fiber = require('fiber')
+ | ---
+ | ...
 engine = test_run:get_cfg('engine')
  | ---
  | ...
@@ -123,6 +126,11 @@ s:insert{3} -- should pass
  | - [3]
  | ...
 
+assert(box.info.synchro.quorum == 2)
+ | ---
+ | - true
+ | ...
+
 -- 6 replicas, 7 nodes -> replication_synchro_quorum = 7/2 + 1 = 4
 test_run:cmd('create server replica2 with rpl_master=default,\
               script="replication/replica-quorum-2.lua"')
@@ -174,6 +182,11 @@ test_run:cmd('start server replica6 with wait=True, wait_load=True')
  | - true
  | ...
 
+assert(box.info.synchro.quorum == 4)
+ | ---
+ | - true
+ | ...
+
 -- All replicas are up and running
 s:insert{4} -- should pass
  | ---
@@ -254,49 +267,76 @@ s:insert{11} -- should pass
  | - [11]
  | ...
 
--- cleanup
-
+-- To test queue length tracking we should enter a state
+-- where attempt to write data stuck waiting for replication
+-- to timeout. For this sake we stop all replicas and try
+-- to insert a new record. This record queued into the limbo.
+-- Note the replication timeout set to a reasonably small value
+-- just to not wait too long and still be able to detect the length
+-- change.
 test_run:cmd('stop server replica1')
  | ---
  | - true
  | ...
-test_run:cmd('delete server replica1')
+test_run:cmd('stop server replica2')
  | ---
  | - true
  | ...
-test_run:cmd('stop server replica2')
+test_run:cmd('stop server replica3')
  | ---
  | - true
  | ...
-test_run:cmd('delete server replica2')
+test_run:cmd('stop server replica4')
  | ---
  | - true
  | ...
-test_run:cmd('stop server replica3')
+test_run:cmd('stop server replica5')
  | ---
  | - true
  | ...
-test_run:cmd('delete server replica3')
+test_run:cmd('stop server replica6')
  | ---
  | - true
  | ...
-test_run:cmd('stop server replica4')
+
+assert(box.info.synchro.queue.len == 0)
  | ---
  | - true
  | ...
-test_run:cmd('delete server replica4')
+box.cfg{replication_synchro_timeout = 2}
+ | ---
+ | ...
+f = fiber.new(function() s:insert{12} end)
+ | ---
+ | ...
+test_run:wait_cond(function() return box.info.synchro.queue.len == 1 end)
  | ---
  | - true
  | ...
-test_run:cmd('stop server replica5')
+test_run:wait_cond(function() return box.info.synchro.queue.len == 0 end)
  | ---
  | - true
  | ...
-test_run:cmd('delete server replica5')
+
+-- Cleanup
+
+test_run:cmd('delete server replica1')
  | ---
  | - true
  | ...
-test_run:cmd('stop server replica6')
+test_run:cmd('delete server replica2')
+ | ---
+ | - true
+ | ...
+test_run:cmd('delete server replica3')
+ | ---
+ | - true
+ | ...
+test_run:cmd('delete server replica4')
+ | ---
+ | - true
+ | ...
+test_run:cmd('delete server replica5')
  | ---
  | - true
  | ...
diff --git a/test/replication/gh-5446-qsync-eval-quorum.test.lua b/test/replication/gh-5446-qsync-eval-quorum.test.lua
index 62d87ddcb..b1fb3c072 100644
--- a/test/replication/gh-5446-qsync-eval-quorum.test.lua
+++ b/test/replication/gh-5446-qsync-eval-quorum.test.lua
@@ -1,4 +1,5 @@
 test_run = require('test_run').new()
+fiber = require('fiber')
 engine = test_run:get_cfg('engine')
 
 box.schema.user.grant('guest', 'replication')
@@ -46,6 +47,8 @@ cfg_set_pass_tmo()
 test_run:cmd('start server replica1 with wait=True, wait_load=True')
 s:insert{3} -- should pass
 
+assert(box.info.synchro.quorum == 2)
+
 -- 6 replicas, 7 nodes -> replication_synchro_quorum = 7/2 + 1 = 4
 test_run:cmd('create server replica2 with rpl_master=default,\
               script="replication/replica-quorum-2.lua"')
@@ -67,6 +70,8 @@ test_run:cmd('create server replica6 with rpl_master=default,\
               script="replication/replica-quorum-6.lua"')
 test_run:cmd('start server replica6 with wait=True, wait_load=True')
 
+assert(box.info.synchro.quorum == 4)
+
 -- All replicas are up and running
 s:insert{4} -- should pass
 
@@ -92,19 +97,33 @@ s:insert{10} -- should pass
 test_run:cmd('start server replica1 with wait=True, wait_load=True')
 s:insert{11} -- should pass
 
--- cleanup
-
+-- To test queue length tracking we should enter a state
+-- where attempt to write data stuck waiting for replication
+-- to timeout. For this sake we stop all replicas and try
+-- to insert a new record. This record queued into the limbo.
+-- Note the replication timeout set to a reasonably small value
+-- just to not wait too long and still be able to detect the length
+-- change.
 test_run:cmd('stop server replica1')
-test_run:cmd('delete server replica1')
 test_run:cmd('stop server replica2')
-test_run:cmd('delete server replica2')
 test_run:cmd('stop server replica3')
-test_run:cmd('delete server replica3')
 test_run:cmd('stop server replica4')
-test_run:cmd('delete server replica4')
 test_run:cmd('stop server replica5')
-test_run:cmd('delete server replica5')
 test_run:cmd('stop server replica6')
+
+assert(box.info.synchro.queue.len == 0)
+box.cfg{replication_synchro_timeout = 2}
+f = fiber.new(function() s:insert{12} end)
+test_run:wait_cond(function() return box.info.synchro.queue.len == 1 end)
+test_run:wait_cond(function() return box.info.synchro.queue.len == 0 end)
+
+-- Cleanup
+
+test_run:cmd('delete server replica1')
+test_run:cmd('delete server replica2')
+test_run:cmd('delete server replica3')
+test_run:cmd('delete server replica4')
+test_run:cmd('delete server replica5')
 test_run:cmd('delete server replica6')
 
 s:drop()
-- 
2.30.2

Re: [Tarantool-patches] [PATCH v2] qsync: provide box.info interface for monitoring

[Vladislav Shpilevoy via Tarantool-patches]

Hi! Thanks for the patch!

See 7 comments below.

>     qsync: provide box.info interface for monitoring
>     
>     Since commit 14fa5fd82 we support symbolic evaluation of

1. Please, after a commit hash provide the referenced commit
title in parentheses and quotes.
https://github.com/tarantool/tarantool/wiki/Code-review-procedure#commit-message

>     `replication_synchro_quorum` parameter and there is no easy
>     way to obtain it current run-time value, ie evaluated one.
>     Moreover we would like to fetch queue length on transaction
>     limbo for tests and extend this statistics in future.
>     
>     Thus we introduce the "synchro" leaf in box.info interface.
>     For now only a few entries are printed out
>     
>      | tarantool> box.info.synchro
>      | ---
>      | - queue:
>      |     1:
>      |       len: 0
>      |   quorum: 1
>      | ...

2. AFAIK, it was decided not to return an array, and return only
the currently used queue. But even if it would be an array, as I
said in the previous review, the indexing must be by box.info.id,
not by any order. Assume there are multiple limbos, and the master is
not instance_id = 1. How is a user supposed to find what is the
queue in the currently used limbo? And what is the currently used
limbo?

Besides, the commit message and the docbot request are almost the
same. You could drop the message and leave only the docbot request
if you don't want to support both places. Also it would save time
on reading that, anyway the information is the same.

>     The `queue` represents limbo instances (since we support only
>     one limbo for now the sole entry is printed) and `len` member
>     shows the number of entries in the queue.
>     
>     The `quorum` member shows the evaluated value of
>     `replication_synchro_quorum` parameter.
>     
>     Closes #5191
>     
>     Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
>     
>     @TarantoolBot document
>     Title: Provice `box.info.synchro` interface

3. Provice -> Provide.

>     The `box.info.synchro` leaf provides information about details of
>     synchronous replication.
>     
>     In particular `quorum` represent the current value of synchronous
>     replication quorum defined by `replication_synchro_quorum`
>     configuration parameter since it can be set as dynamic formula
>     such as `N/2+1` and the value depends on current number
>     of replicas.
>     
>     Since synchronous replication does not commit data immediately
>     but waits for its propagation to replicas such data sits in
>     a queue gathering `commit` responses from remote nodes. Current
>     number of entries sitting in the queue is shown by `queue.len`
>     member.
>     
>     A typical output is the following
>     
>     ``` Lua
>     tarantool> box.info.synchro
>     ---
>     - queue:
>         1:
>           len: 0
>       quorum: 1
>     ...
>     ```
>     
>     For now only one `queue` is supported so the output is filled with
>     one array entry. In future the multiple queues might be implemented.
> 
> diff --git a/src/box/lua/info.c b/src/box/lua/info.c
> index 8cd379756..18b18cc90 100644
> --- a/src/box/lua/info.c
> +++ b/src/box/lua/info.c
> @@ -599,6 +600,47 @@ lbox_info_election(struct lua_State *L)
>  	return 1;
>  }
>  
> +static void
> +lbox_push_synchro_queue(struct lua_State *L)
> +{
> +	/*
> +	 * For fancy formatting.
> +	 */
> +	lua_newtable(L);
> +	lua_pushliteral(L, "mapping");
> +	lua_setfield(L, -2, "__serialize");
> +	lua_setmetatable(L, -2);
> +
> +	struct txn_limbo *queue = &txn_limbo;
> +	const int queue_nr = 1;
> +
> +	lua_createtable(L, 0, 1);
> +	lua_pushstring(L, "len");
> +	lua_pushnumber(L, queue->len);
> +	lua_settable(L, -3);

4. There is a function setfield(), which you
can use instead of pushstring() + settable(). You even
did use it above and below, but somewhy not here.

> +	lua_rawseti(L, -2, queue_nr);
> +}
> diff --git a/test/replication/gh-5446-qsync-eval-quorum.result b/test/replication/gh-5446-qsync-eval-quorum.result
> index 1d13f26db..33d64b5b1 100644
> --- a/test/replication/gh-5446-qsync-eval-quorum.result
> +++ b/test/replication/gh-5446-qsync-eval-quorum.result
> @@ -2,6 +2,9 @@
>  test_run = require('test_run').new()
>   | ---
>   | ...
> +fiber = require('fiber')
> + | ---
> + | ...
>  engine = test_run:get_cfg('engine')
>   | ---
>   | ...
> @@ -123,6 +126,11 @@ s:insert{3} -- should pass
>   | - [3]
>   | ...
>  
> +assert(box.info.synchro.quorum == 2)
> + | ---
> + | - true
> + | ...
> +
>  -- 6 replicas, 7 nodes -> replication_synchro_quorum = 7/2 + 1 = 4
>  test_run:cmd('create server replica2 with rpl_master=default,\
>                script="replication/replica-quorum-2.lua"')
> @@ -174,6 +182,11 @@ test_run:cmd('start server replica6 with wait=True, wait_load=True')
>   | - true
>   | ...
>  
> +assert(box.info.synchro.quorum == 4)
> + | ---
> + | - true
> + | ...
> +
>  -- All replicas are up and running
>  s:insert{4} -- should pass
>   | ---
> @@ -254,49 +267,76 @@ s:insert{11} -- should pass
>   | - [11]
>   | ...
>  
> --- cleanup
> -
> +-- To test queue lentgh tracking we should enter a state

5. lentgh -> length.

> +-- where attempt to write data stucks waiting for replication

6. stuck is past from stick, it does not conjugate. But I also
often forget that, so up to you. Still understandable.

But a more important comment - please, don't alter the old tests
just to get them cover your new feature whenever it feels like it.
The test is called "gh-5446-qsync-eval-quorum", therefore it must
be about quorum. Not about the queue length.

Testing box.info.synchro.quorum is fine here, it is related. But
not the queue length.

For testing the length you can add some new cases to qsync_basic.test.lua,
or even qsync_advanced.test.lua, if you don't want to create a
new file.

7. box/info.test.lua fails on your branch.

Re: [Tarantool-patches] [PATCH v2] qsync: provide box.info interface for monitoring

[Cyrill Gorcunov via Tarantool-patches]

On Wed, Apr 07, 2021 at 10:53:27PM +0200, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> See 7 comments below.
> 
> >     qsync: provide box.info interface for monitoring
> >     
> >     Since commit 14fa5fd82 we support symbolic evaluation of
> 
> 1. Please, after a commit hash provide the referenced commit
> title in parentheses and quotes.
> https://github.com/tarantool/tarantool/wiki/Code-review-procedure#commit-message

Maybe worth to add an example then? Something like

[cyrill@grain tarantool.git] git log --pretty=reference -1 14fa5fd82
14fa5fd82 (cfg: support symbolic evaluation of replication_synchro_quorum, 2020-12-03)

> >     Thus we introduce the "synchro" leaf in box.info interface.
> >     For now only a few entries are printed out
> >     
> >      | tarantool> box.info.synchro
> >      | ---
> >      | - queue:
> >      |     1:
> >      |       len: 0
> >      |   quorum: 1
> >      | ...
> 
> 2. AFAIK, it was decided not to return an array, and return only
> the currently used queue. But even if it would be an array, as I
> said in the previous review, the indexing must be by box.info.id,
> not by any order. Assume there are multiple limbos, and the master is
> not instance_id = 1. How is a user supposed to find what is the
> queue in the currently used limbo? And what is the currently used
> limbo?

Wait, Vlad, this is v2, while I sent v3 already where tried to address
the formatting

https://lists.tarantool.org/tarantool-patches/YGt0TACA8xe6N7Rr@grain/

In short I use

Thus we introduce the "synchro" leaf in box.info interface.
For now only a few entries are printed out

 | tarantool> box.info.synchro
 | ---
 | - queue:
 |     len: 0
 |   quorum: 1
 | ...

The `queue` represents limbo instance and `len` member
shows the number of entries in the queue. Note that if
we gonna support multiple queues then we might create
a separate `queues` array to cover all possible instances.

The `quorum` member shows the evaluated value of
`replication_synchro_quorum` parameter.

Your comments are still relevant and I'll address them
in v4 but please make sure the output above is exactly
what you prefer?

> 
> Besides, the commit message and the docbot request are almost the
> same. You could drop the message and leave only the docbot request
> if you don't want to support both places. Also it would save time
> on reading that, anyway the information is the same.

I'll address all the rest of comments. Thanks for review!

	Cyrill

Re: [Tarantool-patches] [PATCH v2] qsync: provide box.info interface for monitoring

[Vladislav Shpilevoy via Tarantool-patches]

On 07.04.2021 23:15, Cyrill Gorcunov wrote:
> On Wed, Apr 07, 2021 at 10:53:27PM +0200, Vladislav Shpilevoy wrote:
>> Hi! Thanks for the patch!
>>
>> See 7 comments below.
>>
>>>     qsync: provide box.info interface for monitoring
>>>     
>>>     Since commit 14fa5fd82 we support symbolic evaluation of
>>
>> 1. Please, after a commit hash provide the referenced commit
>> title in parentheses and quotes.
>> https://github.com/tarantool/tarantool/wiki/Code-review-procedure#commit-message
> 
> Maybe worth to add an example then? Something like

There is an example, see the link above.

> [cyrill@grain tarantool.git] git log --pretty=reference -1 14fa5fd82
> 14fa5fd82 (cfg: support symbolic evaluation of replication_synchro_quorum, 2020-12-03)
> 
>>>     Thus we introduce the "synchro" leaf in box.info interface.
>>>     For now only a few entries are printed out
>>>     
>>>      | tarantool> box.info.synchro
>>>      | ---
>>>      | - queue:
>>>      |     1:
>>>      |       len: 0
>>>      |   quorum: 1
>>>      | ...
>>
>> 2. AFAIK, it was decided not to return an array, and return only
>> the currently used queue. But even if it would be an array, as I
>> said in the previous review, the indexing must be by box.info.id,
>> not by any order. Assume there are multiple limbos, and the master is
>> not instance_id = 1. How is a user supposed to find what is the
>> queue in the currently used limbo? And what is the currently used
>> limbo?
> 
> Wait, Vlad, this is v2, while I sent v3 already where tried to address
> the formatting
> 
> https://lists.tarantool.org/tarantool-patches/YGt0TACA8xe6N7Rr@grain/
> 
> In short I use
> 
> Thus we introduce the "synchro" leaf in box.info interface.
> For now only a few entries are printed out
> 
>  | tarantool> box.info.synchro
>  | ---
>  | - queue:
>  |     len: 0
>  |   quorum: 1
>  | ...
> 
> The `queue` represents limbo instance and `len` member
> shows the number of entries in the queue. Note that if
> we gonna support multiple queues then we might create
> a separate `queues` array to cover all possible instances.
> 
> The `quorum` member shows the evaluated value of
> `replication_synchro_quorum` parameter.
> 
> Your comments are still relevant and I'll address them
> in v4 but please make sure the output above is exactly
> what you prefer?

The output looks good. Sorry, I missed v3 version. Saw it in
the same thread, but thought it is just some follow-up for v2
and didn't read carefully enough, or maybe marked as read
accidentally, I don't remember. Anyway, waiting for a new
version now.

Re: [Tarantool-patches] [PATCH v2] qsync: provide box.info interface for monitoring

[Cyrill Gorcunov via Tarantool-patches]

On Thu, Apr 08, 2021 at 12:52:48AM +0200, Vladislav Shpilevoy wrote:
> On 07.04.2021 23:15, Cyrill Gorcunov wrote:
> > On Wed, Apr 07, 2021 at 10:53:27PM +0200, Vladislav Shpilevoy wrote:
> >> Hi! Thanks for the patch!
> >>
> >> See 7 comments below.
> >>
> >>>     qsync: provide box.info interface for monitoring
> >>>     
> >>>     Since commit 14fa5fd82 we support symbolic evaluation of
> >>
> >> 1. Please, after a commit hash provide the referenced commit
> >> title in parentheses and quotes.
> >> https://github.com/tarantool/tarantool/wiki/Code-review-procedure#commit-message
> > 
> > Maybe worth to add an example then? Something like
> 
> There is an example, see the link above.

There is a result example but no any hints on how to generate it. I added to the page
an example of use `git log --pretty=format ...`.

Re: [Tarantool-patches] [PATCH v3] qsync: provide box.info interface for monitoring

[Serge Petrenko via Tarantool-patches]

05.04.2021 23:34, Cyrill Gorcunov пишет:
> Guys, here is an updated version. Take a look please.
> Vlad, lets stick with simple version and will add
> `queues` array if we ever really need to.
Hi! Thanks for the patch!

Please find one comment below.

>
> issue https://github.com/tarantool/tarantool/issues/5191
> branch gorcunov/gh-5191-qsync-stat-3
> ---
>
> diff --git a/src/box/lua/info.c b/src/box/lua/info.c
> index 8cd379756..a2ffff4b5 100644
> --- a/src/box/lua/info.c
> +++ b/src/box/lua/info.c
> @@ -50,6 +50,7 @@
>   #include "version.h"
>   #include "box/box.h"
>   #include "box/raft.h"
> +#include "box/txn_limbo.h"
>   #include "lua/utils.h"
>   #include "fiber.h"
>   #include "sio.h"
> @@ -599,6 +600,29 @@ lbox_info_election(struct lua_State *L)
>   	return 1;
>   }
>   
> +static int
> +lbox_info_synchro(struct lua_State *L)
> +{
> +	lua_newtable(L);

Maybe change to lua_createtable(L, 0, 2) ?
This will be consistent with what is done below, and lua doc
states that'll be a bit faster:
https://www.lua.org/manual/5.3/manual.html#lua_newtable
(sorry, a direct link to lua_createtable didn't work)

> +
> +	/* Quorum value may be evaluated via formula */
> +	lua_pushinteger(L, replication_synchro_quorum);
> +	lua_setfield(L, -2, "quorum");
> +
> +	/*
> +	 * Queue information.
> +	 */
> +	struct txn_limbo *queue = &txn_limbo;
> +	lua_createtable(L, 0, 1);
> +	lua_pushstring(L, "len");
> +	lua_pushnumber(L, queue->len);
> +	lua_settable(L, -3);
> +	lua_setfield(L, -2, "queue");
> +
> +	return 1;
> +}
> +
> +
>   static const struct luaL_Reg lbox_info_dynamic_meta[] = {
>   	{"id", lbox_info_id},
>   	{"uuid", lbox_info_uuid},
> @@ -618,6 +642,7 @@ static const struct luaL_Reg lbox_info_dynamic_meta[] = {
>   	{"sql", lbox_info_sql},
>   	{"listen", lbox_info_listen},
>   	{"election", lbox_info_election},
> +	{"synchro", lbox_info_synchro},
>   	{NULL, NULL}
>   };
>
-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH v3] qsync: provide box.info interface for monitoring

[Cyrill Gorcunov via Tarantool-patches]

On Thu, Apr 08, 2021 at 10:34:56AM +0300, Serge Petrenko wrote:
> > +static int
> > +lbox_info_synchro(struct lua_State *L)
> > +{
> > +	lua_newtable(L);
> 
> Maybe change to lua_createtable(L, 0, 2) ?
> This will be consistent with what is done below, and lua doc
> states that'll be a bit faster:
> https://www.lua.org/manual/5.3/manual.html#lua_newtable
> (sorry, a direct link to lua_createtable didn't work)

Sure! Thanks!

Re: [Tarantool-patches] [PATCH v2] qsync: provide box.info interface for monitoring

[Vladislav Shpilevoy via Tarantool-patches]

On 08.04.2021 09:11, Cyrill Gorcunov wrote:
> On Thu, Apr 08, 2021 at 12:52:48AM +0200, Vladislav Shpilevoy wrote:
>> On 07.04.2021 23:15, Cyrill Gorcunov wrote:
>>> On Wed, Apr 07, 2021 at 10:53:27PM +0200, Vladislav Shpilevoy wrote:
>>>> Hi! Thanks for the patch!
>>>>
>>>> See 7 comments below.
>>>>
>>>>>     qsync: provide box.info interface for monitoring
>>>>>     
>>>>>     Since commit 14fa5fd82 we support symbolic evaluation of
>>>>
>>>> 1. Please, after a commit hash provide the referenced commit
>>>> title in parentheses and quotes.
>>>> https://github.com/tarantool/tarantool/wiki/Code-review-procedure#commit-message
>>>
>>> Maybe worth to add an example then? Something like
>>
>> There is an example, see the link above.
> 
> There is a result example but no any hints on how to generate it. I added to the page
> an example of use `git log --pretty=format ...`.

You just do 'git show' on your hash and copy-paste the message.
I don't see anything hard in it.

Re: [Tarantool-patches] [PATCH v2] qsync: provide box.info interface for monitoring

[Cyrill Gorcunov via Tarantool-patches]

On Thu, Apr 08, 2021 at 09:37:54PM +0200, Vladislav Shpilevoy wrote:
> > 
> > There is a result example but no any hints on how to generate it. I added to the page
> > an example of use `git log --pretty=format ...`.
> 
> You just do 'git show' on your hash and copy-paste the message.
> I don't see anything hard in it.

git show generates the whole commit including its body.
It is simply not needed for oneline requirement.

Re: [Tarantool-patches] [PATCH v2] qsync: provide box.info interface for monitoring

[Vladislav Shpilevoy via Tarantool-patches]

On 08.04.2021 21:54, Cyrill Gorcunov wrote:
> On Thu, Apr 08, 2021 at 09:37:54PM +0200, Vladislav Shpilevoy wrote:
>>>
>>> There is a result example but no any hints on how to generate it. I added to the page
>>> an example of use `git log --pretty=format ...`.
>>
>> You just do 'git show' on your hash and copy-paste the message.
>> I don't see anything hard in it.
> 
> git show generates the whole commit including its body.
> It is simply not needed for oneline requirement.

But you are a human, right? So you can just copy-paste the
first line. It is not super hard. You just need to select it,
do 'Ctrl + C', and then 'Ctrl + V' in your new commit message.
Easy!

If you consider it somehow not acceptable or extremely longer
than your way, you can create a script/alias doing it for you.
You just need to omit the date, and still add line breaks if
the line in your new commit message becomes too long.

Re: [Tarantool-patches] [PATCH v2] qsync: provide box.info interface for monitoring

[Cyrill Gorcunov via Tarantool-patches]

On Thu, Apr 08, 2021 at 09:57:17PM +0200, Vladislav Shpilevoy wrote:
> > 
> > git show generates the whole commit including its body.
> > It is simply not needed for oneline requirement.
> 
> But you are a human, right? So you can just copy-paste the
> first line. It is not super hard. You just need to select it,
> do 'Ctrl + C', and then 'Ctrl + V' in your new commit message.
> Easy!
> 
> If you consider it somehow not acceptable or extremely longer
> than your way, you can create a script/alias doing it for you.
> You just need to omit the date, and still add line breaks if
> the line in your new commit message becomes too long.

Look, git already has the helper which does exactly what you
prefer to see in output. I simply added this command to the
our code-review page. If you don't like this example with
`git log --pretty=format:"%h (%s)"` feel free to remove it
from the page.

	Cyrill

Re: [Tarantool-patches] [PATCH v2] qsync: provide box.info interface for monitoring

[Vladislav Shpilevoy via Tarantool-patches]

On 08.04.2021 22:04, Cyrill Gorcunov via Tarantool-patches wrote:
> On Thu, Apr 08, 2021 at 09:57:17PM +0200, Vladislav Shpilevoy wrote:
>>>
>>> git show generates the whole commit including its body.
>>> It is simply not needed for oneline requirement.
>>
>> But you are a human, right? So you can just copy-paste the
>> first line. It is not super hard. You just need to select it,
>> do 'Ctrl + C', and then 'Ctrl + V' in your new commit message.
>> Easy!
>>
>> If you consider it somehow not acceptable or extremely longer
>> than your way, you can create a script/alias doing it for you.
>> You just need to omit the date, and still add line breaks if
>> the line in your new commit message becomes too long.
> 
> Look, git already has the helper which does exactly what you
> prefer to see in output. I simply added this command to the
> our code-review page. If you don't like this example with
> `git log --pretty=format:"%h (%s)"` feel free to remove it
> from the page.

Nope, I won't. The command is useful if somebody else would complain
they need a special command for that. All the amendments to the
self-review guide are appreciated.

Re: [Tarantool-patches] [PATCH v2] qsync: provide box.info interface for monitoring

[Cyrill Gorcunov via Tarantool-patches]

On Thu, Apr 08, 2021 at 10:10:41PM +0200, Vladislav Shpilevoy wrote:
> > Look, git already has the helper which does exactly what you
> > prefer to see in output. I simply added this command to the
> > our code-review page. If you don't like this example with
> > `git log --pretty=format:"%h (%s)"` feel free to remove it
> > from the page.
> 
> Nope, I won't. The command is useful if somebody else would complain
> they need a special command for that. All the amendments to the
> self-review guide are appreciated.

+1 Actually I've been using "git log" in a long form first and
copy-pasted the output, then I recall that there definitely must
be a formatter because I use "--pretty=oneline" output very often,
but never the "format" option, so I dive into man and found literals
to share.