Re: [Tarantool-patches] [PATCH 3/5] applier: split join processing into two stages

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

Hi! Thanks for the fixes!

>> On 15/12/2019 21:58, sergepetrenko wrote:
>>> From: Serge Petrenko < sergepetrenko@tarantool.org >
>>>
>>> We already have 'initial join' and 'final join' stages in applier logic.
>>> The first actually means fetching master's snapshot, and the second one
>>> -- receiving the rows which should contain replica's registration in
>>> _cluster.
>>> These stages will be used separately once anonymous replica is
>>> implemented, so split them as a preparation.
>>>
>>> Prerequisite #3186
>>> ---
>>>  src/box/applier.cc | 61 ++++++++++++++++++++++++++++++++++++----------
>>>  1 file changed, 48 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/src/box/applier.cc b/src/box/applier.cc
>>> index 42374f886..357369025 100644
>>> --- a/src/box/applier.cc
>>> +++ b/src/box/applier.cc
>>> @@ -388,18 +388,12 @@ done:
>>>  applier_set_state(applier, APPLIER_READY);
>>>  }
>>>
>>> -/**
>>> - * Execute and process JOIN request (bootstrap the instance).
>>> - */
>>> -static void
>>> -applier_join(struct applier *applier)
>>> +static uint64_t
>>> +applier_do_fetch_snapshot(struct applier *applier)
>>>  {
>>> -/* Send JOIN request */
>>>  struct ev_io *coio = &applier->io;
>>>  struct ibuf *ibuf = &applier->ibuf;
>>>  struct xrow_header row;
>>> -xrow_encode_join_xc(&row, &INSTANCE_UUID);
>>> -coio_write_xrow(coio, &row);
>>>
>>>  /**
>>>   * Tarantool < 1.7.0: if JOIN is successful, there is no "OK"
>>
>> 1. applier_do_fetch_snapshot() still uses JOIN in some comments
>> and in apply_initial_join_row() function name. The function can
>> be renamed right here. The comments should be fixed when you add
>> REQUEST_SNAPSHOT.
> 
> Renamed the function. The join request is still sent during "normal"
> replication. So I'll leave JOIN mentions in the comments and also write
> something about FETCH_SNAPSHOT in the next commit of the series.
> 

Yes, that what I meant.

>>
>> Also I propose you to rename it to just applier_fetch_snapshot().
> 
> Done.
> 

Perhaps, but looks like you didn't push it on the branch.

>> And the applier_fetch_snapshot() which you add later rename to
>> applier_request_and_fetch_snapshot(). But it is arguable. Lets
>> discuss if you disagree. I don't have a strong opinion here.
> 
> Maybe just use applier_get_snapshot() ? 
> applier_request_and_fetch_snapshot() looks too long IMO.
> 

'get' is basically the same as 'fetch' in that context.
In that case how about the proposal below?

    applier_do_fetch_snapshot -> applier_wait_snapshot

And keep applier_fetch_snapshot() introduced in the next
patches.