-
Notifications
You must be signed in to change notification settings - Fork 60
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
streams: interactive transactions and streams support #185
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the request pool and the work done. Great job!
It seems to me that the only problem is that we don't need to extend the Request
interface. I don't see any advantages of it.
I didn't look at the tests very carefully, I will do it after the correction the Request
interface.
Good news! You can rebase to the master branch. |
bb92c87
to
56c86e6
Compare
56c86e6
to
3ce90ac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the corrections!
It's not your fault but you need to sync with #180 :
- Do we need special methods like
NewStream
,NewPreparedStatement
which will return aStream
andPreparedStatement
object? Or we need a regular Begin request:
func (conn *Connection) Begin(args) (Response, error)
func (conn *Connection) BeginTyped(args) error
func (conn *Connection) BeginAsync(args) *Future
I'll agree to whatever you choose, but you have to choose the same with @vr009 .
- I think we need *Async versions of methods
Begin
,Rollback
,Commit
(not sure about *Typed). It should be synchronized with sql: support prepared statements #180Execute
andUnprepare
too. Please contact with @vr009 .
Please don't take it personally. It's just a problem of implementation a similar feature (from an API point of view) at the same time by two different people. The confusion was compounded by the request objects. It's not your fault. It's just circumstance.
a027a59
to
7379501
Compare
7379501
to
2da3ea1
Compare
You could rebase to the master branch. |
2da3ea1
to
7e5b8d0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the great work! I don't see any critical issues.
So LGTM after resolving the conversations. Also it will be greate if you split long test cases into several shortest ones.
c94d4d4
to
f456b45
Compare
6b08e7b
to
3a86a3f
Compare
So how's it ended? |
It was decided to use request objects + Connection.Do()/Stream.Do(), without specific calls. |
Unfortunately, you are unlucky and we merged a pull request with context support #173 . Most likely |
03e44f6
to
0531fcb
Compare
1968bc7
to
3c3e3b6
Compare
The main purpose of streams is transactions via iproto. Since v. 2.10.0, Tarantool supports streams and interactive transactions over them. Each stream can start its own transaction, so they allows multiplexing several transactions over one connection. API for this feature is the following: * `NewStream()` method to create a stream object for `Connection` and `NewStream(userMode Mode)` method to create a stream object for `ConnectionPool` * stream object `Stream` with `Do()` method and new request objects to work with stream, `BeginRequest` - start transaction via iproto stream; `CommitRequest` - commit transaction; `RollbackRequest` - rollback transaction. Closes #101
3c3e3b6
to
cc38aed
Compare
The main purpose of streams is transactions via iproto.
Since v. 2.10.0, Tarantool supports streams and
interactive transactions over them. Each stream
can start its own transaction, so they allows
multiplexing several transactions over one connection.
Closes #101