Re: [Tarantool-patches] [PATCH 7/8] xrow: introduce CONFIRM entry

[Serge Petrenko <sergepetrenko@tarantool.org>]

19.06.2020 18:18, Leonid Vasiliev пишет:
> LGTM.
> All the following comments can be skipped silently.
Hi! Thanks for the review!
>
> On 09.06.2020 15:20, Serge Petrenko wrote:
>> Add methods to encode/decode CONFIRM entry.
>> A CONFIRM entry will be written to WAL by synchronous replication master
>> as soon as it finds that the transaction was applied on a quorum of
>> replicas.
>> CONFIRM rows share the same header with other rows in WAL,but their body
>> differs: it's just a map containing replica_id and lsn of the last
>> confirmed transaction.
>>
>> Part-of #4847
>> ---
>>   src/box/iproto_constants.h |  3 ++
>>   src/box/xrow.c             | 74 ++++++++++++++++++++++++++++++++++++++
>>   src/box/xrow.h             | 23 ++++++++++++
>>   3 files changed, 100 insertions(+)
>>
>> diff --git a/src/box/iproto_constants.h b/src/box/iproto_constants.h
>> index f8eee0f3f..1466b456f 100644
>> --- a/src/box/iproto_constants.h
>> +++ b/src/box/iproto_constants.h
>> @@ -219,6 +219,9 @@ enum iproto_type {
>>       /** The maximum typecode used for box.stat() */
>>       IPROTO_TYPE_STAT_MAX,
>>   +    /** A confirmation message for synchronous transactions. */
>> +    IPROTO_CONFIRM = 40,
>> +
>
> Seems like IPROTO_CONFIRM must be added to the documentation. If it's
> true, please add @TarantoolBot.
I agree, added doc requests for CONFIRM and ROLLBACK.
>
>>       /** PING request */
>>       IPROTO_PING = 64,
>>       /** Replication JOIN command */
>> diff --git a/src/box/xrow.c b/src/box/xrow.c
>> index bb64864b2..f197e0d85 100644
>> --- a/src/box/xrow.c
>> +++ b/src/box/xrow.c
>> @@ -878,6 +878,80 @@ xrow_encode_dml(const struct request *request, 
>> struct region *region,
>>       return iovcnt;
>>   }
>>   +int
>> +xrow_encode_confirm(struct xrow_header *row, uint32_t replica_id, 
>> int64_t lsn)
>> +{
>> +    size_t len = mp_sizeof_map(2) + mp_sizeof_uint(IPROTO_REPLICA_ID) +
>> +             mp_sizeof_uint(replica_id) + mp_sizeof_uint(IPROTO_LSN) +
>> +             mp_sizeof_uint(lsn);
>> +    char *buf = (char *)region_alloc(&fiber()->gc, len);
>> +    if (buf == NULL) {
>> +        diag_set(OutOfMemory, len, "region_alloc", "buf");
>> +        return -1;
>> +    }
>> +    char *pos = buf;
>> +
>> +    pos = mp_encode_map(pos, 2);
>> +    pos = mp_encode_uint(pos, IPROTO_REPLICA_ID);
>> +    pos = mp_encode_uint(pos, replica_id);
>> +    pos = mp_encode_uint(pos, IPROTO_LSN);
>> +    pos = mp_encode_uint(pos, lsn);
>> +
>> +    row->body[0].iov_base = buf;
>> +    row->body[0].iov_len = len;
>> +
>> +    row->type = IPROTO_CONFIRM;
>> +
>> +    return 1;
>> +}
>
> At the last version
> memset(row, 0, sizeof(*row));
> was added. But, usually it is initialized at the beginning of the
> function, because otherwise it is possible to do return without
> initializing of the row. It does not look like a problem, the decision
> is yours.

I left it as is, looks strange if I memset the row before an error can 
happen IMO.

>
>> +
>> +int
>> +xrow_decode_confirm(struct xrow_header *row, uint32_t *replica_id, 
>> int64_t *lsn)
>> +{
>> +    if (row->bodycnt == 0) {
>> +        diag_set(ClientError, ER_INVALID_MSGPACK, "request body");
>> +        return -1;
>> +    }
>> +
>> +    assert(row->bodycnt == 1);
>> +
>> +    const char * const data = (const char *)row->body[0].iov_base;
>> +    const char * const end = data + row->body[0].iov_len;
>> +    const char *d = data;
>> +    if (mp_check(&d, end) != 0 || mp_typeof(*data) != MP_MAP) {
>> +        xrow_on_decode_err(data, end, ER_INVALID_MSGPACK,
>> +                   "request body");
>> +        return -1;
>> +    }
>> +
>> +    d = data;
>> +    uint32_t map_size = mp_decode_map(&d);
>> +    for (uint32_t i = 0; i < map_size; i++) {
>> +        if (mp_typeof(*d) != MP_UINT) {
>> +            mp_next(&d);
>> +            mp_next(&d);
>> +            continue;
>> +        }
>> +        uint8_t key = mp_decode_uint(&d);
>> +        if (key >= IPROTO_KEY_MAX || iproto_key_type[key] !=
>> +                         mp_typeof(*d)) {
>> +                xrow_on_decode_err(data, end, ER_INVALID_MSGPACK,
>> +                           "request body");
>> +        }
>> +        switch (key) {
>> +        case IPROTO_REPLICA_ID:
>> +            *replica_id = mp_decode_uint(&d);
>> +            break;
>> +        case IPROTO_LSN:
>> +            *lsn = mp_decode_uint(&d);
>> +            break;
>> +        default:
>> +            mp_next(&d);
>> +        }
>> +    }
>> +    return 0;
>> +}
>> +
>>   int
>>   xrow_to_iovec(const struct xrow_header *row, struct iovec *out)
>>   {
>> diff --git a/src/box/xrow.h b/src/box/xrow.h
>> index 2a0a9c852..75af71b77 100644
>> --- a/src/box/xrow.h
>> +++ b/src/box/xrow.h
>> @@ -207,6 +207,29 @@ int
>>   xrow_encode_dml(const struct request *request, struct region *region,
>>           struct iovec *iov);
>>   +/**
>> + * Encode the CONFIRM to row body and set row type to
>> + * IPROTO_CONFIRM.
>> + * @param row xrow header.
>> + * @param replica_id master's instance id.
>> + * @param lsn last confirmed lsn.
>> + * @retval -1 on error.
>> + * @retval > 0 xrow bodycnt.
>> + */
>> +int
>> +xrow_encode_confirm(struct xrow_header *row, uint32_t replica_id, 
>> int64_t lsn);
>> +
>> +/**
>> + * Decode the CONFIRM request body.
>> + * @param row xrow header.
>> + * @param[out] replica_id master's instance id.
>> + * @param[out] lsn last confirmed lsn.
>> + * @retwal -1 on error.
>> + * @retwal 0 success.
>
> The typo is fixed by you in "xrow: fix comment typo".
>
Squashed it into this commit.

>> + */
>> +int
>> +xrow_decode_confirm(struct xrow_header *row, uint32_t *replica_id, 
>> int64_t *lsn);
>> +
>>   /**
>>    * CALL/EVAL request.
>>    */
>>
-- 
Serge Petrenko