tarantool / go-tarantool

Tarantool 1.10+ client for Go language
https://pkg.go.dev/github.com/tarantool/go-tarantool/v2
BSD 2-Clause "Simplified" License
180 stars 58 forks source link

Do not use box.session.push #324

Open locker opened 1 year ago

locker commented 1 year ago

box.session.push is deprecated starting from Tarantool 3.0. We're going to disable it in Tarantool 4.0 with the ability to enable it via the compat.box_session_push_deprecation option. In Tarantool 5.0 we're going to drop it completely. See https://github.com/tarantool/tarantool/issues/8802.

Tarantool integration CI reports a failure if box.session.push is disabled by setting compat.box_session_push_deprecation to 'new'. Please remove all usages of box.session.push from this project code.

oleg-jukovec commented 1 year ago

@locker @unera do you want to completely remove the feature from the connector or leave a user the ability to use the feature for Tarantool < 5.0?

We want to make a next major release of the connector soon, so we can don't worry about a backward compatibility here from our side.

locker commented 1 year ago

According to @unera, the box.session.push feature is dangerous so it should be dropped completely.

cyanide-burnout commented 1 year ago

Too bad. Box.watch() is not an alternative when we use C connector

unera commented 1 year ago

We have a mechanism, that can not be repaired (without protocol refactoring).

More details are described by @locker, here

PS: I looked through the tarantool-centrifuge code. It uses session.push as a data stream, so it is an example of OOM problem. If tarantool has too many slow clients, it can be killed by OOM.

Yes, watch provides only state events, but now tarantool has no unbroken stream mechanisms.

It's a shame but we want to avoid new users at the broken feature. We have a plan to provide a new data-stream mechanism.

FZambia commented 1 year ago

PS: I looked through the tarantool-centrifuge code. It uses session.push as a data stream, so it is an example of OOM problem. If tarantool has too many slow clients, it can be killed by OOM.

Hello, killed by OOM due to unbounded buffer growth right? it's possible to add limits (on application level or Tarantool level probably) I suppose to drop slow connections or drop messages. We were planning to do this at some point. I suppose any alternative streaming mechanism will have similar trade-offs in terms of limits to prevent OOM due to slow connections?

FZambia commented 1 year ago

I'd also like to mention the real-life use case of box.session.push we have - https://centrifugal.dev/blog/2023/08/29/using-centrifugo-in-rabbitx. Nine months in production with zero issues. So I'd like to ask once again - please do not remove the feature without attempt to improve it (which seems possible), maybe properly document it, or providing a good alternative to it. We can replace push with polling - but it will introduce unnecessary latencies.

Any solution is a trade-off, if box.session.push does not introduce global problems for Tarantool protocol/architecture - it has its use cases. Redis PUB/SUB is widely used - it's built on the same principles I believe and nobody want it to be removed.

unera commented 1 year ago

it's possible to add limits

Sorry, but it isn't possible. We analysed the problem and found that it's impossible without protocol modifications. The problem can be moved from server to client, but can not be solved.

unera commented 1 year ago

Nine months in production with zero issues.

I checked the code. You have polling variant, so it wouldn't heavy to use WATCH variant.

PS: zero issues - because You have too many megabytes of RAM and too small slow clients.

FZambia commented 1 year ago

Sorry, but it isn't possible. We analysed the problem and found that it's impossible without protocol modifications. The problem can be moved from server to client, but can not be solved.

Thanks for your answer!

Is it possible to read the analysis somewhere? It's not clear what is the actual problem and why it can't be solved by introducing limits. As I showed there are uses cases where pushing to session seems totally valid. I suppose a wise planning given the number of conns, data size, etc and considering RAM limits on the machine is also a concern for general Tarantool spaces, not sure why RAM spent on box.session.push is special.

I checked the code. You have polling variant, so it wouldn't heavy to use WATCH variant.

What does it mean to use WATCH? Could you show some example? From what I've seen watch is delivering only the latest message, so if the idea is to load messages upon notification - then I suppose the buffer on Tarantool side should be still limited somehow, and we get the same problem. And increased latency.

oleg-jukovec commented 11 months ago

This feature will be removed in the very distant future.