From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> To: tarantool-patches@freelists.org, Konstantin Osipov <kostja@tarantool.org> Cc: vdavydov.dev@gmail.com Subject: Re: [tarantool-patches] Re: [PATCH v4 04/12] [RAW] swim: introduce SWIM's anti-entropy component Date: Tue, 26 Feb 2019 21:28:26 +0300 [thread overview] Message-ID: <fd829d71-2507-bd5f-c700-3b5690900f37@tarantool.org> (raw) In-Reply-To: <20190221183503.GE14247@chai> Thanks for the review. >> +static inline struct swim * >> +swim_by_scheduler(struct swim_scheduler *scheduler) > > By strange convention we call such functions simply after the name > of the class, this was here even before I joined :) > > In other words: > > static inline struct swim * > swim(struct swim_scheduler *scheduler); I failed to find any examples. On the contrary, I found that usually container_of is used without a wrapper in similar cases: - memtx_engine.cc:1183 - memtx_hash.c:167, 187 - memtx_tree.c:386, 407 - relay.cc:440, 688, 701 - vinyl.c:2433, 2440, 2459, 2945 - ... many other vinyl files ... - cbus.c:443, 490, 505, 557, 575, 588 - say.c:314 So I do not think that such policy works for that case. > >> + */ >> +static void >> +swim_member_delete(struct swim *swim, struct swim_member *member) > > It should be subject-verb-object, so swim_delete_member(). It is. 'swim_member' is subject, 'delete' is verb, 'object' is omitted. Now I refactored all the code, that swim_member has only two methods: swim_member_new, swim_member_delete All other methods are owned by swim: swim_new_member, swim_delete_member, swim_wait_ack, swim_update_member_status, swim_update_member_addr, swim_update_member, swim_on_member_status_update, swim_on_member_payload_update etc ... It really looks better now. > >> +static struct swim_member * >> +swim_member_new(struct swim *swim, const struct sockaddr_in *addr, >> + const struct tt_uuid *uuid, enum swim_member_status status) > > swim_create_member() or swim_add_member(). > swim_add_member is a public API, which you approved by the way. I will use swim_new_member. 'Create' does not fit here, because this function does not take a member to initialize as a parameter. > >> +{ >> + struct swim_member *member = >> + (struct swim_member *) calloc(1, sizeof(*member)); >> + if (member == NULL) { >> + diag_set(OutOfMemory, sizeof(*member), "calloc", "member"); >> + return NULL; >> + } >> + member->status = status; >> + member->addr = *addr; >> + member->uuid = *uuid; >> + member->hash = swim_uuid_hash(uuid); >> + mh_int_t rc = mh_swim_table_put(swim->members, >> + (const struct swim_member **) &member, >> + NULL, NULL); > > This part is swim_member_new(), if you want to have > swim_member_new().> >> + if (rc == mh_end(swim->members)) { >> + free(member); > > free(member) is swim_member_delete(). Ok, done. I do not paste diff here because it is too big. >> +static struct swim_member ** >> +swim_shuffle_members(struct swim *swim) >> +{ >> + struct mh_swim_table_t *members = swim->members; >> + struct swim_member **shuffled; >> + int bsize = sizeof(shuffled[0]) * mh_size(members); >> + shuffled = (struct swim_member **) malloc(bsize); >> + if (shuffled == NULL) { >> + diag_set(OutOfMemory, bsize, "malloc", "shuffled"); >> + return NULL; >> + } > > Shan't we ensure shuffle never fails? Why do you make a copy of > the shuffled members each time, isn't it easier to allocate a > shuffled array once? If you are talking about why I do not cache the memory, then because 1) a fail does not break swim, 2) it complicates code. If you mean why do I shuffle the array after each round again - it is because what randomness is supposed to do here by the protocol definition. If I shuffle that only once, then some members can stand in the same positions on different cluster members and have not even network load during their whole lifetime. This is why the original SWIM describes eternal random selection instead of a single shuffle per lifetime. > >> +/** >> + * Encode anti-entropy header and random members data as many as >> + * possible to the end of the packet. >> + * @retval 0 Not error, but nothing is encoded. >> + * @retval 1 Something is encoded. >> + */ >> +static int >> +swim_encode_anti_entropy(struct swim *swim, struct swim_packet *packet) > > Please try to avoid flipping the meaning of the return value. 0 on > success -1 on error. It is not an error. And that function can not fail. > > If you return the number of encoded messages, I do not return the number of encoded messages. What I return stated in the comment. > and the function > never fails, please use unsigned, not int. We never use unsigned without strict necessity. We even have this issue: https://github.com/tarantool/tarantool/issues/2161. So I leave signed. > >> + * Encode source UUID. >> + * @retval 0 Not error, but nothing is encoded. >> + * @retval 1 Something is encoded. >> + */ >> +static inline int >> +swim_encode_src_uuid(struct swim *swim, struct swim_packet *packet) >> +{ >> + struct swim_src_uuid_bin uuid_bin; >> + char *pos = swim_packet_alloc(packet, sizeof(uuid_bin)); >> + if (pos == NULL) >> + return 0; > > Looks like you ignore the error because swim algorithm does not > depend on error. I believe the decision to abort the current round on > error should be made in the algorithm body, not inside nested > functions. The functions should faithfully report problems up. It is not an error. Read the code more attentive. This code fills the packet with data until it is full. If something did not fit - it is not an error. The packet is sent anyway. In all other places, where a real error occurs, it lifts up to a user, or to a libev callback, where it is reported. > >> +void >> +swim_info(struct swim *swim, struct info_handler *info) >> +{ >> + info_begin(info); >> + for (mh_int_t node = mh_first(swim->members), >> + end = mh_end(swim->members); node != end; >> + node = mh_next(swim->members, node)) { >> + struct swim_member *m = >> + *mh_swim_table_node(swim->members, node); >> + info_table_begin(info, >> + sio_strfaddr((struct sockaddr *) &m->addr, >> + sizeof(m->addr))); >> + info_append_str(info, "status", >> + swim_member_status_strs[m->status]); >> + info_append_str(info, "uuid", swim_uuid_str(&m->uuid)); >> + info_table_end(info); >> + } >> + info_end(info); >> +} > > Ouch, swim_info() ties swim together with info.*. 'Info' is moved into src/lib in a separate patchset. > > All this together calls for leaving the core swim features in > src/lib/swim and having a wrapper in src/ which adds diag* and > info* > >> +static void >> +swim_scheduler_on_output(struct ev_loop *loop, struct ev_io *io, int events); > > I once again encourage you to consider merging scheduler and IO > components. Let's look at your test harness to see how this is > feasible. As discussed verbally, it still makes no sense to merge them. > > > -- > Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 > http://tarantool.io - www.twitter.com/kostja_osipov > Despite the fixes, the commit is not ready. I still work on tests.
next prev parent reply other threads:[~2019-02-26 18:28 UTC|newest] Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-01-30 21:28 [PATCH v4 00/12] SWIM draft Vladislav Shpilevoy 2019-01-30 21:28 ` [PATCH v4 01/12] sio: introduce sio_uri_to_addr Vladislav Shpilevoy 2019-02-15 13:21 ` [tarantool-patches] " Konstantin Osipov 2019-02-15 21:22 ` [tarantool-patches] " Vladislav Shpilevoy 2019-01-30 21:28 ` [PATCH v4 10/12] [RAW] swim: introduce 'quit' message Vladislav Shpilevoy 2019-02-21 12:13 ` [tarantool-patches] " Vladislav Shpilevoy 2019-01-30 21:28 ` [PATCH v4 11/12] [RAW] swim: introduce broadcast tasks Vladislav Shpilevoy 2019-01-30 21:28 ` [PATCH v4 12/12] [RAW] swim: allow to use broadcast tasks to send pings Vladislav Shpilevoy 2019-01-30 21:28 ` [PATCH v4 02/12] evio: expose evio_setsockopt_server function Vladislav Shpilevoy 2019-02-15 13:21 ` [tarantool-patches] " Konstantin Osipov 2019-02-15 21:22 ` [tarantool-patches] " Vladislav Shpilevoy 2019-01-30 21:28 ` [PATCH v4 03/12] rlist: introduce rlist_add_tail_entry_sorted Vladislav Shpilevoy 2019-02-15 13:26 ` [tarantool-patches] " Konstantin Osipov 2019-02-15 13:34 ` [tarantool-patches] " Vladislav Shpilevoy 2019-02-15 18:07 ` Konstantin Osipov 2019-01-30 21:28 ` [PATCH v4 04/12] [RAW] swim: introduce SWIM's anti-entropy component Vladislav Shpilevoy 2019-02-21 18:35 ` [tarantool-patches] " Konstantin Osipov 2019-02-26 18:28 ` Vladislav Shpilevoy [this message] 2019-01-30 21:28 ` [PATCH v4 05/12] [RAW] swim: introduce failure detection component Vladislav Shpilevoy 2019-01-30 21:28 ` [PATCH v4 06/12] [RAW] swim: introduce dissemination component Vladislav Shpilevoy 2019-01-30 21:28 ` [PATCH v4 07/12] [RAW] swim: keep encoded round message cached Vladislav Shpilevoy 2019-01-30 21:28 ` [PATCH v4 08/12] [RAW] swim: introduce payload Vladislav Shpilevoy 2019-01-30 21:28 ` [PATCH v4 09/12] [RAW] swim: introduce routing 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=fd829d71-2507-bd5f-c700-3b5690900f37@tarantool.org \ --to=v.shpilevoy@tarantool.org \ --cc=kostja@tarantool.org \ --cc=tarantool-patches@freelists.org \ --cc=vdavydov.dev@gmail.com \ --subject='Re: [tarantool-patches] Re: [PATCH v4 04/12] [RAW] swim: introduce SWIM'\''s anti-entropy component' \ /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