Re: [Tarantool-patches] [PATCH v4 9/9] net.box: add interactive transaction support in net.box

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thanks for the patch!

See 3 comments below.

> diff --git a/changelogs/unreleased/gh-5860-implement-streams-in-iproto.md b/changelogs/unreleased/gh-5860-implement-streams-in-iproto.md
> new file mode 100644
> index 000000000..8a8eec3e7
> --- /dev/null
> +++ b/changelogs/unreleased/gh-5860-implement-streams-in-iproto.md
> @@ -0,0 +1,26 @@
> +## feature/core
> +
> +* Streams and interactive transactions over streams are implemented
> +  in iproto. Stream is associated with it's ID, which is unique within
> +  one connection. All requests with same not zero stream ID belongs to
> +  the same stream. All requests in stream processed synchronously. The
> +  execution of the next request will not start until the previous one is
> +  completed. If request has zero stream ID it does not belong to stream
> +  and is processed in the old way.
> +  In `net.box`, stream is an object above connection that has the same
> +  methods, but allows to execute requests sequentially. ID is generated
> +  on the client side automatically. If user writes his own connector and
> +  wants to use streams, he must transmit stream_id over iproto protocol.
> +  The main purpose of streams is transactions via iproto. Each stream
> +  can start its own transaction, so they allows multiplexing several
> +  transactions over one connection. There are multiple ways to begin,
> +  commit and rollback transaction: using appropriate stream methods, using
> +  `call` or `eval` methods or using `execute` method with sql transaction
> +  syntax. User can mix these methods, for example, start transaction using
> +  `stream:begin()`, and commit transaction using `stream:call('box.commit')`
> +  or stream:execute('COMMIT').
> +  If any request fails during the transaction, it will not affect the other
> +  requests in the transaction. If disconnect occurs when there is some active
> +  transaction in stream, this transaction will be rollbacked, if it does not
> +  have time to commit before this moment.

1. Please, add a reference to the ticket in the end in the form `(gh-####)`.

> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> index 8d707fb26..f203b203e 100644
> --- a/src/box/lua/net_box.lua
> +++ b/src/box/lua/net_box.lua
> @@ -754,11 +757,38 @@ local function stream_new_stream(stream)
>      return stream._conn:new_stream()
>  end
>  
> +local function stream_begin(stream, opts)
> +    check_remote_arg(stream, 'begin')
> +    local res = stream:_request(M_BEGIN, opts, nil, stream._stream_id)
> +    if opts and opts.is_async then
> +        return res
> +    end
> +end
> +
> +local function stream_commit(stream, opts)
> +    check_remote_arg(stream, 'commit')
> +    local res = stream:_request(M_COMMIT, opts, nil, stream._stream_id)
> +    if opts and opts.is_async then
> +        return res
> +    end

2. Why can't you just return the result of :_request()? Isn't it
supposed to return the correct thing right away? For example,
remote_methods:execute() does it, space methods too.

> +end
> +
> +local function stream_rollback(stream, opts)
> +    check_remote_arg(stream, 'rollback')
> +    local res = stream:_request(M_ROLLBACK, opts, nil, stream._stream_id)
> +    if opts and opts.is_async then
> +        return res
> +    end
> +end
> diff --git a/test/box/net.box_iproto_transactions_over_streams.result b/test/box/net.box_iproto_transactions_over_streams.result
> new file mode 100644
> index 000000000..c2167e760
> --- /dev/null
> +++ b/test/box/net.box_iproto_transactions_over_streams.result

<...>

> +---
> +...
> +-- successful begin using stream:call
> +stream:call('box.begin')
> +---
> +...
> +-- error: Operation is not permitted when there is an active transaction

3. Well, it does not look very successful like you said in
the comment. The test seems broken.

I didn't validate the others since I have very few time for that.
I hope Vova will take a closer look at the tests now despite he
has given LGTM already.

(Or am I missing something and the test actually succeeds with
this weird output on purpose?)

> +stream:eval('box.begin()')
> +---
> +- error: 'Operation is not permitted when there is an active transaction '
> +...
> +-- error: Operation is not permitted when there is an active transaction
> +stream:begin()
> +---
> +- error: 'Operation is not permitted when there is an active transaction '
> +...
> +-- error: Operation is not permitted when there is an active transaction
> +stream:execute('START TRANSACTION')
> +---
> +- error: 'Operation is not permitted when there is an active transaction '
> +...
> +stream:call('ping')