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

["Sergey Petrenko" <sergepetrenko@tarantool.org>]

>Воскресенье, 22 декабря 2019, 20:59 +03:00 от Vladislav Shpilevoy <v.shpilevoy@tarantool.org>:
>
>Thanks for the patch!

Hi! Thank you for review!

>
>See 2 comments below.
>
>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.

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

Done.

>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.

>
>Another option - rename it to applier_wait_snapshot(). It will be
>consistent with what I propose below.
>
>> @@ -456,9 +448,16 @@ applier_join(struct applier *applier)
>>    (uint32_t) row.type);
>>  }
>>  }
>> -say_info("initial data received");
>> 
>> -applier_set_state(applier, APPLIER_FINAL_JOIN);
>> +return row_count;
>> +}
>> +
>> +static uint64_t
>> +applier_do_register(struct applier *applier, uint64_t row_count)
>
>2. This looks rather like 'wait_register' because the only thing it
>does is waiting for register response.

Agreed.

diff --git a/src/box/applier.cc b/src/box/applier.cc
index 357369025..3e503f772 100644
--- a/src/box/applier.cc
+++ b/src/box/applier.cc
@@ -202,7 +202,7 @@ applier_writer_f(va_list ap)
 }
 
 static int
-apply_initial_join_row(struct xrow_header *row)
+apply_snapshot_row(struct xrow_header *row)
 {
 int rc;
 struct request request;
@@ -389,7 +389,7 @@ done:
 }
 
 static uint64_t
-applier_do_fetch_snapshot(struct applier *applier)
+applier_fetch_snapshot(struct applier *applier)
 {
 struct ev_io *coio = &applier->io;
 struct ibuf *ibuf = &applier->ibuf;
@@ -425,7 +425,7 @@ applier_do_fetch_snapshot(struct applier *applier)
 coio_read_xrow(coio, ibuf, &row);
 applier->last_row_time = ev_monotonic_now(loop());
 if (iproto_type_is_dml(row.type)) {
-if (apply_initial_join_row(&row) != 0)
+if (apply_snapshot_row(&row) != 0)
 diag_raise();
 if (++row_count % 100000 == 0)
 say_info("%.1fM rows received", row_count / 1e6);
@@ -453,7 +453,7 @@ applier_do_fetch_snapshot(struct applier *applier)
 }
 
 static uint64_t
-applier_do_register(struct applier *applier, uint64_t row_count)
+applier_wait_register(struct applier *applier, uint64_t row_count)
 {
 struct ev_io *coio = &applier->io;
 struct ibuf *ibuf = &applier->ibuf;
@@ -513,13 +513,13 @@ applier_join(struct applier *applier)
 
 applier_set_state(applier, APPLIER_INITIAL_JOIN);
 
-row_count = applier_do_fetch_snapshot(applier);
+row_count = applier_fetch_snapshot(applier);
 
 say_info("initial data received");
 
 applier_set_state(applier, APPLIER_FINAL_JOIN);
 
-if (applier_do_register(applier, row_count) == row_count) {
+if (applier_wait_register(applier, row_count) == row_count) {
 /*
  * We didn't receive any rows during registration.
  * Proceed to "subscribe" and do not finish bootstrap

>
>
>> +{
>> +struct ev_io *coio = &applier->io;
>> +struct ibuf *ibuf = &applier->ibuf;
>> +struct xrow_header row;
>> 
>>  /*
>>   * Tarantool < 1.7.0: there is no "final join" stage.


-- 
Sergey Petrenko