Support IPROTO_PUSH messages #156
Conversation
oleg-jukovec
requested review from
ligurio,
AnaNek,
DifferentialOrange and
LeonidVas
|
BTW, we use 72-symbols limit for commit message body: https://www.tarantool.io/en/doc/latest/dev_guide/developer_guidelines/#how-to-write-a-commit-message |
response_it.go
Outdated
| defer it.fut.mutex.RUnlock() | ||
|
|
||
| resp = it.nextResponse() | ||
| if it.resp == nil { |
There was a problem hiding this comment.
I don't get:
- if iterator doesn't get timeout parameter
- then it doesn't wait anything
- therefore usually it doesn't get anything, because nextResponse sees empty
fut.pushesand emptyresp.
LOL
What i'm missing?
There was a problem hiding this comment.
Exactly, see Lua implementation:
https://www.tarantool.io/en/doc/latest/reference/reference_lua/box_session/push/
Lua's iterator just leave a loop if nothing to do (returns it.Next() == false in the case).
-- There is no pause because conn:call does not wait for
-- server_function to finish. The first time that we go through
-- the pairs() loop, we see the messages table is empty. Like this:
-- tarantool> messages
There was a problem hiding this comment.
Go is not Lua. It doesn't need to match Lua's code exactly.
In Go one gets notifications through channels. Channels are composable.
"Polling" on it.Next() would be definitely anti-pattern in Go.
Well... I could not came with clear api now.
It doesn't look like iterator could be efficiently implemented in Go-friendly way if there is no separate method "conn.CallStream".
There was a problem hiding this comment.
I agree that iterators are not idiomatic in Go. But we need to provide a similar interface as in Lua, which would not confuse an user. If each language implements features in its own way, it will be too difficult to translate very simple examples of code between connectors.
Maybe, but sometimes Next() is used in this context:
https://pkg.go.dev/database/sql#Rows.Next
https://pkg.go.dev/go.mongodb.org/mongo-driver/mongo#ChangeStream.Next
request.go
Outdated
| @@ -12,6 +13,9 @@ type Future struct { | |||
| requestId uint32 | |||
| requestCode int32 | |||
| timeout time.Duration | |||
| mutex sync.RWMutex | |||
| cond *sync.Cond | |||
There was a problem hiding this comment.
Instead of (ab)using sync.Cond, you'd better
// this doesn't consume more memory, than make(chan struct{})
fut.ready := make(chan struct{}, 1000000000)
And then notify about PUSH using fut.ready <- struct{}{}
This way you will need no separate goroutine to merry push notifications with timeouts and final response.
Whole iterator handling will be much simpler.
There was a problem hiding this comment.
Thank you, it's much easier.
Oh, I'm surprised. It fixed and remembered. |
There was a problem hiding this comment.
Should be LGTM after resolving remaining comments.
Please don't forget to rebase onto master if needed (for example, when #160 will be merged) and solve all possible conflicts and warnings.
oleg-jukovec
dismissed
ligurio’s stale review
ligurio removed their request for review
For me it seems ok, I only sometimes do rebase. |
This is not a critical change. It just splits request.go into request.go and future.go.
This patch adds support for receiving messages sent using box.session.push() via an iterator in the manner of asynchronous case of a Lua implementation[1]. Now the calls Future.Get() and Future.GetTyped() ignore push messages, and do not report an error. 1. https://www.tarantool.io/ru/doc/latest/reference/reference_lua/box_session/push/ Closes #67
This patch adds support for receiving messages sent using box.session.push()
via an iterator in the manner of asynchronous case of a Lua implementation[1].
Now the calls Future.Get() and Future.GetTyped() ignore push messages, and
do not report an error.
Closes #67
I'm not particularly familiar with Go. So I most likely made mistakes in some basic and idiomatic things