Re: [tarantool-patches] [PATCH 1/3] Applier gets rid of a xstream

[Vladimir Davydov <vdavydov.dev@gmail.com>]

On Sun, Mar 03, 2019 at 11:26:16PM +0300, Georgy Kirichenko wrote:
> diff --git a/src/box/applier.h b/src/box/applier.h
> index d942b6fbb..5bff90031 100644
> --- a/src/box/applier.h
> +++ b/src/box/applier.h
> @@ -305,28 +301,29 @@ recovery_journal_create(struct recovery_journal *journal, struct vclock *v)
>  	journal->vclock = v;
>  }
>  
> -static inline void
> -apply_row(struct xstream *stream, struct xrow_header *row)
> +int
> +apply_row(struct xrow_header *row)
>  {
> -	(void) stream;
>  	struct request request;
>  	xrow_decode_dml_xc(row, &request, dml_request_key_map(row->type));

This function may throw an exception while apply_row protocol forbids
that.

>  	if (request.type == IPROTO_NOP) {
>  		if (process_nop(&request) != 0)
> -			diag_raise();
> -		return;
> +			return -1;
> +		return 0;
>  	}
>  	struct space *space = space_cache_find_xc(request.space_id);

Ditto.

>  	if (box_process_rw(&request, space, NULL) != 0) {
>  		say_error("error applying row: %s", request_str(&request));
> -		diag_raise();
> +		return -1;
>  	}
> +	return 0;
>  }
>  
>  static void
>  apply_wal_row(struct xstream *stream, struct xrow_header *row)
>  {
> -	apply_row(stream, row);
> +	if (apply_row(row) != 0)
> +		diag_raise();
>  
>  	struct wal_stream *xstream =
>  		container_of(stream, struct wal_stream, base);
> @@ -352,15 +349,14 @@ wal_stream_create(struct wal_stream *ctx, size_t wal_max_rows)
>  	ctx->yield = (wal_max_rows >> 4)  + 1;
>  }
>  
> -static void
> -apply_initial_join_row(struct xstream *stream, struct xrow_header *row)
> +int
> +apply_initial_join_row(struct xrow_header *row)
>  {
> -	(void) stream;
>  	struct request request;
>  	xrow_decode_dml_xc(row, &request, dml_request_key_map(row->type));
>  	struct space *space = space_cache_find_xc(request.space_id);

Ditto.

Anyway, I don't think it's worth removing exceptions in the scope of
this patch. Since applier is written in C++, let's let them be for now -
we'll remove them later when we convert applier to pure C.

>  	/* no access checks here - applier always works with admin privs */
> -	space_apply_initial_join_row_xc(space, &request);
> +	return space_apply_initial_join_row(space, &request);

I agree with Kostja that it's worth moving apply_initial_join_row and
apply_row to applier.cc rather than exporting them, because they are
only used by appliers (names match, too).

As for apply_wal_row, I think we should leave it in box.cc and implement
it without the help of apply_row, because those two functions are
essentially different and the difference will only grow over time. E.g.
we don't need to handle NOP requests when recovering from WAL, we can
simply skip those. And when transaction boundaries are introduced, we
won't need to handle them on local recovery, either.

Let's please do that in the scope of this patch.