[Tarantool-patches] [PATCH v4 1/2] applier: send transaction's first row WAL time in the applier_writer_f
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Wed May 12 22:59:49 MSK 2021
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.
More information about the Tarantool-patches
mailing list