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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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?