From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp44.i.mail.ru (smtp44.i.mail.ru [94.100.177.104]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id C77BD42EF5C for ; Mon, 22 Jun 2020 13:14:19 +0300 (MSK) References: <1a6094cfafb763decde38534e3095cc3b96e5cb2.1591701695.git.sergepetrenko@tarantool.org> <1bc2476a-01f9-a367-4138-c373915ce62e@tarantool.org> From: Serge Petrenko Message-ID: <7e5892d4-97d5-eab5-6766-3be9e5541198@tarantool.org> Date: Mon, 22 Jun 2020 13:14:18 +0300 MIME-Version: 1.0 In-Reply-To: <1bc2476a-01f9-a367-4138-c373915ce62e@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-GB Subject: Re: [Tarantool-patches] [PATCH 7/8] xrow: introduce CONFIRM entry List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Leonid Vasiliev , v.shpilevoy@tarantool.org, sergos@tarantool.org, gorcunov@gmail.com Cc: tarantool-patches@dev.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