From: Serge Petrenko <sergepetrenko@tarantool.org> To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH v6 1/3] replication: omit 0-th vclock component in replication responses Date: Fri, 10 Apr 2020 15:25:06 +0300 [thread overview] Message-ID: <9DB87F34-3674-4316-BA19-8DA801F4BBE9@tarantool.org> (raw) In-Reply-To: <e51ede3d-2eb7-2875-63b7-895a9cf33205@tarantool.org> > 10 апр. 2020 г., в 02:08, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> написал(а): > > Hi! Thanks for the patch! Hi! Thanks for your review! > >> diff --git a/src/box/xrow.c b/src/box/xrow.c >> index be026a43c..21a68220a 100644 >> --- a/src/box/xrow.c >> +++ b/src/box/xrow.c >> @@ -51,7 +51,7 @@ static_assert(IPROTO_DATA < 0x7f && IPROTO_METADATA < 0x7f && >> static inline uint32_t >> mp_sizeof_vclock(const struct vclock *vclock) >> { >> - uint32_t size = vclock_size(vclock); >> + uint32_t size = vclock_size_ignore0(vclock); > > This doesn't look right to silently ignore 0 component in a > function, which says nothing about that: mp_sizeof_vclock(). > Not in the function name, nor in a comment. Maybe worth renaming > it to mp_sizeof_vclock_ignore0(). Along with mp_encode_vclock(). > > I am picky to the names, because we still actually have places, > where 0 component *is* needed (we have, right?). And we need to > say explicitly, which places ignore 0, which don't. > > Also seems mp_decode_vclock() does not ignore 0. Is it possible > that we will get a connection from tarantool, which still does not > ignore 0 component, and will send it to us? For example, it was > upgraded recently on the latest branch, right before we pushed this > patchset. Ok, renamed mp_decode_vclock -> mp_decode_vclock_ignore0 mp_encode_vclock -> mp_encode_vclock_ignore0 I also replaced back the now unnecessary vclock_compare_ignore0 to vclock_compare. The diff’s below. diff --git a/src/box/relay.cc b/src/box/relay.cc index fec9f07d1..c634348a4 100644 --- a/src/box/relay.cc +++ b/src/box/relay.cc @@ -464,7 +464,7 @@ relay_schedule_pending_gc(struct relay *relay, const struct vclock *vclock) * the greater signatures is due to changes pulled * from other members of the cluster. */ - if (vclock_compare_ignore0(&curr->vclock, vclock) > 0) + if (vclock_compare(&curr->vclock, vclock) > 0) break; stailq_shift(&relay->pending_gc); free(gc_msg); diff --git a/src/box/replication.cc b/src/box/replication.cc index ac71e0d89..7c10fb6f2 100644 --- a/src/box/replication.cc +++ b/src/box/replication.cc @@ -775,8 +775,8 @@ replicaset_needs_rejoin(struct replica **master) continue; const struct ballot *ballot = &applier->ballot; - if (vclock_compare_ignore0(&ballot->gc_vclock, - &replicaset.vclock) <= 0) { + if (vclock_compare(&ballot->gc_vclock, + &replicaset.vclock) <= 0) { /* * There's at least one master that still stores * WALs needed by this instance. Proceed to local diff --git a/src/box/xrow.c b/src/box/xrow.c index 21a68220a..51de49387 100644 --- a/src/box/xrow.c +++ b/src/box/xrow.c @@ -49,7 +49,7 @@ static_assert(IPROTO_DATA < 0x7f && IPROTO_METADATA < 0x7f && "one byte"); static inline uint32_t -mp_sizeof_vclock(const struct vclock *vclock) +mp_sizeof_vclock_ignore0(const struct vclock *vclock) { uint32_t size = vclock_size_ignore0(vclock); return mp_sizeof_map(size) + size * (mp_sizeof_uint(UINT32_MAX) + @@ -57,7 +57,7 @@ mp_sizeof_vclock(const struct vclock *vclock) } static inline char * -mp_encode_vclock(char *data, const struct vclock *vclock) +mp_encode_vclock_ignore0(char *data, const struct vclock *vclock) { data = mp_encode_map(data, vclock_size_ignore0(vclock)); struct vclock_iterator it; @@ -74,7 +74,7 @@ mp_encode_vclock(char *data, const struct vclock *vclock) } static int -mp_decode_vclock(const char **data, struct vclock *vclock) +mp_decode_vclock_ignore_0(const char **data, struct vclock *vclock) { vclock_create(vclock); if (mp_typeof(**data) != MP_MAP) @@ -86,6 +86,14 @@ mp_decode_vclock(const char **data, struct vclock *vclock) uint32_t id = mp_decode_uint(data); if (mp_typeof(**data) != MP_UINT) return -1; + /* + * Skip vclock[0] coming from the remote + * instances. + */ + if (id == 0) { + mp_next(data); + continue; + } int64_t lsn = mp_decode_uint(data); if (lsn > 0) vclock_follow(vclock, id, lsn); @@ -412,7 +420,7 @@ iproto_reply_vclock(struct obuf *out, const struct vclock *vclock, uint64_t sync, uint32_t schema_version) { size_t max_size = IPROTO_HEADER_LEN + mp_sizeof_map(1) + - mp_sizeof_uint(UINT32_MAX) + mp_sizeof_vclock(vclock); + mp_sizeof_uint(UINT32_MAX) + mp_sizeof_vclock_ignore0(vclock); char *buf = obuf_reserve(out, max_size); if (buf == NULL) { @@ -424,7 +432,7 @@ iproto_reply_vclock(struct obuf *out, const struct vclock *vclock, char *data = buf + IPROTO_HEADER_LEN; data = mp_encode_map(data, 1); data = mp_encode_uint(data, IPROTO_VCLOCK); - data = mp_encode_vclock(data, vclock); + data = mp_encode_vclock_ignore0(data, vclock); size_t size = data - buf; assert(size <= max_size); @@ -445,8 +453,10 @@ iproto_reply_vote(struct obuf *out, const struct ballot *ballot, mp_sizeof_uint(UINT32_MAX) + mp_sizeof_map(4) + mp_sizeof_uint(UINT32_MAX) + mp_sizeof_bool(ballot->is_ro) + mp_sizeof_uint(UINT32_MAX) + mp_sizeof_bool(ballot->is_loading) + - mp_sizeof_uint(UINT32_MAX) + mp_sizeof_vclock(&ballot->vclock) + - mp_sizeof_uint(UINT32_MAX) + mp_sizeof_vclock(&ballot->gc_vclock); + mp_sizeof_uint(UINT32_MAX) + + mp_sizeof_vclock_ignore0(&ballot->vclock) + + mp_sizeof_uint(UINT32_MAX) + + mp_sizeof_vclock_ignore0(&ballot->gc_vclock); char *buf = obuf_reserve(out, max_size); if (buf == NULL) { @@ -464,9 +474,9 @@ iproto_reply_vote(struct obuf *out, const struct ballot *ballot, data = mp_encode_uint(data, IPROTO_BALLOT_IS_LOADING); data = mp_encode_bool(data, ballot->is_loading); data = mp_encode_uint(data, IPROTO_BALLOT_VCLOCK); - data = mp_encode_vclock(data, &ballot->vclock); + data = mp_encode_vclock_ignore0(data, &ballot->vclock); data = mp_encode_uint(data, IPROTO_BALLOT_GC_VCLOCK); - data = mp_encode_vclock(data, &ballot->gc_vclock); + data = mp_encode_vclock_ignore0(data, &ballot->gc_vclock); size_t size = data - buf; assert(size <= max_size); @@ -1244,11 +1254,13 @@ xrow_decode_ballot(struct xrow_header *row, struct ballot *ballot) ballot->is_loading = mp_decode_bool(&data); break; case IPROTO_BALLOT_VCLOCK: - if (mp_decode_vclock(&data, &ballot->vclock) != 0) + if (mp_decode_vclock_ignore_0(&data, + &ballot->vclock) != 0) goto err; break; case IPROTO_BALLOT_GC_VCLOCK: - if (mp_decode_vclock(&data, &ballot->gc_vclock) != 0) + if (mp_decode_vclock_ignore_0(&data, + &ballot->gc_vclock) != 0) goto err; break; default: @@ -1270,7 +1282,8 @@ xrow_encode_register(struct xrow_header *row, size_t size = mp_sizeof_map(2) + mp_sizeof_uint(IPROTO_INSTANCE_UUID) + mp_sizeof_str(UUID_STR_LEN) + - mp_sizeof_uint(IPROTO_VCLOCK) + mp_sizeof_vclock(vclock); + mp_sizeof_uint(IPROTO_VCLOCK) + + mp_sizeof_vclock_ignore0(vclock); char *buf = (char *) region_alloc(&fiber()->gc, size); if (buf == NULL) { diag_set(OutOfMemory, size, "region_alloc", "buf"); @@ -1281,7 +1294,7 @@ xrow_encode_register(struct xrow_header *row, data = mp_encode_uint(data, IPROTO_INSTANCE_UUID); data = xrow_encode_uuid(data, instance_uuid); data = mp_encode_uint(data, IPROTO_VCLOCK); - data = mp_encode_vclock(data, vclock); + data = mp_encode_vclock_ignore0(data, vclock); assert(data <= buf + size); row->body[0].iov_base = buf; row->body[0].iov_len = (data - buf); @@ -1298,7 +1311,8 @@ xrow_encode_subscribe(struct xrow_header *row, uint32_t id_filter) { memset(row, 0, sizeof(*row)); - size_t size = XROW_BODY_LEN_MAX + mp_sizeof_vclock(vclock); + size_t size = XROW_BODY_LEN_MAX + + mp_sizeof_vclock_ignore0(vclock); char *buf = (char *) region_alloc(&fiber()->gc, size); if (buf == NULL) { diag_set(OutOfMemory, size, "region_alloc", "buf"); @@ -1312,7 +1326,7 @@ xrow_encode_subscribe(struct xrow_header *row, data = mp_encode_uint(data, IPROTO_INSTANCE_UUID); data = xrow_encode_uuid(data, instance_uuid); data = mp_encode_uint(data, IPROTO_VCLOCK); - data = mp_encode_vclock(data, vclock); + data = mp_encode_vclock_ignore0(data, vclock); data = mp_encode_uint(data, IPROTO_SERVER_VERSION); data = mp_encode_uint(data, tarantool_version_id()); data = mp_encode_uint(data, IPROTO_REPLICA_ANON); @@ -1391,7 +1405,7 @@ xrow_decode_subscribe(struct xrow_header *row, struct tt_uuid *replicaset_uuid, case IPROTO_VCLOCK: if (vclock == NULL) goto skip; - if (mp_decode_vclock(&d, vclock) != 0) { + if (mp_decode_vclock_ignore_0(&d, vclock) != 0) { xrow_on_decode_err(data, end, ER_INVALID_MSGPACK, "invalid VCLOCK"); return -1; @@ -1473,7 +1487,7 @@ xrow_encode_vclock(struct xrow_header *row, const struct vclock *vclock) memset(row, 0, sizeof(*row)); /* Add vclock to response body */ - size_t size = 8 + mp_sizeof_vclock(vclock); + size_t size = 8 + mp_sizeof_vclock_ignore0(vclock); char *buf = (char *) region_alloc(&fiber()->gc, size); if (buf == NULL) { diag_set(OutOfMemory, size, "region_alloc", "buf"); @@ -1482,7 +1496,7 @@ xrow_encode_vclock(struct xrow_header *row, const struct vclock *vclock) char *data = buf; data = mp_encode_map(data, 1); data = mp_encode_uint(data, IPROTO_VCLOCK); - data = mp_encode_vclock(data, vclock); + data = mp_encode_vclock_ignore0(data, vclock); assert(data <= buf + size); row->body[0].iov_base = buf; row->body[0].iov_len = (data - buf); @@ -1498,7 +1512,8 @@ xrow_encode_subscribe_response(struct xrow_header *row, { memset(row, 0, sizeof(*row)); size_t size = mp_sizeof_map(2) + - mp_sizeof_uint(IPROTO_VCLOCK) + mp_sizeof_vclock(vclock) + + mp_sizeof_uint(IPROTO_VCLOCK) + + mp_sizeof_vclock_ignore0(vclock) + mp_sizeof_uint(IPROTO_CLUSTER_UUID) + mp_sizeof_str(UUID_STR_LEN); char *buf = (char *) region_alloc(&fiber()->gc, size); @@ -1509,7 +1524,7 @@ xrow_encode_subscribe_response(struct xrow_header *row, char *data = buf; data = mp_encode_map(data, 2); data = mp_encode_uint(data, IPROTO_VCLOCK); - data = mp_encode_vclock(data, vclock); + data = mp_encode_vclock_ignore0(data, vclock); data = mp_encode_uint(data, IPROTO_CLUSTER_UUID); data = xrow_encode_uuid(data, replicaset_uuid); assert(data <= buf + size); > >> return mp_sizeof_map(size) + size * (mp_sizeof_uint(UINT32_MAX) + >> mp_sizeof_uint(UINT64_MAX)); >> } -- Serge Petrenko sergepetrenko@tarantool.org
next prev parent reply other threads:[~2020-04-10 12:25 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-04-07 15:49 [Tarantool-patches] [PATCH v6 0/3] replication: fix local space tracking Serge Petrenko 2020-04-07 15:49 ` [Tarantool-patches] [PATCH v6 1/3] replication: omit 0-th vclock component in replication responses Serge Petrenko 2020-04-09 23:08 ` Vladislav Shpilevoy 2020-04-10 12:25 ` Serge Petrenko [this message] 2020-04-11 16:05 ` Vladislav Shpilevoy 2020-04-13 10:12 ` Serge Petrenko 2020-04-07 15:49 ` [Tarantool-patches] [PATCH v6 2/3] gc: rely on minimal vclock components instead of signatures Serge Petrenko 2020-04-09 23:08 ` Vladislav Shpilevoy 2020-04-10 12:25 ` Serge Petrenko 2020-04-11 16:04 ` Vladislav Shpilevoy 2020-04-13 10:12 ` Serge Petrenko 2020-04-07 15:49 ` [Tarantool-patches] [PATCH v6 3/3] box: start counting local space requests separately Serge Petrenko 2020-04-13 14:38 ` [Tarantool-patches] [PATCH v6 0/3] replication: fix local space tracking Vladislav Shpilevoy 2020-04-16 13:49 ` Kirill Yukhin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=9DB87F34-3674-4316-BA19-8DA801F4BBE9@tarantool.org \ --to=sergepetrenko@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v6 1/3] replication: omit 0-th vclock component in replication responses' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox