Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Cyrill Gorcunov <gorcunov@gmail.com>,
	tml <tarantool-patches@dev.tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH v4 1/2] applier: send transaction's first row WAL time in the applier_writer_f
Date: Wed, 12 May 2021 21:59:49 +0200	[thread overview]
Message-ID: <bc27f0a3-c2f6-b96a-aa49-d0504df2479b@tarantool.org> (raw)
In-Reply-To: <20210506214132.533913-2-gorcunov@gmail.com>

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.

  reply	other threads:[~2021-05-12 19:59 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-06 21:41 [Tarantool-patches] [PATCH v4 0/2] relay: provide downstream lag information Cyrill Gorcunov via Tarantool-patches
2021-05-06 21:41 ` [Tarantool-patches] [PATCH v4 1/2] applier: send transaction's first row WAL time in the applier_writer_f Cyrill Gorcunov via Tarantool-patches
2021-05-12 19:59   ` Vladislav Shpilevoy via Tarantool-patches [this message]
2021-05-06 21:41 ` [Tarantool-patches] [PATCH v4 2/2] relay: provide information about downstream lag Cyrill Gorcunov via Tarantool-patches
2021-05-12 20:10   ` Vladislav Shpilevoy via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=bc27f0a3-c2f6-b96a-aa49-d0504df2479b@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=gorcunov@gmail.com \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v4 1/2] applier: send transaction'\''s first row WAL time in the applier_writer_f' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox