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 83AE36EC5B; Thu, 13 May 2021 13:11:44 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 83AE36EC5B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1620900704; bh=970QYTt5QNwr/gQoJ1QHxFWrCubb40eFNy3KGdxw7mg=; 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=W7kFDSeeDI13A9gAV/s2iUl/9eHAJbaoAswvXU3o8I3wYpHh7VDKD8KPhr9ZpEh7F 2jxOP9KphN5OZeyaQBTqUfP29YerCLOWfTlDWYqiyO/Ek0qPxjPvpYsOOoQLNOkdow fNSr6WupN9S2zbPL2Pvae/3q1UFwIdF52V+DIVj0= Received: from smtp17.mail.ru (smtp17.mail.ru [94.100.176.154]) (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 8141B6EC5B for ; Thu, 13 May 2021 13:11:42 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 8141B6EC5B Received: by smtp17.mail.ru with esmtpa (envelope-from ) id 1lh8Jh-0006Hw-PM; Thu, 13 May 2021 13:11:42 +0300 To: Vladislav Shpilevoy , gorcunov@gmail.com Cc: tarantool-patches@dev.tarantool.org References: <20210512112928.12509-1-sergepetrenko@tarantool.org> Message-ID: Date: Thu, 13 May 2021 13:11:41 +0300 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 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: 4F1203BC0FB41BD95978C26455E69BE0FB16CA32063744B94D060C3A2F6991A4182A05F53808504018234B3A70B9960B324826CC01AB595F4508F92E2835152DB0D6741B17D9F960 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7378B36DA860966D8EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006378F586D843116CFB2EA1F7E6F0F101C67CDEEF6D7F21E0D1D9295C2E9FA3191EE1B59CA4C82EFA658DD55C1EFE47FB499B3B552678AFDE981F6B57BC7E64490618DEB871D839B73339E8FC8737B5C22496EA1BA7CA28B4A74CC7F00164DA146DAFE8445B8C89999729449624AB7ADAF37F6B57BC7E64490611E7FA7ABCAF51C92176DF2183F8FC7C0C8FBEF6EAD8577B08941B15DA834481F9449624AB7ADAF37BA3038C0950A5D3613377AFFFEAFD269176DF2183F8FC7C0C30ACD7F1301FA5A7B076A6E789B0E97A8DF7F3B2552694A1E7802607F20496D49FD398EE364050F1E561CDFBCA1751F96C9B5BF839F39F6B3661434B16C20AC78D18283394535A9E827F84554CEF5019E625A9149C048EE9ECD01F8117BC8BEE2021AF6380DFAD18AA50765F790063735872C767BF85DA227C277FBC8AE2E8BEB1A37DF9DABAA8F75ECD9A6C639B01B4E70A05D1297E1BBCB5012B2E24CD356 X-C1DE0DAB: 0D63561A33F958A5B5AEEDF71A3368BDF2BFF228F0E6DF539E1CB0504616E41FD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75F04B387B5D7535DE410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34C02783DDD5A45DEFBCB448D2561947D6145A68999CDA4AA8D0CDD2FF494DE6D8E543B38221FA50181D7E09C32AA3244C3894568F2C1D03CE5587167250EBFF4D853296C06374E602FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojoybArHp+PQXKPsNQ1MVX6w== X-Mailru-Sender: 583F1D7ACE8F49BD95918038521BA2AAD7A9C67904761859115AC4856B6268591469574CAE429050424AE0EB1F3D1D21E2978F233C3FAE6EE63DB1732555E4A8EE80603BA4A5B0BC112434F685709FCF0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v2] recovery: make it yield when positioning in a WAL 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" 12.05.2021 23:36, Vladislav Shpilevoy пишет: > Hi! Thanks for the patch! > > See 4 comments below. > > On 12.05.2021 13:29, Serge Petrenko wrote: Thanks for the review! >> We had various places in box.cc and relay.cc which counted processed >> rows and yielded every now and then. These yields didn't cover cases, >> when recovery has to position inside a long WAL file: >> >> For example, when tarantool exits without leaving an empty WAL file >> which'll be used to recover instance vclock on restart. In this case >> the instance freezes while processing the last availabe WAL in order > 1. availabe -> available. Fixed. > >> diff --git a/changelogs/unreleased/gh-5979-recovery-ligs.md b/changelogs/unreleased/gh-5979-recovery-ligs.md >> new file mode 100644 >> index 000000000..86abfd66a >> --- /dev/null >> +++ b/changelogs/unreleased/gh-5979-recovery-ligs.md >> @@ -0,0 +1,11 @@ >> +# bugfix/core >> + >> +* Now tarantool yields when scanning `.xlog` files for the latest applied vclock >> + and when finding the right place in `.xlog`s to start recovering. This means >> + that the instance is responsive right after `box.cfg` call even when an empty >> + `.xlog` was not created on previous exit. >> + Also this prevents relay from timing out when a freshly subscribed replica >> + needs rows from the end of a relatively long (hundreds of MBs) `.xlog` >> + (gh-5979). > 2. Maybe an empty line here? Could simplify reading a bit. Ok. Just noticed, the file had a strange name, so changed it as well. > >> +* The counter in `x.yM rows processed` log messages does not reset on each new >> + recovered `xlog` anymore. >> diff --git a/src/box/box.cc b/src/box/box.cc >> index 59925962d..8a7b8593d 100644 >> --- a/src/box/box.cc >> +++ b/src/box/box.cc >> @@ -610,15 +597,28 @@ end_error: >> diag_raise(); >> } >> >> +static struct wal_stream wal_stream; >> + >> +/** >> + * Plan a yield in recovery stream. Wal stream will execute it as soon as it's >> + * ready. >> + */ >> +static void >> +wal_stream_schedule_yield(void) > 3. Since now you have an object owning the callback, you > could pass it as a first argument, like 'this' in C++, > and like apply_row() does. Then you wouldn't need the > global wal_stream. Indeed. Fixed. > >> +{ >> + wal_stream.has_yield = true; >> + wal_stream_try_yield(&wal_stream); >> +} >> diff --git a/src/box/xstream.h b/src/box/xstream.h >> index d29ff4213..d27de09a3 100644 >> --- a/src/box/xstream.h >> +++ b/src/box/xstream.h >> @@ -41,16 +41,40 @@ extern "C" { >> struct xrow_header; >> struct xstream; >> >> +/** >> + * A type for a callback invoked by recovery after some batch of rows is >> + * processed. Is used mostly to unblock the event loop every now and then. >> + */ >> +typedef void (*schedule_yield_f)(void); > 4. How about moving closer to the consistency and calling it > xstream_yield_f/xstream_schedule_yield_f? So it has xstream_ prefix > like xstream_write_f does. I thought of xstream_schedule_yield_f, but that's too long. Let it be xstream_yield_f. Here's the incremental diff: ========================================== diff --git a/changelogs/unreleased/gh-5979-recovery-ligs.md b/changelogs/unreleased/gh-5979-recovery-yield.md similarity index 99% rename from changelogs/unreleased/gh-5979-recovery-ligs.md rename to changelogs/unreleased/gh-5979-recovery-yield.md index 86abfd66a..71bec5669 100644 --- a/changelogs/unreleased/gh-5979-recovery-ligs.md +++ b/changelogs/unreleased/gh-5979-recovery-yield.md @@ -7,5 +7,6 @@    Also this prevents relay from timing out when a freshly subscribed replica    needs rows from the end of a relatively long (hundreds of MBs) `.xlog`    (gh-5979). +  * The counter in `x.yM rows processed` log messages does not reset on each new    recovered `xlog` anymore. diff --git a/src/box/box.cc b/src/box/box.cc index 8a7b8593d..c10e0d8bf 100644 --- a/src/box/box.cc +++ b/src/box/box.cc @@ -597,17 +597,16 @@ end_error:      diag_raise();  } -static struct wal_stream wal_stream; -  /**   * Plan a yield in recovery stream. Wal stream will execute it as soon as it's   * ready.   */  static void -wal_stream_schedule_yield(void) +wal_stream_schedule_yield(struct xstream *base)  { -    wal_stream.has_yield = true; -    wal_stream_try_yield(&wal_stream); +    struct wal_stream *stream = container_of(base, struct wal_stream, base); +    stream->has_yield = true; +    wal_stream_try_yield(stream);  }  static void @@ -3124,6 +3123,7 @@ local_recovery(const struct tt_uuid *instance_uuid,      say_info("instance uuid %s", tt_uuid_str(&INSTANCE_UUID)); +    struct wal_stream wal_stream;      wal_stream_create(&wal_stream);      auto stream_guard = make_scoped_guard([&]{          wal_stream_abort(&wal_stream); @@ -3131,7 +3131,8 @@ local_recovery(const struct tt_uuid *instance_uuid,      struct recovery *recovery;      bool is_force_recovery = cfg_geti("force_recovery"); -    recovery = recovery_new(wal_dir(), is_force_recovery, checkpoint_vclock); +    recovery = recovery_new(wal_dir(), is_force_recovery, +                checkpoint_vclock);      /*       * Make sure we report the actual recovery position diff --git a/src/box/recovery.cc b/src/box/recovery.cc index e5b88fcc2..1476622c3 100644 --- a/src/box/recovery.cc +++ b/src/box/recovery.cc @@ -142,7 +142,7 @@ recovery_scan(struct recovery *r, struct vclock *end_vclock,      while (xlog_cursor_next(&cursor, &row, true) == 0) {          vclock_follow_xrow(end_vclock, &row);          if (++stream->row_count % WAL_ROWS_PER_YIELD == 0) -            xstream_schedule_yield(stream); +            xstream_yield(stream);      }      xlog_cursor_close(&cursor, false); @@ -250,7 +250,7 @@ recover_xlog(struct recovery *r, struct xstream *stream,      while (xlog_cursor_next_xc(&r->cursor, &row,                     r->wal_dir.force_recovery) == 0) {          if (++stream->row_count % WAL_ROWS_PER_YIELD == 0) { -            xstream_schedule_yield(stream); +            xstream_yield(stream);          }          if (stream->row_count % 100000 == 0)              say_info("%.1fM rows processed", diff --git a/src/box/relay.cc b/src/box/relay.cc index 81ac35bf2..efc201e80 100644 --- a/src/box/relay.cc +++ b/src/box/relay.cc @@ -262,8 +262,9 @@ relay_new(struct replica *replica)  /** A callback recovery calls every now and then to unblock the event loop. */  static void -relay_yield(void) +relay_yield(struct xstream *stream)  { +    (void) stream;      fiber_sleep(0);  } diff --git a/src/box/xstream.h b/src/box/xstream.h index d27de09a3..b3c771eba 100644 --- a/src/box/xstream.h +++ b/src/box/xstream.h @@ -45,36 +45,36 @@ struct xstream;   * A type for a callback invoked by recovery after some batch of rows is   * processed. Is used mostly to unblock the event loop every now and then.   */ -typedef void (*schedule_yield_f)(void); +typedef void (*xstream_yield_f)(struct xstream *);  typedef void (*xstream_write_f)(struct xstream *, struct xrow_header *);  struct xstream {      xstream_write_f write; -    schedule_yield_f schedule_yield; +    xstream_yield_f yield;      uint64_t row_count;  };  static inline void  xstream_create(struct xstream *xstream, xstream_write_f write, -           schedule_yield_f schedule_yield) +           xstream_yield_f yield)  {      xstream->write = write; -    xstream->schedule_yield = schedule_yield; +    xstream->yield = yield;      xstream->row_count = 0;  }  static inline void -xstream_schedule_yield(struct xstream *stream) +xstream_yield(struct xstream *stream)  { -    stream->schedule_yield(); +    stream->yield(stream);  }  static inline void  xstream_reset(struct xstream *stream)  {      stream->row_count = 0; -    xstream_schedule_yield(stream); +    xstream_yield(stream);  }  int ========================================== -- Serge Petrenko