uber / tchannel

network multiplexing and framing protocol for RPC
MIT License
1.15k stars 129 forks source link

Proposal: streaming bit field #1402

Closed Raynos closed 8 years ago

Raynos commented 8 years ago

To improve our timeout implementations I would like to add an "is streaming" bit flag so we can improve the timeout semantics in Hyperbahn.

I'll be sending a broader document to define the semantics of Timeouts in more edge cases but getting this PR out to add the new bit field to the protocol as it's slightly orthogonal.

i.e. we can start sending this bit field before we fix the Timeouts.

r: @jcorbin @prashantv @mranney

cc: @junchaowu @blampe @kriskowal

prashantv commented 8 years ago

lgtm

jcorbin commented 8 years ago
Raynos commented 8 years ago

What's the use case for the total stream timeout ? What are the semantics.

Streams sound like virtual sockets to me. Is it similar to a socket timeout ? That might have value. Do we want a total socket timeout ?

HelloGrayson commented 8 years ago

:+1:

jcorbin commented 8 years ago

when using a stream to implement a transaction, I could see wanted an overall timeout... however that may be better left to the application developer, let's not add the header just yet.

kriskowal commented 8 years ago

Timeout should have the same semantics for streaming and non-streaming requests: the time from first request frame to the last response frame. However, there should be a notion of an indefinitely streaming request, or simply an enormous timeout. Liveliness is a separate concern, and would have independent values for request streams and response streams.

For large requests, pagination is an idiomatic option for breaking large responses into chunks in the confines of the ttl/retry approach to RPC.

Raynos commented 8 years ago

Correct. the "notion of an indefinitely streaming request" is exactly this bit flag.

The semantics of timeouts (which must always be <2 minutes) when this bit flag is set is to fail the request if the first call response frame is not returned within the ttl.

kriskowal commented 8 years ago

I’ve posted some recommended prose changes. Otherwise, I buy the approach. The streaming bit means indefinite. It does not disable the ttl, but does relax it in a way that implies handlers have to at least ack immediately.

kriskowal commented 8 years ago

@jcorbin To your point, perhaps this timeout behavior is orthogonal to whether the request is streaming. Perhaps we should rename the flag "nottl" or "ttlack" and let it be used to relax the timeout for some but not all streaming requests. The existing semantics are sufficient for streaming requests with a long timeout for completion.

Raynos commented 8 years ago

I've addressed all feedback.