Re: [Tarantool-patches] [PATCH 1/2] gc/xlog: delay xlog cleanup until relays are subscribed

[Cyrill Gorcunov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

On Fri, Mar 19, 2021 at 04:50:38PM +0300, Serge Petrenko wrote:
> > +static void
> > +box_check_wal_cleanup_delay(int tmo)
> > +{
> > +	if (tmo < 0 && tmo != -1) {
> > +		tnt_raise(ClientError, ER_CFG, "wal_cleanup_delay",
> > +			  "the value must be either >= 0 or -1");
> > +	}
> > +}
> > +
> 
> 
> 1. AFAICS all other delay and timeout parameters are 'double'.
>    Let's make this one also a double.

Sure

> > +	/*
> > +	 * Now we can resume wal/engine gc as relay
> > +	 * is up and running.
> > +	 */
> > +	if (!relay->replica->anon) {
> > +		static const struct cmsg_hop gc_delay_route[] = {
> > +			{relay_gc_delay_unref, NULL}
> > +		};
> > +		struct cmsg gc_delay_msg;
> > +		cmsg_init(&gc_delay_msg, gc_delay_route);
> > +		cpipe_push(&relay->tx_pipe, &gc_delay_msg);
> > +	}
> > +
> 
> 2. I agree with Vlad here. No need to call gc_delay_unref() from the relay
> thread.
>    We need to wait until a corresponding gc consumer is registered. This is
> done
>    in relay_subscribe(), which is in tx thread.
> 
>    In fact, you may as well move the gc_delay_unref() call directly to
> gc_consumer_register().
>    This would look even better, IMO.

I don't mind to move it inside gc_consumer_register itself. Still this
bothers me a bit, the gc_consumer_register might be used not only by
relays since it is general api, would it be OK in a long term?