[Tarantool-patches] [PATCH v3 02/10] xrow: introduce a PROMOTE entry
Serge Petrenko
sergepetrenko at tarantool.org
Fri Apr 16 19:18:03 MSK 2021
16.04.2021 02:19, Vladislav Shpilevoy пишет:
> I appreciate the work you did here!
>
> See 2 comments below.
>
>> diff --git a/src/box/xrow.c b/src/box/xrow.c
>> index cc8e43ed4..5d515ce92 100644
>> --- a/src/box/xrow.c
>> +++ b/src/box/xrow.c
>> @@ -884,28 +884,63 @@ xrow_encode_dml(const struct request *request, struct region *region,
>> return iovcnt;
>> }
>>
>> -void
>> -xrow_encode_synchro(struct xrow_header *row,
>> - struct synchro_body_bin *body,
>> - const struct synchro_request *req)
>> +static void
>> +xrow_encode_synchro_body(struct synchro_body_bin *body,
>> + const struct synchro_request *req)
>> {
>> /*
>> - * A map with two elements. We don't compress
>> + * A map with two or three elements. We don't compress
>> * numbers to have this structure constant in size,
>> * which allows us to preallocate it on stack.
>> */
>> - body->m_body = 0x80 | 2;
>> + body->m_body = 0x80 | (req->type == IPROTO_PROMOTE ? 3 : 2);
>> body->k_replica_id = IPROTO_REPLICA_ID;
>> body->m_replica_id = 0xce;
>> body->v_replica_id = mp_bswap_u32(req->replica_id);
>> body->k_lsn = IPROTO_LSN;
>> body->m_lsn = 0xcf;
>> body->v_lsn = mp_bswap_u64(req->lsn);
>> +}
>> +
>> +void
>> +xrow_encode_synchro(struct xrow_header *row,
>> + struct synchro_body_bin *body,
>> + const struct synchro_request *req)
>> +{
>> + assert(req->type == IPROTO_CONFIRM || req->type == IPROTO_ROLLBACK);
>> +
>> + xrow_encode_synchro_body(body, req);
>>
>> memset(row, 0, sizeof(*row));
>> + row->type = req->type;
>> + row->body[0].iov_base = body;
>> + row->body[0].iov_len = sizeof(*body);
>> + row->bodycnt = 1;
>> +}
>> +
>> +static inline void
>> +xrow_encode_promote_body(struct promote_body_bin *body,
>> + const struct synchro_request *req)
> 1. I would propose to inline it. It is used in a single place,
> and now it looks like if we would have more than 1 place where
> we would need the promote body.
>
> But more interestingly, it looks you could keep it a single
> function xrow_encode_synchro. Although we wouldn't be able to
> have a PACKED struct with predefined fields. Not a big deal
> anyway.
>
> The reasoning is similar to xrow_encode_dml(). It also uses
> a single struct request for all kinds of DML, and conditionally
> encodes the non-zero fields. I think your case is the same. It
> would simplify some code, and remove branching from other
> places. For example, from txn_limbo_write_synchro(), where you
> branch between PROMOTE and non-PROMOTE when decide what to encode.
> We even had the same issue when tried to encode CONFIRM and
> ROLLBACK via separate functions.
Ok, let's encode the entries together.
Incremental diff is below.
>
>> +{
>> + xrow_encode_synchro_body(&body->base, req);
>> +
>> + body->k_term = IPROTO_TERM;
>> + body->m_term = 0xcf;
>> + body->v_term = mp_bswap_u64(req->term);
>> +}
>> +
>>
>> +void
>> +xrow_encode_promote(struct xrow_header *row, struct promote_body_bin *body,
>> + const struct synchro_request *req)
>> +{
>> + assert(req->type == IPROTO_PROMOTE);
>> +
>> + xrow_encode_promote_body(body, req);
>> +
>> + memset(row, 0, sizeof(*row));
>> row->type = req->type;
>> - row->body[0].iov_base = (void *)body;
>> + row->body[0].iov_base = body;
> 2. Unnecessary change. But I don't mind, up to you.
This was accidental, but let's keep the change.
I'm either removing the unnecessary cast from xrow_encode_synchro()
or adding it to xrow_encode_promote().
I'd better remove it rather than add another place where it's used.
>
>> row->body[0].iov_len = sizeof(*body);
>> row->bodycnt = 1;
>> }
=======================================================
diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
index addcb0f97..c96e497c6 100644
--- a/src/box/txn_limbo.c
+++ b/src/box/txn_limbo.c
@@ -331,7 +331,7 @@ txn_limbo_write_synchro(struct txn_limbo *limbo,
uint16_t type, int64_t lsn)
* This is a synchronous commit so we can
* allocate everything on a stack.
*/
- struct synchro_body_bin body;
+ char body[XROW_SYNCHRO_BODY_LEN_MAX];
struct xrow_header row;
char buf[sizeof(struct journal_entry) +
sizeof(struct xrow_header *)];
@@ -339,7 +339,7 @@ txn_limbo_write_synchro(struct txn_limbo *limbo,
uint16_t type, int64_t lsn)
struct journal_entry *entry = (struct journal_entry *)buf;
entry->rows[0] = &row;
- xrow_encode_synchro(&row, &body, &req);
+ xrow_encode_synchro(&row, body, &req);
journal_entry_create(entry, 1, xrow_approx_len(&row),
txn_limbo_write_cb, fiber());
diff --git a/src/box/xrow.c b/src/box/xrow.c
index 5b689a86a..2e364cea5 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -884,56 +884,34 @@ xrow_encode_dml(const struct request *request,
struct region *region,
return iovcnt;
}
-static void
-xrow_encode_synchro_body(struct synchro_body_bin *body,
- const struct synchro_request *req)
-{
- /*
- * A map with two or three elements. We don't compress
- * numbers to have this structure constant in size,
- * which allows us to preallocate it on stack.
- */
- body->m_body = 0x80 | (req->type == IPROTO_PROMOTE ? 3 : 2);
- body->k_replica_id = IPROTO_REPLICA_ID;
- body->m_replica_id = 0xce;
- body->v_replica_id = mp_bswap_u32(req->replica_id);
- body->k_lsn = IPROTO_LSN;
- body->m_lsn = 0xcf;
- body->v_lsn = mp_bswap_u64(req->lsn);
-}
-
void
-xrow_encode_synchro(struct xrow_header *row,
- struct synchro_body_bin *body,
+xrow_encode_synchro(struct xrow_header *row, char *body,
const struct synchro_request *req)
{
- assert(req->type == IPROTO_CONFIRM || req->type == IPROTO_ROLLBACK);
+ assert(iproto_type_is_synchro_request(req->type));
- xrow_encode_synchro_body(body, req);
+ char *pos = body;
- memset(row, 0, sizeof(*row));
- row->type = req->type;
- row->body[0].iov_base = body;
- row->body[0].iov_len = sizeof(*body);
- row->bodycnt = 1;
-}
+ pos = mp_encode_map(pos,
+ iproto_type_is_promote_request(req->type) ? 3 : 2);
-void
-xrow_encode_promote(struct xrow_header *row, struct promote_body_bin *body,
- const struct synchro_request *req)
-{
- assert(req->type == IPROTO_PROMOTE);
+ pos = mp_encode_uint(pos, IPROTO_REPLICA_ID);
+ pos = mp_encode_uint(pos, req->replica_id);
- xrow_encode_synchro_body(&body->base, req);
+ pos = mp_encode_uint(pos, IPROTO_LSN);
+ pos = mp_encode_uint(pos, req->lsn);
+
+ if (iproto_type_is_promote_request(req->type)) {
+ pos = mp_encode_uint(pos, IPROTO_TERM);
+ pos = mp_encode_uint(pos, req->term);
+ }
- body->k_term = IPROTO_TERM;
- body->m_term = 0xcf;
- body->v_term = mp_bswap_u64(req->term);
+ assert(pos - body < XROW_SYNCHRO_BODY_LEN_MAX);
memset(row, 0, sizeof(*row));
row->type = req->type;
row->body[0].iov_base = body;
- row->body[0].iov_len = sizeof(*body);
+ row->body[0].iov_len = pos - body;
row->bodycnt = 1;
}
diff --git a/src/box/xrow.h b/src/box/xrow.h
index 80a5cd909..b3c664be2 100644
--- a/src/box/xrow.h
+++ b/src/box/xrow.h
@@ -49,6 +49,7 @@ enum {
XROW_IOVMAX = XROW_HEADER_IOVMAX + XROW_BODY_IOVMAX,
XROW_HEADER_LEN_MAX = 52,
XROW_BODY_LEN_MAX = 256,
+ XROW_SYNCHRO_BODY_LEN_MAX = 32,
IPROTO_HEADER_LEN = 28,
/** 7 = sizeof(iproto_body_bin). */
IPROTO_SELECT_HEADER_LEN = IPROTO_HEADER_LEN + 7,
@@ -260,25 +261,6 @@ struct synchro_request {
uint64_t term;
};
-/** Synchro request xrow's body in MsgPack format. */
-struct PACKED synchro_body_bin {
- uint8_t m_body;
- uint8_t k_replica_id;
- uint8_t m_replica_id;
- uint32_t v_replica_id;
- uint8_t k_lsn;
- uint8_t m_lsn;
- uint64_t v_lsn;
-};
-
-/** PROMOTE request's xrow body in MsgPack format. */
-struct PACKED promote_body_bin {
- struct synchro_body_bin base;
- uint8_t k_term;
- uint8_t m_term;
- uint64_t v_term;
-};
-
/**
* Encode synchronous replication request.
* @param row xrow header.
@@ -286,18 +268,7 @@ struct PACKED promote_body_bin {
* @param req Request parameters.
*/
void
-xrow_encode_synchro(struct xrow_header *row,
- struct synchro_body_bin *body,
- const struct synchro_request *req);
-
-/**
- * Encode a promote request.
- * @param row xrow header.
- * @param body A place to encode promote body.
- * @param req Request parameters.
- */
-void
-xrow_encode_promote(struct xrow_header *row, struct promote_body_bin *body,
+xrow_encode_synchro(struct xrow_header *row, char *body,
const struct synchro_request *req);
/**
--
Serge Petrenko
More information about the Tarantool-patches
mailing list