Re: [tarantool-patches] [PATCH v4 04/12] [RAW] swim: introduce SWIM's anti-entropy component

[Konstantin Osipov <kostja@tarantool.org>]

* Vladislav Shpilevoy <v.shpilevoy@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