[tarantool-patches] Re: [PATCH 3/5] swim: allow to hang triggers on member updates

[Konstantin Osipov <kostja@tarantool.org>]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/06/03 14:33]:

I like the design: elegant and lean. Some comments below.

> This commit allows to hang triggers on member updates.

"hang triggers" sounds ominous. I think you simply
meant "set triggers". See also patch subject, please change as
well.

>  	struct rlist in_dissemination_queue;
> +	/**
> +	 * Each time a member is updated, or created, or dropped,
> +	 * it is added to an update queue. Members from this queue
> +	 * are dispatched into user defined triggers.
> +	 */
> +	struct stailq_entry in_update_queue;

I think the name is a bit confusing. After all, the dissemination
queue is also an update queue. in_local_queue? in_trigger_queue?
in_event_queue? in_pending_member_update_queue? 
in_member_update? in_member_update_queue?

Please also make sure that all API names continue to agree with
each other and internal member names after the rename.

> +	/**
> +	 * Mask of events happened with this member since a
> +	 * previous trigger invocation. Once the events are
> +	 * delivered into a trigger, the mask is nullified and
> +	 * starts collecting new events.
> +	 */
> +	enum swim_ev_mask events;

Since you use 'events' for an associated mask, perhaps
in_event_queue or in_local_event_queue is good enough.
> +	/**
> +	 * Worker fiber to serve member update triggers. This task
> +	 * is being done in a separate fiber, because user
> +	 * triggers can yield and libev callbacks, processing
> +	 * member updates, are not allowed to yield.
> +	 */
> +	struct fiber *worker;

I think as long as the worker is only used for triggers, the name
should be more specific. when the worker is used for other events
or actions, it could be renamed.

> @@ -734,7 +801,6 @@ swim_delete_member(struct swim *swim, struct swim_member *member)
>  	mh_int_t rc = mh_swim_table_find(swim->members, key, NULL);
>  	assert(rc != mh_end(swim->members));
>  	mh_swim_table_del(swim->members, rc, NULL);
> -	swim_cached_round_msg_invalidate(swim);
>  	rlist_del_entry(member, in_round_queue);

Stray change? At least I don't understand it.

> +	struct swim *s = va_arg(va, struct swim *);
> +	struct swim_on_member_update_ctx ctx;
> +	while (! fiber_is_cancelled()) {
> +		while (stailq_empty(&s->update_queue)) {
> +			fiber_yield();
> +			if (fiber_is_cancelled())
> +				goto end;

> +		}

Since you use goto anyway, why a double loop?

    while (! fiber_is_cancelled()) {
        if (stailq_empty(&s->update_queue) {
            fiber_yield();
            continue;
        }
        ... 
    }
> +	}
> +end:
> +	swim_do_delete(s);

I don't mind swim_do_delete(), but I think the worker should not
be destorying the instance. Looks a breach of encapsulation, this assumes
the workers knows about all other uses of struct swim.

Why not cancel and join the worker from the destructor and then
safely do the delete?

> +	swim->worker = fiber_new("SWIM worker", swim_worker_f);
> +	if (swim->worker == NULL) {
> +		free(swim);
> +		return NULL;
> +	}

>  	swim->members = mh_swim_table_new();
>  	if (swim->members == NULL) {
> +		fiber_cancel(swim->worker);

Better to start the worker last, then you will have easy time
destroying it on failure.  This is a minor nit, ignore if you wish.

>  		free(swim);
>  		diag_set(OutOfMemory, sizeof(*swim->members),
>  			 "mh_swim_table_new", "members");
> @@ -1700,7 +1832,10 @@ swim_new(void)
> @@ -1951,17 +2088,19 @@ swim_size(const struct swim *swim)
>  	return mh_size(swim->members);
>  }
>  
>  
> +void
> +swim_delete(struct swim *swim)
> +{
> +	fiber_wakeup(swim->worker);
> +	fiber_cancel(swim->worker);
> +	fiber_sleep(0);

This looks like a hack, please cancel and join the worker instead.

> @@ -1991,7 +2139,8 @@ swim_quit_step_complete(struct swim_task *task,
>  	(void) rc;
>  	struct swim *swim = swim_by_scheduler(scheduler);
>  	if (rlist_empty(&swim->round_queue)) {
> -		swim_delete(swim);
> +		fiber_wakeup(swim->worker);
> +		fiber_cancel(swim->worker);
>  		return;

And now this code implicitly destroys the swim instance, which is
hard to follow when reading.
-- 
Konstantin Osipov, Moscow, Russia