[tarantool-patches] Re: [PATCH v3 1/4] relay: adjust gc state on relay status update

[Konstantin Osipov]

* Georgy Kirichenko <georgy at tarantool.org> [19/10/09 19:50]:
> Don't use on_close_log trigger to track xlog file boundaries. As we
> intend implement in-memory replication relay could have no more xlog
> file operations and couldn't rely on previous trigger invocations. Now
> the consumer state is advanced together with relay vclock. After
> parallel applier implementation relay wouldn't receive an ACK packet for
> each transaction (because an applier groups them) so it should not be
> too expensive to advance gc on each relay vclock update.
> 
> Note: this changes cluster gc behavior - an instance gc will hold
> only it's locally generated transaction. Also it is only a
> temporary solution until relay processing would be merged with
> a wal writer context when wal will process relay ACK requests
> as well as log writing and redundancy evaluating.

I still don't fully get this change. You explain why you had to
remove the trigger, but why did you switch to instance ids vs
entire vlocks? 
You promise to restore the old behaviour but I haven't seen the
patch which restores it.

Meanwhile:

> --- a/src/box/gc.c
> +++ b/src/box/gc.c
> @@ -57,6 +57,7 @@
>  #include "engine.h"		/* engine_collect_garbage() */
>  #include "wal.h"		/* wal_collect_garbage() */
>  #include "checkpoint_schedule.h"
> +#include "replication.h"

You add a module dependency for one declaration only -
instance_id. 

Why not store instance lsn instead of entire vclock in struct gc_consumer
to avoid vclock_get and instance id dependency altogether?

Using instance id creates a tricky dependency between the order of
initialization of global variables - struct gc and struct
instance_id.

Today the order of initialization holds, tomorrow it will not. 


-- 
Konstantin Osipov, Moscow, Russia