From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 21 Feb 2019 21:35:03 +0300 From: Konstantin Osipov Subject: Re: [tarantool-patches] [PATCH v4 04/12] [RAW] swim: introduce SWIM's anti-entropy component Message-ID: <20190221183503.GE14247@chai> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: To: tarantool-patches@freelists.org Cc: vdavydov.dev@gmail.com List-ID: * Vladislav Shpilevoy [19/01/31 10:28]: > +/** > + * Helper to do not call tt_static_buf() in all places where it is > + * wanted to get string UUID. > + */ > +static inline const char * > +swim_uuid_str(const struct tt_uuid *uuid) > +{ > + char *buf = tt_static_buf(); > + tt_uuid_to_string(uuid, buf); > + return buf; > +} I thought swim should not depend on src/. How does this work? By the same token, it should not depend on diag_* or otherwise diag* should be moved to src/lib as a separate library. > +#define mh_name _swim_table > +struct mh_swim_table_key { > + uint32_t hash; > + const struct tt_uuid *uuid; > +}; > +#define mh_key_t struct mh_swim_table_key > +#define mh_node_t struct swim_member * > +#define mh_arg_t void * > +#define mh_hash(a, arg) ((*a)->hash) > +#define mh_hash_key(a, arg) (a.hash) > +#define mh_cmp(a, b, arg) (tt_uuid_compare(&(*a)->uuid, &(*b)->uuid)) > +#define mh_cmp_key(a, b, arg) (tt_uuid_compare(a.uuid, &(*b)->uuid)) > +#define MH_SOURCE 1 > +#include "salad/mhash.h" It's OK to depend on salad. Shall we move tt_static_buf() implementation to small/ or some other lib/ library to make sure swim does not depend on src? > +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); > + */ > +static void > +swim_member_delete(struct swim *swim, struct swim_member *member) It should be subject-verb-object, so swim_delete_member(). > +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(). > +{ > + 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(). > + diag_set(OutOfMemory, sizeof(mh_int_t), "malloc", "node"); > + return NULL; > + } > + rlist_add_entry(&swim->queue_round, member, in_queue_round); > + > + say_verbose("SWIM: member %s is added", swim_uuid_str(uuid)); > + return member; This is swim_add_member(). > +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? > +/** > + * 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. If you return the number of encoded messages, and the function never fails, please use unsigned, not int. > + * 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. > +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.*. 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. -- Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 http://tarantool.io - www.twitter.com/kostja_osipov