From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 828F52C1AB for ; Thu, 18 Apr 2019 11:12:28 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id TBGMT1Sx5I4w for ; Thu, 18 Apr 2019 11:12:28 -0400 (EDT) Received: from smtp53.i.mail.ru (smtp53.i.mail.ru [94.100.177.113]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id CC52F2BC3C for ; Thu, 18 Apr 2019 11:12:27 -0400 (EDT) Received: by smtp53.i.mail.ru with esmtpa (envelope-from ) id 1hH8i9-0006Y1-J4 for tarantool-patches@freelists.org; Thu, 18 Apr 2019 18:12:25 +0300 Date: Thu, 18 Apr 2019 18:12:25 +0300 From: Konstantin Osipov Subject: [tarantool-patches] Re: [PATCH 6/6] swim: introduce payload Message-ID: <20190418151225.GA13022@chai> References: <04b17677a6e24d46587f718733000f426e1176e7.1555021137.git.v.shpilevoy@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <04b17677a6e24d46587f718733000f426e1176e7.1555021137.git.v.shpilevoy@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org * Vladislav Shpilevoy [19/04/12 01:25]: > Payload is arbitrary user data disseminated over the cluster > along with other member attributes. Please see my comments inline. > Part of #3234 > --- > src/lib/swim/swim.c | 155 ++++++++++++++++++++++++++-- > src/lib/swim/swim.h | 8 ++ > src/lib/swim/swim_proto.c | 31 +++++- > src/lib/swim/swim_proto.h | 41 +++++++- > test/unit/swim.c | 195 +++++++++++++++++++++++++++++++++++- > test/unit/swim.result | 32 +++++- > test/unit/swim_test_utils.c | 62 ++++++++++++ > test/unit/swim_test_utils.h | 18 ++++ > 8 files changed, 525 insertions(+), 17 deletions(-) > > diff --git a/src/lib/swim/swim.c b/src/lib/swim/swim.c > index 2dac6eedd..2be1c846b 100644 > --- a/src/lib/swim/swim.c > +++ b/src/lib/swim/swim.c > @@ -308,6 +308,38 @@ struct swim_member { > * allow other members to learn the dead status. > */ > int status_ttl; > + /** Arbitrary user data, disseminated on each change. */ > + char *payload; > + /** Payload size, in bytes. */ > + uint16_t payload_size; > + /** > + * True, if the payload is thought to be of the most > + * actual version. In such a case it can be disseminated > + * farther. Otherwise @a payload is suspected to be farther -> further. > + * outdated and can be updated in two cases only: > + * > + * 1) when it is received with a bigger incarnation from > + * anywhere; > + * > + * 2) when it is received with the same incarnation, but > + * local payload is outdated. > + * > + * A payload can become outdated, if anyhow a new > + * incarnation of the member has been learned, but not a > + * new payload. Please describe how this can happen: the payload message is lost, but the server with the new payload responeded to ping request, and a new incarnation arrived along with the ack. > In such a case it can't be said exactly Nit: in such a case is very rarely used. Why not simply: in this case? > + * whether the member has updated payload, or another > + * attribute. The only way here is to wait until the most > + * actual payload will be received from another instance. > + * Note, that such an instance always exists - the payload > + * originator instance. > + */ > + bool is_payload_up_to_date; > + /** > + * TTL of payload. At most this number of times payload is > + * sent as a part of dissemination component. Reset on > + * each payload update. > + */ > + int payload_ttl; As agreed, let's rename ttl to ttd across the board and update the comments to say that ttd is time to disseminate. > +/** Update member's payload, register a corresponding event. */ > +static inline int > +swim_update_member_payload(struct swim *swim, struct swim_member *member, > + const char *payload, uint16_t payload_size, > + int incarnation_increment) Passing incarnation_increment raises a lot of questions when reading the code. I would not use this function from the constructor - I don't see any issue in copy-pasting 3 lines of code into the constructor to make both branches simpler. Or I would ban passing payload to the constructor and make this function public. > static int > swim_encode_member(struct swim_packet *packet, struct swim_member *m, > - struct swim_passport_bin *passport) > + struct swim_passport_bin *passport, > + struct swim_member_payload_bin *payload_header, > + bool is_payload_needed) Please rename is_payload_needed -> encode_payload. There is no comment for encode_payload and how it is used. I don't see why you can't move all the decision making about whether to encode the payload or not into this function. The payload should be encoded if: - its not null - payload ttl is greater than zero - it's not trustworthy. (is_payload_up_to_date). I would btw rename is_payload_up_to_date to is_payload_trustworthy - this name would be closer to truth. Why can't you look at these conditions once in swim_encode_member rather than evaluate them outside this function? BTW, there is no harm in encoding an empty payload all the time (e.g. mp_bin of size 0). This would make the code simpler - you would only need to look at payload_size to see if payload exists. > +int > +swim_set_payload(struct swim *swim, const char *payload, uint16_t payload_size) > +{ > + if (payload_size > MAX_PAYLOAD_SIZE) { > + diag_set(IllegalParams, "Payload should be <= %d", > + MAX_PAYLOAD_SIZE); > + return -1; > + } > + return swim_update_member_payload(swim, swim->self, payload, > + payload_size, 1); > +} Like I said above, if there is swim_set_paylaod, there is no need to supply payload in swim_new_member(). > + def->payload_size = -1; Ugh. -- Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 http://tarantool.io - www.twitter.com/kostja_osipov