<@U0N6C2Q9F>: i opened <https://github.com/pantsbu...
# development
@fast-nail-55400: i opened https://github.com/pantsbuild/pants/issues/16188, but there is annoying aspect to it:
that’s solveable with some fiddling. but there is also a question of the intent of the timeout: at least to me, it seems like it should only apply to network time, so that adjusting the concurrency limit doesn’t make you more or less likely to hit the timeout
on the other hand, if we do want it to apply to the entire cache lookup, we’ll likely want to increase the timeout significantly, which makes it less meaningful
having rubber ducked on this (sorry), i think that it is probably worth the effort to figure out how to apply the timeout 1) at the network level, 2) only to cache reads … as described on the ticket.
IIRC a service is a really cheap to construct wrapped around a channel? So making new ones, even per-request, should be ~free?
Yes, but they would need to share the concurrency limit.
Which... I think that you can do by supplying the semaphore to use... ?
requests in Tonic can carry extra typed data as an “extension”
then implement a Tower layer which checks for the extension for this type of timeout and apply the timeout
(assuming Tower layers can be used in such a manner to apply timeouts)
then if code wants the network-level timeout to apply, it would add this extension to the request
Yea, that seems like it would be pretty clean
if you haven’t started on this, I can work on it
i haven’t started it. but yea, that would be helpful.
i was leaning toward creating two services, one for reading and one for writing, and then having them share the
but whatever you’d like to do is fine.
what’s the benefit of having two services?
as Daniel said: they’re cheap. and it avoids the need to tag and interpret tags
but yea, whatever ends up being easiest.
thank you.
but the timeout would need to be below the concurrency limit in the stack, so I don’t see how two services changes that fact
and then having them share the
^ … i.e., use a single semaphore. iirc, there was a facility for this.
ah and each would have a different layer stack?
namely, a timeout layer in one but not the other?
yea. or they both do, but the default timeout for an unbounded stack would be very large until/unless we add configuration
tower has a
layer so good starting point (since we want metrics which it doesn’t have hooks for)
@witty-crayon-22786: I wonder if the
function in
actually makes the problem worse since it assumes that the timeout is only for network-level waiting and not the concurrency limit.
hmm maybe not, it only deals with exponential backoff for the sleep between retries, not a timeout on the actual call
https://github.com/pantsbuild/pants/pull/16196 is the draft so far. I don’t like the approach I took though in vendoring the Tower
I may rewrite to have a custom layer with the sole purpose of detecting when a timeout error is emitted.