From: Konstantin Osipov <kostja@tarantool.org> To: tarantool-patches@freelists.org Subject: [tarantool-patches] Re: [PATCH 6/6] swim: introduce payload Date: Thu, 18 Apr 2019 18:12:25 +0300 [thread overview] Message-ID: <20190418151225.GA13022@chai> (raw) In-Reply-To: <04b17677a6e24d46587f718733000f426e1176e7.1555021137.git.v.shpilevoy@tarantool.org> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [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
next prev parent reply other threads:[~2019-04-18 15:12 UTC|newest] Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-04-11 22:22 [tarantool-patches] [PATCH 0/6] swim payload Vladislav Shpilevoy 2019-04-11 22:22 ` [tarantool-patches] [PATCH 1/6] swim: factor out MP_BIN decoding from swim_decode_uuid Vladislav Shpilevoy 2019-04-11 23:09 ` [tarantool-patches] " Konstantin Osipov 2019-04-12 19:23 ` Vladislav Shpilevoy 2019-04-11 22:22 ` [tarantool-patches] [PATCH 2/6] swim: replace event_bin and member_bin with the passport Vladislav Shpilevoy 2019-04-11 23:10 ` [tarantool-patches] " Konstantin Osipov 2019-04-12 19:23 ` Vladislav Shpilevoy 2019-04-11 22:22 ` [tarantool-patches] [PATCH 3/6] swim: factor out 'update' part of swim_member_upsert() Vladislav Shpilevoy 2019-04-11 23:11 ` [tarantool-patches] " Konstantin Osipov 2019-04-12 19:23 ` Vladislav Shpilevoy 2019-04-11 22:22 ` [tarantool-patches] [PATCH 4/6] test: generalize SWIM fake descriptor filters Vladislav Shpilevoy 2019-04-11 23:11 ` [tarantool-patches] " Konstantin Osipov 2019-04-12 19:23 ` Vladislav Shpilevoy 2019-04-11 22:22 ` [tarantool-patches] [PATCH 5/6] test: introduce new SWIM packet filter by component names Vladislav Shpilevoy 2019-04-11 23:11 ` [tarantool-patches] " Konstantin Osipov 2019-04-12 19:23 ` Vladislav Shpilevoy 2019-04-11 22:22 ` [tarantool-patches] [PATCH 6/6] swim: introduce payload Vladislav Shpilevoy 2019-04-18 15:12 ` Konstantin Osipov [this message] 2019-04-18 17:43 ` [tarantool-patches] " Vladislav Shpilevoy 2019-04-18 18:03 ` Konstantin Osipov 2019-04-18 20:40 ` Vladislav Shpilevoy 2019-04-18 17:43 ` [tarantool-patches] [PATCH 5.5/6] swim: rename TTL to TTD Vladislav Shpilevoy 2019-04-18 17:48 ` [tarantool-patches] " Konstantin Osipov 2019-04-18 20:40 ` Vladislav Shpilevoy 2019-04-18 18:16 ` [tarantool-patches] [PATCH 7/6] swim: drop incarnation_inc parameter from update() routines Vladislav Shpilevoy 2019-04-18 18:20 ` [tarantool-patches] " Konstantin Osipov 2019-04-18 20:40 ` Vladislav Shpilevoy
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=20190418151225.GA13022@chai \ --to=kostja@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='[tarantool-patches] Re: [PATCH 6/6] swim: introduce payload' \ /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