[Tarantool-patches] [PATCH v6 1/3] replication: omit 0-th vclock component in replication responses

Serge Petrenko sergepetrenko at tarantool.org
Fri Apr 10 15:25:06 MSK 2020


> 10 апр. 2020 г., в 02:08, Vladislav Shpilevoy <v.shpilevoy at 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 at tarantool.org


More information about the Tarantool-patches mailing list