Re: [Tarantool-patches] [PATCH v4 1/2] applier: send transaction's first row WAL time in the applier_writer_f

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

Thanks for working on this!

See 3 comments below.

> diff --git a/src/box/applier.cc b/src/box/applier.cc
> index 33181fdbf..3038ce05f 100644
> --- a/src/box/applier.cc
> +++ b/src/box/applier.cc> @@ -789,6 +828,8 @@ apply_synchro_row_cb(struct journal_entry *entry)
>  	if (entry->res < 0) {
>  		applier_rollback_by_wal_io();
>  	} else {
> +		if (synchro_entry->wal_st != NULL)
> +			wal_stat_update(synchro_entry->wal_st);

1. You have 3 branches here: first in apply_synchro_row() where
you check replica_id != 0, then here, and then in wal_stat_update()
to check if the replica exists.

You could make it 2 if would add wal_stat in  apply_synchro_row()
always. Anyway there is no replica with replica_id 0, so
wal_state_update() won't do anything.

>  		txn_limbo_process(&txn_limbo, synchro_entry->req);
>  		trigger_run(&replicaset.applier.on_wal_write, NULL);
>  	}
> @@ -1088,11 +1151,14 @@ applier_apply_tx(struct applier *applier, struct stailq *rows)
>  		 * each other.
>  		 */
>  		assert(first_row == last_row);
> -		if ((rc = apply_synchro_row(first_row)) != 0)
> +		rc = apply_synchro_row(applier->instance_id, first_row);
> +		if (rc != 0)
> +			goto finish;
> +	} else {
> +		rc = apply_plain_tx(applier->instance_id, rows,
> +				    replication_skip_conflict, true);
> +		if (rc != 0)
>  			goto finish;

2. These 2 error checks are identical. You can move them out of if-else.

> -	} else if ((rc = apply_plain_tx(rows, replication_skip_conflict,
> -					true)) != 0) {
> -		goto finish;
>  	}
>  	vclock_follow(&replicaset.applier.vclock, last_row->replica_id,
>  		      last_row->lsn);
> diff --git a/src/box/replication.h b/src/box/replication.h
> index 2ad1cbf66..a40582cd3 100644
> --- a/src/box/replication.h
> +++ b/src/box/replication.h
> @@ -331,6 +331,10 @@ struct replica {
>  	 * separate from applier.
>  	 */
>  	enum applier_state applier_sync_state;
> +	/**
> +	 * Applier's WAL related statistics.
> +	 */
> +	struct applier_wal_stat wal_st;

3. The stat object also stores replica_id which is useless to store
in struct replica - you know its ID anyway. There is already
replica->id. I would suggest to store only the timestamp, and move
the stat declaration to .cc file.