[Tarantool-patches] [PATCH v5 4/4] box: start counting local space requests separately

Vladislav Shpilevoy v.shpilevoy at tarantool.org
Sat Apr 4 23:51:57 MSK 2020


Thanks for the patch!

See 3 comments below.

> diff --git a/src/box/box.cc b/src/box/box.cc
> index bf95d1b5e..0762266b0 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -1879,6 +1879,23 @@ box_process_subscribe(struct ev_io *io, struct xrow_header *header)
>  	say_info("remote vclock %s local vclock %s",
>  		 vclock_to_string(&replica_clock), vclock_to_string(&vclock));
>  
> +	/*
> +	 * Replica clock is used in gc state and recovery
> +	 * initialization, so we need to replace the remote 0-th
> +	 * component with our own one. This doesn't break
> +	 * recovery: it finds the WAL with a vclock strictly less
> +	 * than replia clock in all components except the 0th one.
> +	 * This leads to finding the correct WAL, if it exists,
> +	 * since we do not need to recover local rows (the ones,
> +	 * that contribute to the 0-th vclock component).
> +	 * Note, that it would be bad to set 0-th vclock component
> +	 * to a smaller value, since it would unnecessarily
> +	 * require additional WALs, which may have already been
> +	 * deleted.

1. But you just said above, that 0 component is ignored in all
comparators anyway. So what is a problem of setting 0 component
to 0?

> +	 * Speaking of gc, remote instances' local vclock
> +	 * components are not used by consumers at all.
> +	 */
> +	vclock_reset(&replica_clock, 0, vclock_get(&replicaset.vclock, 0));
> diff --git a/src/box/wal.c b/src/box/wal.c
> index 3b094b0e8..a74bdecd9 100644
> --- a/src/box/wal.c
> +++ b/src/box/wal.c
> @@ -953,13 +953,19 @@ wal_assign_lsn(struct vclock *vclock_diff, struct vclock *base,
>  	/** Assign LSN to all local rows. */
>  	for ( ; row < end; row++) {
>  		if ((*row)->replica_id == 0) {
> -			(*row)->lsn = vclock_inc(vclock_diff, instance_id) +
> -				      vclock_get(base, instance_id);
>  			/*
> -			 * Note, an anonymous replica signs local
> -			 * rows whith a zero instance id.
> +			 * All rows representing local space data
> +			 * manipulations are signed wth a zero
> +			 * instance id. This is also true for
> +			 * anonymous replicas, since they are
> +			 * only capable of writing to local and
> +			 * temporary spaces.
>  			 */
> -			(*row)->replica_id = instance_id;
> +			if ((*row)->group_id != GROUP_LOCAL)
> +				(*row)->replica_id = instance_id;
> +
> +			(*row)->lsn = vclock_inc(vclock_diff, (*row)->replica_id) +
> +						 vclock_get(base, (*row)->replica_id);

2. Bad indentation. vlock_get() should be aligned under vclock_inc().

>  			/* Use lsn of the first local row as transaction id. */
>  			tsn = tsn == 0 ? (*row)->lsn : tsn;
>  			(*row)->tsn = tsn;

3. I run the new test without your patch, and it just hangs. No
xloggaperror like in the ticket. Did it disappear thanks to the
previous commit, about GC?


More information about the Tarantool-patches mailing list