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 81B2E6EC5B; Tue, 30 Mar 2021 11:16:06 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 81B2E6EC5B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1617092166; bh=AfwurV/naTiJnRxRXv3XFPTNTZTgGUUyGSBiXR8hp2E=; 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=kgsTmJ1PkzoCPpAWwAH4fjWRP/wSCCx3q1nBQcRzIS+jSqiRhmoEpVfY72EK5sUTC fS+gyaAU004PnqlKmpBCzfDJnOukrTTB+neRI4pg2mBe971Wk9waZe8J+S588fXAyx YwHd+C+9qoo4APSPDF2p+onE0nt3j3r8Crv30z0o= Received: from smtp53.i.mail.ru (smtp53.i.mail.ru [94.100.177.113]) (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 E5B3A6EC5B for ; Tue, 30 Mar 2021 11:15:44 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org E5B3A6EC5B Received: by smtp53.i.mail.ru with esmtpa (envelope-from ) id 1lR9XM-0004NW-6h; Tue, 30 Mar 2021 11:15:44 +0300 To: Vladislav Shpilevoy , gorcunov@gmail.com Cc: tarantool-patches@dev.tarantool.org References: <5cfa8f8e4d733aabd53138dd3ffc6f524ffac743.1616588119.git.sergepetrenko@tarantool.org> <63555ea6-6f0a-964e-65e2-6213eecb2ea1@tarantool.org> <30275fb1-4797-6923-c2a0-17e670720e65@tarantool.org> Message-ID: Date: Tue, 30 Mar 2021 11:15:43 +0300 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: <30275fb1-4797-6923-c2a0-17e670720e65@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9ED7173E37F4E32947287414FD1D04A09E656A5F3377C994A182A05F538085040E3A0172D5DC32832A97EA2D6743749DEDB343F2D3842B4C487349C382CCD176E X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7B8387EA37EC1BE7DEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006379C6BD13E0D9523298638F802B75D45FF914D58D5BE9E6BC131B5C99E7648C95CE99938B3FD79E1DF89001FF83ED96423D95D3381B2AC2F18A471835C12D1D9774AD6D5ED66289B5259CC434672EE6371117882F4460429724CE54428C33FAD30A8DF7F3B2552694AC26CFBAC0749D213D2E47CDBA5A9658378DA827A17800CE77A825AB47F0FC8649FA2833FD35BB23DF004C906525384302BEBFE083D3B9BA71A620F70A64A45A98AA50765F790063735872C767BF85DA227C277FBC8AE2E8B693784CDF876D4B675ECD9A6C639B01B4E70A05D1297E1BBCB5012B2E24CD356 X-C1DE0DAB: 0D63561A33F958A53DF00375FCD69594A99B4BF125D50E18572381C05C6E11B3D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D349A949488F6BF46DDBA28600AB906A7B1699F357E945D9D33B61C46EF4ADA36858E05B0D41EF3EA741D7E09C32AA3244CE2DB7D2DCB31C11E1426D9ACD58944AAC86C126E7119A0FEFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojljIiQOC84rTnexTHxofuKQ== X-Mailru-Sender: 3B9A0136629DC9125D61937A2360A4462A4013516AE6581FBB4BE02B0E07DA09911FC8635B099D6E424AE0EB1F3D1D21E2978F233C3FAE6EE63DB1732555E4A8EE80603BA4A5B0BC112434F685709FCF0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v2 5/7] applier: make final join transactional 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" 30.03.2021 00:51, Vladislav Shpilevoy пишет: > Good job on the fixes! > > See 4 comments below. Thanks for the review! > >> ================================= >> diff --git a/src/box/applier.cc b/src/box/applier.cc >> index b96eb360b..0f4492fe3 100644 >> --- a/src/box/applier.cc >> +++ b/src/box/applier.cc >> @@ -497,6 +496,7 @@ struct applier_tx_row { >>  static uint64_t >>  applier_wait_register(struct applier *applier, uint64_t row_count) >>  { >> +#define ROWS_PER_LOG 100000 > 1. Better avoid in-function macro. This can be done as 'const uint64_t' or > as a enum in the beginning of the file. Ok. > >>      /* >>       * Tarantool < 1.7.0: there is no "final join" stage. >>       * Proceed to "subscribe" and do not finish bootstrap >> @@ -505,16 +505,23 @@ applier_wait_register(struct applier *applier, uint64_t row_count) >>      if (applier->version_id < version_id(1, 7, 0)) >>          return row_count; >> >> +    uint64_t next_log_cnt = >> +        row_count + ROWS_PER_LOG - row_count % ROWS_PER_LOG; >>      /* >>       * Receive final data. >>       */ >>      while (true) { >>          struct stailq rows; >> -        applier_read_tx(applier, &rows, &row_count); >> +        row_count += applier_read_tx(applier, &rows, TIMEOUT_INFINITY); >> +        if (row_count >= next_log_cnt) { >> +            say_info("%.1fM rows received", next_log_cnt / 1e6); >> +            next_log_cnt += ROWS_PER_LOG; > 2. What if row_count > ROWS_PER_LOG? Then it would be printed on the > next transaction immediately again (although I don't know if it is possible > to have such a big transaction). First of all, I don't think someone will have a 100k-row-long transaction. Secondly, even if this is the case, yes, the second info message will be printed almost immediately after the first one. But is it a problem? Say, we had row_count = 2 599 999, then we receive a transaction worth 100 001 rows. We'll print 2.6M rows received first, and then 2.7M rows received after the next transaction. An alternative would be: ``` while (row_count >= next_log_cnt) {     say_info(...)     next_log_cnt += ROWS_PER_LOG } ``` I like this more, actually, so let's change. Thanks for pointing this out! > >> +        } >>          struct xrow_header *first_row = >>              &stailq_first_entry(&rows, struct applier_tx_row, >>                          next)->row; >>          if (first_row->type == IPROTO_OK) { >> +            /* Current vclock. This is not used now, ignore. */ >>              assert(first_row == >>                     &stailq_last_entry(&rows, struct applier_tx_row, >>                            next)->row); >> @@ -1234,6 +1229,15 @@ applier_subscribe(struct applier *applier) >>          trigger_clear(&on_rollback); >>      }); >> >> +    /* >> +     * Tarantool < 1.7.7 does not send periodic heartbeat >> +     * messages so we can't assume that if we haven't heard >> +     * from the master for quite a while the connection is >> +     * broken - the master might just be idle. >> +     */ >> +    double timeout = applier->version_id < version_id(1, 7, 7) ? >> +             TIMEOUT_INFINITY : replication_disconnect_timeout(); > 3. What if replication_timeout is changed after first box.cfg{}? It > seems it won't affect the running appliers now, will it? Missed that, thanks! > >> + >>      /* >>       * Process a stream of rows from the binary log. >>       */ > <...> > >> +/** A simpler version of applier_apply_tx() for final join stage. */ >> +static int >> +apply_final_join_tx(struct stailq *rows) >> +{ >> + struct xrow_header *first_row = >> + &stailq_first_entry(rows, struct applier_tx_row, next)->row; >> + struct xrow_header *last_row = >> + &stailq_last_entry(rows, struct applier_tx_row, next)->row; >> + int rc = 0; >> + /* WAL isn't enabled yet, so follow vclock manually. */ >> + vclock_follow_xrow(&replicaset.vclock, last_row); >> + if (unlikely(iproto_type_is_synchro_request(first_row->type))) { >> + assert(first_row == last_row); >> + rc = apply_synchro_row(first_row); >> + goto end; >> + } > 4. You don't really need the 'end' label here: > > ==================== > --- a/src/box/applier.cc > +++ b/src/box/applier.cc > @@ -970,11 +970,9 @@ apply_final_join_tx(struct stailq *rows) > if (unlikely(iproto_type_is_synchro_request(first_row->type))) { > assert(first_row == last_row); > rc = apply_synchro_row(first_row); > - goto end; > + } else { > + rc = apply_plain_tx(rows, false, false); > } > - > - rc = apply_plain_tx(rows, false, false); > -end: > fiber_gc(); > return rc; > } Good point, thanks! ========================== diff --git a/src/box/applier.cc b/src/box/applier.cc index 0f4492fe3..f00ffbd34 100644 --- a/src/box/applier.cc +++ b/src/box/applier.cc @@ -59,6 +59,13 @@  STRS(applier_state, applier_STATE); +enum { +    /** +     * How often to log received row count. Used during join and register. +     */ +    ROWS_PER_LOG = 100000, +}; +  static inline void  applier_set_state(struct applier *applier, enum applier_state state)  { @@ -435,7 +442,7 @@ applier_wait_snapshot(struct applier *applier)          if (iproto_type_is_dml(row.type)) {              if (apply_snapshot_row(&row) != 0)                  diag_raise(); -            if (++row_count % 100000 == 0) +            if (++row_count % ROWS_PER_LOG == 0)                  say_info("%.1fM rows received", row_count / 1e6);          } else if (row.type == IPROTO_OK) {              if (applier->version_id < version_id(1, 7, 0)) { @@ -496,7 +503,6 @@ struct applier_tx_row {  static uint64_t  applier_wait_register(struct applier *applier, uint64_t row_count)  { -#define ROWS_PER_LOG 100000      /*       * Tarantool < 1.7.0: there is no "final join" stage.       * Proceed to "subscribe" and do not finish bootstrap @@ -513,7 +519,7 @@ applier_wait_register(struct applier *applier, uint64_t row_count)      while (true) {          struct stailq rows;          row_count += applier_read_tx(applier, &rows, TIMEOUT_INFINITY); -        if (row_count >= next_log_cnt) { +        while (row_count >= next_log_cnt) {              say_info("%.1fM rows received", next_log_cnt / 1e6);              next_log_cnt += ROWS_PER_LOG;          } @@ -532,7 +538,6 @@ applier_wait_register(struct applier *applier, uint64_t row_count)      }      return row_count; -#undef ROWS_PER_LOG  }  static void @@ -970,11 +975,9 @@ apply_final_join_tx(struct stailq *rows)      if (unlikely(iproto_type_is_synchro_request(first_row->type))) {          assert(first_row == last_row);          rc = apply_synchro_row(first_row); -        goto end; +    } else { +        rc = apply_plain_tx(rows, false, false);      } - -    rc = apply_plain_tx(rows, false, false); -end:      fiber_gc();      return rc;  } @@ -1229,15 +1232,6 @@ applier_subscribe(struct applier *applier)          trigger_clear(&on_rollback);      }); -    /* -     * Tarantool < 1.7.7 does not send periodic heartbeat -     * messages so we can't assume that if we haven't heard -     * from the master for quite a while the connection is -     * broken - the master might just be idle. -     */ -    double timeout = applier->version_id < version_id(1, 7, 7) ? -             TIMEOUT_INFINITY : replication_disconnect_timeout(); -      /*       * Process a stream of rows from the binary log.       */ @@ -1250,6 +1244,16 @@ applier_subscribe(struct applier *applier)              applier_set_state(applier, APPLIER_FOLLOW);          } +        /* +         * Tarantool < 1.7.7 does not send periodic heartbeat +         * messages so we can't assume that if we haven't heard +         * from the master for quite a while the connection is +         * broken - the master might just be idle. +         */ +        double timeout = applier->version_id < version_id(1, 7, 7) ? +                 TIMEOUT_INFINITY : +                 replication_disconnect_timeout(); +          struct stailq rows;          applier_read_tx(applier, &rows, timeout); -- Serge Petrenko