Re: [Tarantool-patches] [RFC v3 2/3] applier: send first row's WAL time in the applier_writer_f

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thanks for working on this!

On 30.04.2021 17:39, Cyrill Gorcunov wrote:
> This fibers sends current vclock of the node to remote relay reader.
> This packet carries xrow_header::tm field as a zero but we can reuse
> it to provide information about first timestamp in a transaction we
> wrote to our WAL. Since old instances of Tarantool simply ignore this
> field such extension won't cause any problems.
> 
> This timestamp will be needed to account lags of downstream replicas
> suitable for information purpose and cluster health monitoring.
> 
> We update applier statistics in WAL callbacks but since both
> apply_synchro_row and apply_plain_tx are used not only in real data
> application but in final join stage as well (in this stage we're not
> yet writing the data) the apply_synchro_row is extended with a flag
> pointing that applier update is needed.
> 
> Same time the apply_plain_tx uses asynchronous WAL write completion
> and at moment when the write procedure is finished the applier might
> be removed from replicaset already thus we use applier's instance

Did you mean instance id?

> to lookup if it is still alive.
> 
> The calculation of the downstream lag itself lag will be addressed

One of the 'lag' words is redundant.

> in next patch because sending the timestamp and its observation
> are independent actions.

See 6 comments below.

> diff --git a/src/box/applier.cc b/src/box/applier.cc
> index 33181fdbf..626dc0324 100644
> --- a/src/box/applier.cc
> +++ b/src/box/applier.cc
> @@ -751,11 +752,41 @@ applier_txn_rollback_cb(struct trigger *trigger, void *event)
>  	return 0;
>  }
>  
> +/** Applier WAL related statistics */
> +struct awstat {

1. Please, lets avoid such hard contractions. At first I
thought you decided to do something related to AWS when saw
this name.

	struct applier_lag

would be just fine.

> +	uint32_t instance_id;
> +	double first_row_tm;
> +};
> +
> +static void
> +awstat_update(struct awstat *awstat)
> +{
> +	/* Ignore if not needed */
> +	if (awstat->instance_id == 0)
> +		return;

2. Why did you even allocate this stat if it is not needed?
Maybe it would be better to have it NULL then and check
for NULL? AFAIU these are the initial and final join cases.

Did you try the way I proposed about waiting for all applier's
WAL writes to end in applier_stop()? Does it look worse? After
the fiber stop and wal_sync() it would be safe to assume there
are no WAL writes in fly from this applier. But I don't know if
it would look better.

> +
> +	/*
> +	 * Write to WAL happens in two contexts: as
> +	 * synchronous writes and as asynchronous. In
> +	 * second case the applier might be already
> +	 * stopped and removed.> +	 */
> +	struct replica *r = replica_by_id(awstat->instance_id);
> +	if (r == NULL && r->applier == NULL)

3. There is another idea - store the timestamp in struct replica.
Then it is -1 dereference. Although you would need to call
replica_by_id() before each ACK, but one ACK covers multiple
transactions and it would mean less lookups than now.

> +		return;
> +
> +	r->applier->first_row_wal_time = awstat->first_row_tm;

4. In case there was a batch of transactions written to WAL,
the latest one will override the timestamp of the previous ones and
this would make the lag incorrect, because you missed the older
transactions. Exactly like when you tried to take a timestamp of
the last row instead of the first row, but in a bigger scope.
Unless I missed something.

Probably you need to assign a new timestamp only when the old
one is not 0, and reset it to 0 on each sent ACK. Don't know.

> @@ -817,6 +852,12 @@ apply_synchro_row(struct xrow_header *row)
>  			     apply_synchro_row_cb, &entry);
>  	entry.req = &req;
>  	entry.owner = fiber();
> +	if (use_awstat) {

5. You don't really need this flag. Because during joins applier's
instance ID should be zero anyway. Therefore you would assign
stat.instance_id = 0 in this case regardless of the flag.

Also this means you don't need the entire applier struct. You only
need the instance_id as an argument. I am saying this because these
functions apply_* didn't not depend on a concrete applier object
probably exactly because it could be deleted. Not having an applier
pointer in their arguments could be intentional.

> +		entry.awstat.instance_id = applier->instance_id;
> +		entry.awstat.first_row_tm = row->tm;
> +	} else {
> +		entry.awstat.instance_id = 0;
> +	}> diff --git a/src/box/applier.h b/src/box/applier.h
> index 15ca1fcfd..bd98827e7 100644
> --- a/src/box/applier.h
> +++ b/src/box/applier.h
> @@ -93,6 +93,11 @@ struct applier {
>  	ev_tstamp last_row_time;
>  	/** Number of seconds this replica is behind the remote master */
>  	ev_tstamp lag;
> +	/**
> +	 * WAL time of first applied row in a transaction.
> +	 * For relay statistics sake.

6. It is not a first applied row on the whole. Only for the latest
transaction written to WAL.

> +	 */
> +	double first_row_wal_time;
>  	/** The last box_error_code() logged to avoid log flooding */
>  	uint32_t last_logged_errcode;
>  	/** Remote instance ID. */
>