[tarantool-patches] [PATCH v4 04/12] [RAW] swim: introduce SWIM's anti-entropy component
Konstantin Osipov
kostja at tarantool.org
Thu Feb 21 21:35:03 MSK 2019
* Vladislav Shpilevoy <v.shpilevoy at tarantool.org> [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
More information about the Tarantool-patches
mailing list