Re: [Tarantool-patches] [PATCH v6 1/3] replication: omit 0-th vclock component in replication responses

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

Hi! Thanks for the patch!

> diff --git a/src/box/xrow.c b/src/box/xrow.c
> index be026a43c..21a68220a 100644
> --- a/src/box/xrow.c
> +++ b/src/box/xrow.c
> @@ -51,7 +51,7 @@ static_assert(IPROTO_DATA < 0x7f && IPROTO_METADATA < 0x7f &&
>  static inline uint32_t
>  mp_sizeof_vclock(const struct vclock *vclock)
>  {
> -	uint32_t size = vclock_size(vclock);
> +	uint32_t size = vclock_size_ignore0(vclock);

This doesn't look right to silently ignore 0 component in a
function, which says nothing about that: mp_sizeof_vclock().
Not in the function name, nor in a comment. Maybe worth renaming
it to mp_sizeof_vclock_ignore0(). Along with mp_encode_vclock().

I am picky to the names, because we still actually have places,
where 0 component *is* needed (we have, right?). And we need to
say explicitly, which places ignore 0, which don't.

Also seems mp_decode_vclock() does not ignore 0. Is it possible
that we will get a connection from tarantool, which still does not
ignore 0 component, and will send it to us? For example, it was
upgraded recently on the latest branch, right before we pushed this
patchset.

>  	return mp_sizeof_map(size) + size * (mp_sizeof_uint(UINT32_MAX) +
>  					     mp_sizeof_uint(UINT64_MAX));
>  }