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

[Vladimir Davydov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

On Thu, Aug 12, 2021 at 08:52:30PM +0300, Vladislav Shpilevoy wrote:
> > 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.

_request returns nil on success. I assume we want to return nothing,
similarly to box.commit().

> > +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.

It's not the command output, it's a comment :-)

> 
> 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')