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 581336EC55; Fri, 23 Jul 2021 10:46:11 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 581336EC55 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1627026371; bh=rm7ZKIToL0Ym9DaeOk+b5bRi0wiPeCFUsfbmcKOjszI=; 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=SSYLKw49fYL1ID+VC8YB0N5gv2r+pVDJQDoYUYsJUZMBIo4h3tlr+wa/A26CnK8kM dS7qPQqhA5I+BZ4zocqBeczAGp9Gr0vwf4nMXmkJ/9exRD3BDhvCR4dYAVup7zbQg6 UHnqI4+L6VRKAvrgfzTn/rzMSxO7U89lrYpixA0Q= Received: from smtp44.i.mail.ru (smtp44.i.mail.ru [94.100.177.104]) (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 A56326EC57 for ; Fri, 23 Jul 2021 10:45:05 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A56326EC57 Received: by smtp44.i.mail.ru with esmtpa (envelope-from ) id 1m6prk-0007LW-VJ; Fri, 23 Jul 2021 10:45:05 +0300 To: Vladislav Shpilevoy , gorcunov@gmail.com Cc: tarantool-patches@dev.tarantool.org References: <5c38c2b4f516ce3777b2f3374f6c76f8e9229448.1626287002.git.sergepetrenko@tarantool.org> Message-ID: Date: Fri, 23 Jul 2021 09:45:03 +0200 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.12.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD941C43E597735A9C320FB367CC9CBEE800EEBD3117ABA9A5B182A05F53808504069ED02E30A3C964B8BC3985C46B080674B35EB1C23D9FD0CB62E3732B768F6FD X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE751C3618ECA219898EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637F72512A705E00D288638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8915133DDF105E99BD94C7ADF7AEEF616117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC8C7ADC89C2F0B2A5A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18618001F51B5FD3F9D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE902A1BE408319B292D242C3BD2E3F4C64AD6D5ED66289B52698AB9A7B718F8C46E0066C2D8992A16725E5C173C3A84C34B6590F86FB8E8FBBA3038C0950A5D36B5C8C57E37DE458B0BC6067A898B09E46D1867E19FE14079C09775C1D3CA48CF3D321E7403792E342EB15956EA79C166A417C69337E82CC275ECD9A6C639B01B78DA827A17800CE7994FE22CF3C16DE0731C566533BA786AA5CC5B56E945C8DA X-C1DE0DAB: 0D63561A33F958A55617151B8A24A6A42D5EA6CB495CF2C1D8C3F68A9692FBC9D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7501A9DF589746230F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D344B4608D01B1CB59AA6AD814AA9BEE635BEE913C7C43DC5178440BBE8EAD492CEB80F6D462F2F30BA1D7E09C32AA3244CC995A7271C2AC32B6CDA725304F993B5B4DF56057A86259FFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojtKXY6NTIXk2liOMDUq1PfA== X-Mailru-Sender: 9482C2B233321BD2827D960CA7C4D2E2630808A4FB4BFFAA1C9A1058054341953395CDB77AD8BED26BB2E709EA627F343C7DDD459B58856F0E45BC603594F5A135B915D4279FF0579437F6177E88F7363CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v4 09/16] box: split promote() into reasonable parts 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: Sergey Petrenko via Tarantool-patches Reply-To: Sergey Petrenko Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 22.07.2021 01:26, Vladislav Shpilevoy пишет: > Thanks for working on this! > > See 3 comments below. Thanks for the review! >> diff --git a/src/box/box.cc b/src/box/box.cc >> index 86370514a..445875f8f 100644 >> --- a/src/box/box.cc >> +++ b/src/box/box.cc >> @@ -1527,6 +1527,147 @@ box_wait_quorum(uint32_t lead_id, int64_t target_lsn, int quorum, > <...> > >> + >> +/** >> + * Check whether the greatest promote term has changed since it was last read. >> + * IOW check that a foreign PROMOTE arrived while we were sleeping. >> + */ >> +static int >> +box_check_promote_term_changed(uint64_t promote_term) > 1. Normally you call check functions using the pattern > "check_something_correct". Here the correct behaviour is the term > being intact. So I propose to rename it to box_check_promote_term_intact. Ok, sure. >> +{ >> + if (txn_limbo.promote_greatest_term != promote_term) { >> + diag_set(ClientError, ER_INTERFERING_PROMOTE, >> + txn_limbo.owner_id); >> + return -1; >> + } >> + return 0; >> +} > <...> > >> + >> +/** >> + * A helper to wait until all limbo entries are ready to be confirmed, i.e. >> + * written to WAL and have gathered a quorum of ACKs from replicas. >> + * Return lsn of the last limbo entry on success, -1 on error. >> + */ >> +static int64_t >> +box_wait_limbo_acked(void) >> +{ >> + if (txn_limbo_is_empty(&txn_limbo)) >> + return txn_limbo.confirmed_lsn; >> + >> + uint64_t promote_term = txn_limbo.promote_greatest_term; >> + int quorum = replication_synchro_quorum; >> + struct txn_limbo_entry *last_entry; >> + last_entry = txn_limbo_last_synchro_entry(&txn_limbo); >> + /* Wait for the last entries WAL write. */ >> + if (last_entry->lsn < 0) { >> + int64_t tid = last_entry->txn->id; >> + >> + if (wal_sync(NULL) < 0) >> + return -1; >> + >> + if (box_check_promote_term_changed(promote_term) < 0) > 2. Why < 0? It is not a in the code guidelines, but don't we usually > use '!= 0'? '< 0' normally assumes you can get > 0, 0, and < 0 meaning > different things, like it is done in iproto occassionally. I've put '< 0' here without a second thought. I'm just used to if (smth() < 0) { err; }, I guess. AFAICS there are more places where we use if (rc != 0) { err;} more, so I'll change my code accordingly. >> + return -1; >> + if (txn_limbo_is_empty(&txn_limbo)) >> + return txn_limbo.confirmed_lsn; >> + if (tid != txn_limbo_last_synchro_entry(&txn_limbo)->txn->id) { >> + diag_set(ClientError, ER_QUORUM_WAIT, quorum, >> + "new synchronous transactions appeared"); >> + return -1; >> + } >> + } > <...> > >> + >> +/** Write and process a PROMOTE request. */ >> +static void >> +box_issue_promote(uint32_t prev_leader_id, int64_t promote_lsn) >> +{ >> + assert(box_raft()->volatile_term == box_raft()->term); >> + assert(promote_lsn >= 0); >> + txn_limbo_write_promote(&txn_limbo, promote_lsn, >> + box_raft()->term); > 3. Maybe cache box_raft() value in a variable? Its usage would look shorter > then. The same in other places where it is used more than once. Up to > you. Done. Incremental diff: ========================= diff --git a/src/box/box.cc b/src/box/box.cc index 341857267..d83c30918 100644 --- a/src/box/box.cc +++ b/src/box/box.cc @@ -1563,7 +1563,7 @@ box_run_elections(void)   * IOW check that a foreign PROMOTE arrived while we were sleeping.   */  static int -box_check_promote_term_changed(uint64_t promote_term) +box_check_promote_term_intact(uint64_t promote_term)  {      if (txn_limbo.promote_greatest_term != promote_term) {          diag_set(ClientError, ER_INTERFERING_PROMOTE, @@ -1579,7 +1579,7 @@ box_try_wait_confirm(double timeout)  {      uint64_t promote_term = txn_limbo.promote_greatest_term;      txn_limbo_wait_empty(&txn_limbo, timeout); -    return box_check_promote_term_changed(promote_term); +    return box_check_promote_term_intact(promote_term);  }  /** @@ -1604,7 +1604,7 @@ box_wait_limbo_acked(void)          if (wal_sync(NULL) < 0)              return -1; -        if (box_check_promote_term_changed(promote_term) < 0) +        if (box_check_promote_term_intact(promote_term) != 0)              return -1;          if (txn_limbo_is_empty(&txn_limbo))              return txn_limbo.confirmed_lsn; @@ -1618,10 +1618,10 @@ box_wait_limbo_acked(void)      int64_t wait_lsn = last_entry->lsn;      if (box_wait_quorum(txn_limbo.owner_id, wait_lsn, quorum, -                replication_synchro_timeout) < 0) +                replication_synchro_timeout) != 0)          return -1; -    if (box_check_promote_term_changed(promote_term) < 0) +    if (box_check_promote_term_intact(promote_term) != 0)          return -1;      if (txn_limbo_is_empty(&txn_limbo)) @@ -1722,10 +1722,10 @@ box_promote(void)      int64_t wait_lsn = -1; -    if (run_elections && box_run_elections() < 0) +    if (run_elections && box_run_elections() != 0)          return -1;      if (try_wait && -        box_try_wait_confirm(2 * replication_synchro_timeout) < 0) +        box_try_wait_confirm(2 * replication_synchro_timeout) != 0)          return -1;      if ((wait_lsn = box_wait_limbo_acked()) < 0)          return -1; ========================= >> + struct synchro_request req = { >> + .type = IPROTO_PROMOTE, >> + .replica_id = prev_leader_id, >> + .origin_id = instance_id, >> + .lsn = promote_lsn, >> + .term = box_raft()->term, >> + }; >> + txn_limbo_process(&txn_limbo, &req); >> + assert(txn_limbo_is_empty(&txn_limbo)); >> +}