From: Konstantin Osipov <kostja@tarantool.org> To: tarantool-patches@freelists.org Subject: [tarantool-patches] Re: [PATCH 7/9] box: rewrite checkpoint daemon in C Date: Fri, 30 Nov 2018 11:58:10 +0300 [thread overview] Message-ID: <20181130085810.GG5760@chai> (raw) In-Reply-To: <3a95ae5e66ebfd72125eb53afb914efb5ab9cc0a.1543419109.git.vdavydov.dev@gmail.com> * Vladimir Davydov <vdavydov.dev@gmail.com> [18/11/28 19:16]: > Long time ago, when the checkpoint daemon was added to Tarantool, it was > responsible not only for making periodic checkpoints, but also for > maintaining the configured number of checkpoints and removing old snap > and xlog times, so it was much easier to implement it in Lua than in C. files > However, over time, all its responsibilities have been reimplemented in > C and moved to the server code so that now it just calls box.snapshot() > periodically. Let's rewrite this simple procedure in C as well - this > will allow us to easily add more complex logic there, e.g. triggering > checkpoint when WAL files exceed a configured threshold. > --- > src/box/CMakeLists.txt | 1 - > src/box/box.cc | 102 ++++++++++++++++++++++++ > src/box/box.h | 1 + > src/box/lua/cfg.cc | 12 +++ > src/box/lua/checkpoint_daemon.lua | 136 -------------------------------- > src/box/lua/init.c | 2 - > src/box/lua/load_cfg.lua | 2 +- > test/xlog/checkpoint_daemon.result | 145 ----------------------------------- Could we please move this fiber to gc.c at least? Let's not pollute box? > + if (box_checkpoint_is_in_progress) { > + /* > + * The next checkpoint will be scheduled > + * by the concurrent box_checkpoint(). > + */ This is rather fragile. Can we make the interaction between box-checkpoint and checkpoint daemon less obvious? Imagine in future we can perform checkpoints according to a cron. I think it would be better if the logic of next checkpoint time calculation is consolidated in the checkpoint daemon alone. If the daemon sees that a checkpoint is in progress it can skip the current checkpoint, but not delay the next checkpoint till infinity. > + next_checkpoint_time = now + TIMEOUT_INFINITY; > + continue; > + } > + box_checkpoint(); > + } > + checkpoint_daemon = NULL; > + return 0; > +} > + > --- a/test/xlog/checkpoint_daemon.test.lua > +++ b/test/xlog/checkpoint_daemon.test.lua Please keep the test. I don't understand what's wrong with it, neither it is obvious from changeset comments. -- Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 http://tarantool.io - www.twitter.com/kostja_osipov
next prev parent reply other threads:[~2018-11-30 8:58 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 ` Konstantin Osipov [this message] 2018-11-30 9:41 ` [tarantool-patches] " 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 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=20181130085810.GG5760@chai \ --to=kostja@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='[tarantool-patches] Re: [PATCH 7/9] box: rewrite checkpoint daemon in C' \ /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