[tarantool-patches] Re: [PATCH 7/9] box: rewrite checkpoint daemon in C

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev at 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