From: Vladimir Davydov <vdavydov.dev@gmail.com> To: Konstantin Osipov <kostja@tarantool.org> Cc: tarantool-patches@freelists.org Subject: Re: [tarantool-patches] Re: [PATCH 9/9] wal: trigger checkpoint if there are too many WALs Date: Tue, 4 Dec 2018 14:25:20 +0300 [thread overview] Message-ID: <20181204112520.2di4acmhts24oj32@esperanza> (raw) In-Reply-To: <20181203203417.GI2890@chai> On Mon, Dec 03, 2018 at 11:34:17PM +0300, Konstantin Osipov wrote: > * Vladimir Davydov <vdavydov.dev@gmail.com> [18/11/28 19:16]: > > Please avoid using 0 for infinity: Tarantool doesn't use 0 to mean > anything special. As a matter of fact, we do - setting checkpoint_interval/count to 0 results in infinite checkpoint interval/count. I want to make checkpoint_wal_threshold consistent with those configuration options. Anyway, if 0 doesn't mean infinity, what should one set checkpoint_wal_threshold to to disable this feature? A very large value? What value? 100 GB, 100 TB? Would look weird in box.cfg IMO. > > > Closes #1082 > > > > @TarantoolBot document > > Title: Document box.cfg.checkpoint_wal_threshold > > Please document the default value of the new variable. OK. > > Please add checks for the range of valid values of the new > variable, as well as tests for these. We don't check checkpoint_interval - setting it to a value <= 0 means infinite timeout. I though why bother about checkpoint_wal_threshold then? > > > + int64_t checkpoint_wal_size; > > + /** > > + * If greater than 0 > > Ugh. > > + , this variable sets a limit on the > > + * total size of WAL files written since the last checkpoint. > > + * Exceeding it will trigger auto checkpointing in tx. > > + */ > > + int64_t checkpoint_threshold; > > > > + bool checkpoint_threshold_signalled; > > + bool checkpoint_threshold_exceeded; > > If you had the checkpoint object wit hall the messages in > the wal writer signleton, then the entire checkpoint state, > including this variable, could be easily observed in a single > place. Now that I see this flag I'm more inclined to insist > on having a singleton wal_checkpoint object, inside struct > wal_writer or standalone. I'll remove checkpoint_threshold_exceeded and will use a separate message for this kind of notifications instead of piggybacking on a WAL request, as we agreed. Regarding checkpoint_threshold_signalled, quite frankly, I don't think that introducing a new checkpoint state struct and putting it in there would make the code look any better. This flag isn't really bound with checkpointing - it merely indicates whether we've already triggered a checkpoint while checkpointing may or may not be in progress. > > > +void > > +wal_set_checkpoint_threshold(int64_t checkpoint_threshold) > > +{ > > + struct wal_writer *writer = &wal_writer_singleton; > > + if (writer->wal_mode == WAL_NONE) > > + return; > > + struct wal_set_checkpoint_threshold_msg msg; > > + msg.checkpoint_threshold = checkpoint_threshold; > > + bool cancellable = fiber_set_cancellable(false); > > + cbus_call(&wal_thread.wal_pipe, &wal_thread.tx_prio_pipe, > > + &msg.base, wal_set_checkpoint_threshold_f, NULL, > > + TIMEOUT_INFINITY); > > + fiber_set_cancellable(cancellable); > > +} > > Please add a comment explaining that WAL_NONE is also set when wal > is not yet initialized. OK. > > I don't see where you calculate the value of the variable upon > server start. Did I miss this hunk? No, it is set by load_cfg.lua, just like box.cfg.checkpoint_interval.
next prev parent reply other threads:[~2018-12-04 11:25 UTC|newest] Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-11-28 16:14 [PATCH 0/9] Allow to limit size of WAL files Vladimir Davydov 2018-11-28 16:14 ` [PATCH 1/9] wal: separate checkpoint and flush paths Vladimir Davydov 2018-11-29 16:24 ` [tarantool-patches] " Konstantin Osipov 2018-11-28 16:14 ` [PATCH 2/9] wal: remove files needed for recovery from backup checkpoints on ENOSPC Vladimir Davydov 2018-11-29 16:31 ` [tarantool-patches] " Konstantin Osipov 2018-11-29 17:42 ` Vladimir Davydov 2018-11-28 16:14 ` [PATCH 3/9] recovery: restore garbage collector vclock after restart Vladimir Davydov 2018-11-29 16:37 ` [tarantool-patches] " Konstantin Osipov 2018-11-29 17:42 ` Vladimir Davydov 2018-11-28 16:14 ` [PATCH 4/9] gc: run garbage collection in background Vladimir Davydov 2018-11-29 16:42 ` [tarantool-patches] " Konstantin Osipov 2018-11-29 17:43 ` Vladimir Davydov 2018-11-28 16:14 ` [PATCH 5/9] gc: do not use WAL watcher API for deactivating stale consumers Vladimir Davydov 2018-11-29 17:02 ` [tarantool-patches] " Konstantin Osipov 2018-11-28 16:14 ` [PATCH 6/9] wal: simplify watcher API Vladimir Davydov 2018-11-29 17:33 ` [tarantool-patches] " Konstantin Osipov 2018-11-28 16:14 ` [PATCH 7/9] box: rewrite checkpoint daemon in C Vladimir Davydov 2018-11-30 8:58 ` [tarantool-patches] " Konstantin Osipov 2018-11-30 9:41 ` Vladimir Davydov 2018-12-05 16:21 ` Vladimir Davydov 2018-11-28 16:14 ` [PATCH 8/9] wal: pass struct instead of vclock to checkpoint methods Vladimir Davydov 2018-11-30 9:00 ` [tarantool-patches] " Konstantin Osipov 2018-11-30 9:43 ` Vladimir Davydov 2018-12-03 20:20 ` Konstantin Osipov 2018-11-28 16:14 ` [PATCH 9/9] wal: trigger checkpoint if there are too many WALs Vladimir Davydov 2018-12-03 20:34 ` [tarantool-patches] " Konstantin Osipov 2018-12-04 11:25 ` Vladimir Davydov [this message] 2018-12-04 12:53 ` Konstantin Osipov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20181204112520.2di4acmhts24oj32@esperanza \ --to=vdavydov.dev@gmail.com \ --cc=kostja@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='Re: [tarantool-patches] Re: [PATCH 9/9] wal: trigger checkpoint if there are too many WALs' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox