[tarantool-patches] Re: [PATCH v2 4/4] replication: use wal memory buffer to fetch rows

[Konstantin Osipov]

* Georgy Kirichenko <georgy at tarantool.org> [19/09/18 12:39]:
> +int
> +wal_relay(struct wal_relay *wal_relay, struct vclock *vclock,
> +	  wal_relay_cb on_wal_relay, void *cb_data, const char *endpoint_name)
> +{
> +	wal_relay->vclock = vclock;
> +	wal_relay->on_wal_relay = on_wal_relay;
> +	wal_relay->cb_data = cb_data;
> +	diag_create(&wal_relay->diag);
> +	wal_relay->cancel_msg.route = NULL;
> +
> +	fiber_cond_create(&wal_relay->done_cond);
> +	wal_relay->done = false;
> +
> +	/* Establish a connection with wal thread. */
> +	cbus_pair("wal", endpoint_name, &wal_relay->wal_pipe,
> +		  &wal_relay->relay_pipe,
> +		  wal_relay_attach, wal_relay, cbus_process);
> +
> +	while (!wal_relay->done) {
> +		if (fiber_is_cancelled() &&
> +		    wal_relay->cancel_msg.route == NULL) {
> +			/* Send a cancel message to a wal relay fiber. */
> +			static struct cmsg_hop cancel_route[]= {
> +				{wal_relay_cancel, NULL}};
> +			cmsg_init(&wal_relay->cancel_msg, cancel_route);
> +			cpipe_push(&wal_relay->wal_pipe, &wal_relay->cancel_msg);
> +		}
> +		fiber_cond_wait(&wal_relay->done_cond);
> +	}
> +
> +	/* Disconnect from wal thread. */
> +	cbus_unpair(&wal_relay->wal_pipe, &wal_relay->relay_pipe,
> +		    NULL, NULL, cbus_process);
> +
> +	if (!diag_is_empty(&wal_relay->diag)) {
> +		diag_move(&wal_relay->diag, &fiber()->diag);
> +		return -1;
> +	}
> +	if (fiber_is_cancelled()) {
> +		diag_set(FiberIsCancelled);
> +		return -1;
> +	}
> +	return 0;

Very, very, nice. This hunk made me think, why do you create a new
cbus pair each time? While this puts a very nice stress on the
pairing code, which few people understand and even some suspect to
be buggy :), isn't it easier to pair when creating the relay and unpair
on destroy? What happens when relay is stopped while it's
pairing? I see that you added pthread_setcancelstate(), but there
are other reasons a relay may stop, not just cancel. Sounds like
we need a clear protocol/API for stopping a paired thread, e.g.
document that before a cord exits, it must unpair all pipes first.
This can be done by keeping a list of all pairings in the cord and
asserting that the list is empty in the end of cord function.
Thoughts?

-- 
Konstantin Osipov, Moscow, Russia