From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 597A145C304 for ; Sat, 28 Nov 2020 01:11:03 +0300 (MSK) References: <20201124131820.32981-1-sergepetrenko@tarantool.org> From: Vladislav Shpilevoy Message-ID: <22c104d3-bb7b-fd4d-ebd0-730133e8ded9@tarantool.org> Date: Fri, 27 Nov 2020 23:10:57 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Tarantool-patches] [PATCH] raft: make sure the leader stays ro till it clears the limbo List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Serge Petrenko , gorcunov@gmail.com Cc: tarantool-patches@dev.tarantool.org >> On 24.11.2020 14:18, Serge Petrenko wrote: >>> When running a cluster with leader election, its useful to wait till the >>> instance is writeable to determine that it has become a leader. However, >>> sometimes the instance fails to write data right after transitioning to >>> leader because its limbo still contains pending transactions from the >>> old leader. Make sure the instance deals with pending transactions first >>> and becomes writeable only once the limbo is empty. >> I just realized one thing. We can add a function txn_limbo_is_ro(), >> like we did with raft_is_ro(), account it in box_update_ro_summary(), >> and call box_update_ro_summary() when we see that the limbo is emptied, >> or when its ownership changes to a different instance. >> >> Probably would be simpler, and also we could make it work with manual >> election! So users could call box.ctl.wait_rw() even without using raft! >> >> To show concrete error if somebody still tries to write, we could >> patch box_check_writable() to show the reason why the instance is not >> writable. We will do it anyway for raft, to tell the users the real >> leader in case they are trying to write on a replica. In scope of >> https://github.com/tarantool/tarantool/issues/5568. >> >> Your version of the patch also looks good. >> >> What do you think? > > Thanks for your answer! > > Your proposal looks good. One question though. What about multimaster > synchro? Are we planning to support it one day? If yes, then limbo > emptiness will mean nothing. Indeed. But I have no idea if we will ever support it, and if yes - when. It is possible in theory, but we never tried to elaborate so far. I wouldn't expect it happening in the next year or so. > So, there're two options: > > 1) we may leave this patch as is. Then one won't be >    able to call wait_rw() with manual election. That's a pity, since >    your proposal looks quite logical, especially from the user's point >    of view. Having a single error for all these cases would be good. It also bothers me, that now box.ctl.wait_rw() actually does not wait for rw, strictly speaking. So it is probably even a bug, not a feature. A user can get wait_rw() true, but still won't be able to write. Value of such helper becomes zero with synchronous replication, in essence. > 2) Make limbo affect is_ro. Then everything's good for now, but we'll >    have to rewrite it back once (and if) we decide to implement >    multimaster synchro. Then the patch will look exactly like it does >    now. Perhaps. If nothing will change in raft and limbo significantly. > I'm not sure whether we're planning to make multimaster synchro work, > so I can't choose between these options and leave you to decide. We discuss it, but there is no really a plan. No customer request, and no ticket AFAIR. I assume some big customer must ask for this for us to even start planning. > Besides, looks like if we take option 2 the patch will differ in a single > line: `box_update_ro_summary()` after `box_clear_synchro_queue` Why? You will need to patch the limbo as well, not just remove box_update_ro_summary from after box_clear_synchro_queue. We will need to add a method like txn_limbo_is_ro() (similar to raft_is_ro()), use it in box_update_ro_summary(), and call the latter from some places in the limbo. I vote for the option 2. Mostly because I am afraid it is rather a bug. wait_rw just does not guarantee anything now.