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 120E56EC5D; Wed, 7 Apr 2021 23:53:30 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 120E56EC5D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1617828810; bh=FAtI2DzFoatGE/tGsZJFZBBRcFSpfNfJoyI9FR37UHw=; h=To:References:Date:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=k5Dhe8f+Jg+u4f0IncH0v2si7nVOtETb1iL9m2KTgvKvjlhqJYOmvBoSN+6uU+5Q+ uVPS262wj5tnB62vNqL0wMhh2+wIM25VHTXYks9zMBAzy/j3gJTh+5nxmI2wuXgXEP FMLeC/XLoX5MaPCSjLdL7E8OOov9tjav9hXgSExg= Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 822186EC5D for ; Wed, 7 Apr 2021 23:53:29 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 822186EC5D Received: by smtpng2.m.smailru.net with esmtpa (envelope-from ) id 1lUFB2-0006rx-Qo; Wed, 07 Apr 2021 23:53:29 +0300 To: Cyrill Gorcunov , tml References: <20210405155823.1121042-1-gorcunov@gmail.com> Message-ID: Date: Wed, 7 Apr 2021 22:53:27 +0200 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.9.0 MIME-Version: 1.0 In-Reply-To: <20210405155823.1121042-1-gorcunov@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD912A3E3D5D4B49FC130D613A19A4D1F6017F5C004462A7E0D00894C459B0CD1B99B5B579365E4D951B83AF9AFC67FEEDD4F15B2E7FE47776861CD1C36FF7CC69F X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7A0175C48BD57B26BC2099A533E45F2D0395957E7521B51C2CFCAF695D4D8E9FCEA1F7E6F0F101C6778DA827A17800CE71D16D29A965DB9EEEA1F7E6F0F101C67CDEEF6D7F21E0D1D9295C2E9FA3191EE1B59CA4C82EFA658BD40D89596C8999661C7C28DBB5527CDF6B57BC7E64490618DEB871D839B73339E8FC8737B5C22494854413538E1713FCC7F00164DA146DAFE8445B8C89999729449624AB7ADAF37F6B57BC7E64490611E7FA7ABCAF51C92176DF2183F8FC7C0DEC8C2C8BCD2534D8941B15DA834481F9449624AB7ADAF372E808ACE2090B5E14AD6D5ED66289B5259CC434672EE63711DD303D21008E298D5E8D9A59859A8B6B372FE9A2E580EFC725E5C173C3A84C3FB9365559B687AC835872C767BF85DA2F004C90652538430E4A6367B16DE6309 X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A2368A440D3B0F60894D097F901B7558688B39FA754255D0DFB8CCF60F6B7FD3948E1CD14B953EB46DCD5097902EC95E15355D89D7DBCDD132 X-C1DE0DAB: 0D63561A33F958A5221F38B94D1C9749CAE40D7EAB937E8D33F415CA6F5CB210D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D347341ACA13FB8BDD9B6F8EA2698AC7F7E25980FD02C4C77374E06F619A5149E95D2C8B70699AE92891D7E09C32AA3244CBC1DDCC732900DEF19445164C4962BC7C3B3ADDA61883BB5729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioju+jaMfvANXqRnEELS2RnWQ== X-Mailru-Sender: 689FA8AB762F73936BC43F508A0638227F7E596256EC283B08C2B98A143BEEAF3841015FED1DE5223CC9A89AB576DD93FB559BB5D741EB963CF37A108A312F5C27E8A8C3839CE0E267EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v2] qsync: provide box.info interface for monitoring 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: Vladislav Shpilevoy via Tarantool-patches Reply-To: Vladislav Shpilevoy Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "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 > > @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.