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 D59E16EC58; Mon, 21 Jun 2021 18:02:54 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org D59E16EC58 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1624287774; bh=j8Wmh5XQOPqzRMlxNfWX8uWRS76Br6FR/33L38FMoS8=; h=To:Cc:References:Date:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=P85kIkD7gzS/i+GVALp9YPChTCnBnQDiOjbtJTWhWHMQmrmhEkwRiqcmdR29lrOHn 9AIc+tOUtHCPpztPTrJJXHvFfF27KqmlF5CtILPhT77N3JikNi98vmjJ98Fh2Sv33K Fa2au7MTPCZq+6R81+uuNpX3t0qSf9ZRFADiJG2E= Received: from smtp56.i.mail.ru (smtp56.i.mail.ru [217.69.128.36]) (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 CCAE36EC58 for ; Mon, 21 Jun 2021 18:02:52 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org CCAE36EC58 Received: by smtp56.i.mail.ru with esmtpa (envelope-from ) id 1lvLRs-0003Or-2r; Mon, 21 Jun 2021 18:02:52 +0300 To: Vladislav Shpilevoy , gorcunov@gmail.com Cc: tarantool-patches@dev.tarantool.org References: <31dfa1eb-fa8a-001d-a47f-0038e71e7bdb@tarantool.org> Message-ID: <4d43aaa8-cabd-b762-906d-299596ca9779@tarantool.org> Date: Mon, 21 Jun 2021 18:02:50 +0300 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <31dfa1eb-fa8a-001d-a47f-0038e71e7bdb@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD91C2C07775F13263A075164279ADA4DEE4DF6CBEA76D0A4B700894C459B0CD1B9FE98BBB7D4AE215699192B75DC9E015210D5A1075979132AF649426FA9741B02 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE780DF8B060B28AA3EEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637BC6099E116BE74F18638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8314A9016840E9D0C7208A5CC1935D258117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC2EE5AD8F952D28FBA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18618001F51B5FD3F9D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6BF3059D42242344A089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A5BF7C9E7553CE733C02DC89AB467A3F73D36D3A32A286BB55D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75448CF9D3A7B2C848410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34B5900AD87B4159A47FF08E9CBCA2E26E553B5EA40FD8AB1E7F60ECADE8244299AB918B467C6CBEE71D7E09C32AA3244CDDF4D855B018688BED286A35C88B49A63A76366E8A9DE7CAFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj8x+Gb+jwA+S4OEosr5KkmQ== X-Mailru-Sender: 3B9A0136629DC9125D61937A2360A446A6010E5E3FD1283A5CCE7044B33686E2C16B137F9509A407424AE0EB1F3D1D21E2978F233C3FAE6EE63DB1732555E4A8EE80603BA4A5B0BC112434F685709FCF0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH 7/7] box: make promote/demote always bump the term 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: Serge Petrenko via Tarantool-patches Reply-To: Serge Petrenko Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 19.06.2021 01:53, Vladislav Shpilevoy пишет: > Thanks for the patch! > > See 4 comments below. > >> box: make promote always bump the term >> >> When called without elections, promote and resulted in multiple > 1. 'and' is excess. Thanks, fixed. > >> PROMOTE entries for the same term. This is not right, because all >> the promotions for the same term except the first one would be ignored >> as already seen. >> >> Part-of #6034 >> >> diff --git a/src/box/box.cc b/src/box/box.cc >> index 6a0950f44..53a8f80e5 100644 >> --- a/src/box/box.cc >> +++ b/src/box/box.cc >> @@ -1687,16 +1687,18 @@ box_promote(void) >> rc = -1; >> } else { >> promote: >> - /* We cannot possibly get here in a volatile state. */ >> - assert(box_raft()->volatile_term == box_raft()->term); >> + if (try_wait) >> + raft_new_term(box_raft()); > 2. It starts to bother me, that we use try_wait flag as a kind of a > state of the promote process rather just just a command 'you need to wait'. > But I can't propose anything better right away. Just noticing. I agree. I'd like to refactor box_clear_synchro_queue()/box_ctl_promote() sometime soon. It starts to look really bad in my opinion. > >> diff --git a/test/replication/gh-4114-local-space-replication.result b/test/replication/gh-4114-local-space-replication.result >> index 9b63a4b99..676400cef 100644 >> --- a/test/replication/gh-4114-local-space-replication.result >> +++ b/test/replication/gh-4114-local-space-replication.result> @@ -77,9 +76,9 @@ box.space.test:insert{3} >> | - [3] >> | ... >> >> -box.info.vclock[0] >> +box.info.vclock[0] == a + 3 or box.info.vclock[0] - a > 3. Maybe use an assertion? They really do look easier to read > when you try to understand a test. Ok, sure. > >> | --- >> - | - 3 >> + | - true >> | ... >> diff --git a/test/replication/gh-6034-promote-bump-term.result b/test/replication/gh-6034-promote-bump-term.result >> new file mode 100644 >> index 000000000..20e352922 >> --- /dev/null >> +++ b/test/replication/gh-6034-promote-bump-term.result >> @@ -0,0 +1,37 @@ >> +-- test-run result file version 2 >> +test_run = require('test_run').new() >> + | --- >> + | ... >> + >> +-- gh-6034: test that every box.ctl.promote() bumps >> +-- the instance's term. Even when elections are disabled. Even for consequent >> +-- promotes on the same instance. >> +election_mode = box.cfg.election_mode >> + | --- >> + | ... >> +box.cfg{election_mode='off'} >> + | --- >> + | ... >> + >> +term = box.info.election.term >> + | --- >> + | ... >> +box.ctl.promote() >> + | --- >> + | ... >> +assert(box.info.election.term == term + 1) >> + | --- >> + | - true >> + | ... >> +box.ctl.promote() >> + | --- >> + | ... >> +assert(box.info.election.term == term + 2) >> + | --- >> + | - true > 4. Could you please remind why do we issue a new promote even > if we own the limbo already? > > Especially if its ownership is going to get ever more strict > after this series. > Hmm. I just didn't change that behaviour. Why not though? Since it bumps the term as well. Say, when promote is issued in 'manual' election mode, it results in a new election every time. So, why not make it bump the term for each consequent call even when elections are off? Here's the diff: ======================================= diff --git a/test/replication/gh-4114-local-space-replication.result b/test/replication/gh-4114-local-space-replication.result index 676400cef..e71eb60a8 100644 --- a/test/replication/gh-4114-local-space-replication.result +++ b/test/replication/gh-4114-local-space-replication.result @@ -76,7 +76,7 @@ box.space.test:insert{3}   | - [3]   | ... -box.info.vclock[0] == a + 3 or box.info.vclock[0] - a +assert(box.info.vclock[0] == a + 3)   | ---   | - true   | ... diff --git a/test/replication/gh-4114-local-space-replication.test.lua b/test/replication/gh-4114-local-space-replication.test.lua index 8f787db84..65fef3bf6 100644 --- a/test/replication/gh-4114-local-space-replication.test.lua +++ b/test/replication/gh-4114-local-space-replication.test.lua @@ -27,7 +27,7 @@ box.space.test:insert{2}  box.snapshot()  box.space.test:insert{3} -box.info.vclock[0] == a + 3 or box.info.vclock[0] - a +assert(box.info.vclock[0] == a + 3)  test_run:cmd('switch default') ======================================= -- Serge Petrenko