[tarantool-patches] Re: [PATCH v5 2/2] replication: force gc to clean xdir on ENOSPC err

[Konstantin Osipov <kostja@tarantool.org>]

* Konstantin Belyavskiy <k.belyavskiy@tarantool.org> [18/07/05 22:56]:
> Garbage collector do not delete xlog unless replica do not notify
> master with newer vclock. This can lead to running out of disk
> space error and this is not right behaviour since it will stop the
> master.
> Fix it by forcing gc to clean xlogs for replica with highest lag.
> Add an error injection and a test.

Please rebase this patch to the latest 1.10

Please use relay_stop() as a callback to unregister the consumer.

> +void
> +gc_xdir_clean_notify()
> +{
> +	/*
> +	 * Compare the current time with the time of the last run.
> +	 * This is needed in case of multiple failures to prevent
> +	 * from deleting all replicas.


> +	 */
> +	static double prev_time = 0.;
> +	double cur_time = ev_monotonic_time();
> +	if (cur_time - prev_time < 1.)
> +		return;

This throttles gc, which is good. But we would still get a lot of messages
from WAL thread. Maybe we should move the throttling to the WAL
side? This would spare us from creating the message as well.
Ideally we should use a single statically allocated message from
the WAL for this purpose (but still throttle it as well).

Plus, eventually you're going to reach a state when kicking off
replicas doesn't help with space. In this case you're going to
have a lot of messages, and they are going to be all useless.
This also suggests that throttling should be done on the WAL side.

> +	prev_time = cur_time;
> +	struct gc_consumer *leftmost =
> +	    gc_tree_first(&gc.consumers);
> +	/*
> +	 * Exit if no consumers left or if this consumer is
> +	 * not associated with replica (backup for example).
> +	 */

> +	if (leftmost == NULL || leftmost->replica == NULL)
> +		return;


> +	/*
> +	 * We have to maintain @checkpoint_count oldest snapshots,
> +	 * plus we can't remove snapshots that are still in use.
> +	 * So if leftmost replica has signature greater or equel
> +	 * then the oldest checkpoint that must be preserved,
> +	 * nothing to do.
> +	 */

This comment is useful, but the search in checkpoint array is not.
What about possible other types of consumers which are not
dispensable with anyway, e.g. backups? What if they are holding a
reference as well? 

Apparently this check is taking care of the problem:

> +	if (leftmost == NULL || leftmost->replica == NULL)
> +		return;

Could you write a test with two 
"abandoned" replicas, each holding an xlog file?


-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov