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

[Tarantool-patches] [RFC] 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 tatistics in future.

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

 |  synchro:
 |    limbo:
 |      qlen: 0
 |      owner_id: 0
 |    quorum: 1

Where `qlen` stands for current limbo number of entries in queue,
`owner_id` for limbo owner, and finally `quorum` for evaluated
`replication_synchro_quorum` parameter.

Part-of #4642

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
issue https://github.com/tarantool/tarantool/issues/5191

Guys, I didn't put is anywhere yet, because I would like
to gather comments on design, so not for merge.

1) Vlad pointed that we might have several limbo instances
in future. Should I start printing limbos as map initially,
say

	synchro:
	  limbo:
	    1:
	      qlen: 0
	      owner_id: 0
	    2:
	      qlen: 0
	      owner_id: 0

where 1 and 2 gonna be either plain enumeration of limbo
instances?

2) Vlad, could you please comment which else values we
need for better testing?

3) Mons, could you please think through what else parameters
are might be needed for solutions team?

4) Any other ideas are highly appreciated!

 src/box/lua/info.c  | 29 +++++++++++++++++++++++++++++
 src/box/txn_limbo.c |  5 ++++-
 src/box/txn_limbo.h |  4 ++++
 3 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/src/box/lua/info.c b/src/box/lua/info.c
index c4c9fa0a0..f15d733b5 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 "tt_static.h"
@@ -594,6 +595,33 @@ lbox_info_election(struct lua_State *L)
 	return 1;
 }
 
+static int
+lbox_info_synchro(struct lua_State *L)
+{
+	struct txn_limbo *limbo = &txn_limbo;
+	(void)limbo;
+
+	/* box.info.synchro */
+	lua_newtable(L);
+
+	/* Quorum value may be evaluated via formula */
+	lua_pushinteger(L, replication_synchro_quorum);
+	lua_setfield(L, -2, "quorum");
+
+	/* Transation queue related data. */
+	lua_newtable(L);
+	lua_pushstring(L, "owner_id");
+	lua_pushnumber(L, limbo->owner_id);
+	lua_settable(L, -3);
+	lua_pushstring(L, "qlen");
+	lua_pushnumber(L, limbo->qlen);
+	lua_settable(L, -3);
+
+	lua_setfield(L, -2, "limbo");
+	return 1;
+}
+
+
 static const struct luaL_Reg lbox_info_dynamic_meta[] = {
 	{"id", lbox_info_id},
 	{"uuid", lbox_info_uuid},
@@ -613,6 +641,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 9c4c3cdf1..8abe9310d 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->qlen = 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->qlen++;
 	/*
 	 * 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->qlen--;
 }
 
 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->qlen--;
 	++limbo->rollback_count;
 }
 
diff --git a/src/box/txn_limbo.h b/src/box/txn_limbo.h
index c28b5666d..43505e7a2 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 qlen;
 	/**
 	 * Instance ID of the owner of all the transactions in the
 	 * queue. Strictly speaking, nothing prevents to store not
-- 
2.29.2

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

[Vladislav Shpilevoy via Tarantool-patches]

Hi! Thanks for the patch!

> Guys, I didn't put is anywhere yet, because I would like
> to gather comments on design, so not for merge.
> 
> 1) Vlad pointed that we might have several limbo instances
> in future. Should I start printing limbos as map initially,
> say
> 
> 	synchro:
> 	  limbo:
> 	    1:
> 	      qlen: 0
> 	      owner_id: 0
> 	    2:
> 	      qlen: 0
> 	      owner_id: 0

I am not sure 'limbo' name should be exposed. It is an
internal term. Also it looks strange to shorten 'queue len'
to 'qlen', but leave 'owner id' as is.

Besides, what is 'owner_id'? Don't synchro.limbo indexes
mean owner ids?

In future if we will have many limbos, they will belong to
one instance each. No ownwership transfers AFAIU. That is
the whole idea.

Perhaps a better naming would be this:

	synchro:
	  queue:
	    1:
	      len/length: <number>
          quorum: <number>

There is only one key now in each queue, but more may be
added in future, so I wouldn't flatten it.

Another problem I see - to get your own limbo you need to
know your instance id. So to get the stat you need to do the
ugly thing:

	mystat = box.info.synchro.queue[box.info.id]

Maybe it is not that bad though. I don't use monitoring so
can't tell. But would probably be nice to have

	box.info.synchro.local_queue

which would reference the same Lua table as used in
box.info.synchro.queue with the local id.

> where 1 and 2 gonna be either plain enumeration of limbo
> instances?
> 
> 2) Vlad, could you please comment which else values we
> need for better testing?

For testing the queue length and quorum value are enough
I hope.

> 3) Mons, could you please think through what else parameters
> are might be needed for solutions team?
> 
> 4) Any other ideas are highly appreciated!

I advise to ask the same questions to Vlad Grubov. He is
going to use it in prod.

> diff --git a/src/box/txn_limbo.h b/src/box/txn_limbo.h
> index c28b5666d..43505e7a2 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 qlen;

Maybe just call it 'size'? Or 'length'? Limbo is
the queue itself. So it looks like 'queue.qlen', which
sounds tautological.

>  	/**
>  	 * Instance ID of the owner of all the transactions in the
>  	 * queue. Strictly speaking, nothing prevents to store not