Re: [Tarantool-patches] [PATCH v5 2/4] replication: hide 0-th vclock components in replication responses

[Serge Petrenko <sergepetrenko@tarantool.org>]

Hi! Thanks for the review!

> 4 апр. 2020 г., в 23:51, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> написал(а):
> 
> Thanks for the patch!
> 
> Was it considered to ignore 0 component on receiver's
> side rather than on sender's?
> 
> I see this:
> 
>>    This is needed for backward compatibility with old instances, which
>>    don't ignore 0th vclock component coming from a remote instance by
>>    default.
> 
> But can anon replicas connect to old versions?

Anon replicas can be promoted to normal ones, and then connect
to old versions, while still having a non-zero vclock[0].

> 
> I am not saying that it would be better, but I don't
> see why technically not.
> 
> On 30/03/2020 13:04, Serge Petrenko wrote:
>> If an anonymous replica is promoted to a normal one and becomes
>> replication master later, its vclock contains a non-empty zero
>> component, tracking local changes on this replica from the time when it
>> had been anonymous. No need to pollute joining instance's vclock with
>> our non-empty 0 component.
>> When an anonymous replica reports its status to a remote instance it
>> should also hide its 0-th vclock component.
>> 
>> This is needed for backward compatibility with old instances, which
>> don't ignore 0th vclock component coming from a remote instance by
>> default.
>> Also make sure that new instances ignore 0th vclock component.
>> 
>> Follow-up #3186
>> Prerequisite #4114
>> ---
>> src/box/applier.cc             |  4 +++-
>> src/box/box.cc                 | 12 ++++++++----
>> src/box/relay.cc               |  6 ++++--
>> test/replication/anon.result   |  5 +++++
>> test/replication/anon.test.lua |  2 ++
>> 5 files changed, 22 insertions(+), 7 deletions(-)
>> 
>> diff --git a/src/box/applier.cc b/src/box/applier.cc
>> index 47a26c366..f5f67b6a9 100644
>> --- a/src/box/applier.cc
>> +++ b/src/box/applier.cc
>> @@ -173,7 +173,9 @@ applier_writer_f(va_list ap)
>> 			continue;
>> 		try {
>> 			struct xrow_header xrow;
>> -			xrow_encode_vclock(&xrow, &replicaset.vclock);
>> +			struct vclock vclock;
>> +			vclock_copy_ignore0(&vclock, &replicaset.vclock);
>> +			xrow_encode_vclock(&xrow, &vclock);
> 
> xrow_encode_vclock without 0 component is needed 4 times.
> With 0 it is encoded 2 times. Maybe better add a function
> xrow_encode_vclock_ignore0 or like that. Because copy_ignore0
> is copying of ~290 bytes. This is several cache lines.

Okay, will do.

> 
> Probably even the original xrow_encode_vclock can appear to be
> not needed anywhere.

--
Serge Petrenko
sergepetrenko@tarantool.org