Connection timeouts about ginepro HOT 6 OPEN

I'm considering a new approach to the DNS timeout problem. The design looks something like the diagram below.

What I have in mind is a new DnsTimeout middleware wrapped around the LoadBalancedChannel. I'm imagining it would work like so:

• If the underlying service is not ready, it checks whether our GrpcServiceProbe currently has any discovered endpoints. If it does, the lack of readiness is because of something else and we return Pending. If it's empty, then we'll need to wait until it does have a discovered endpoint or until we reach a timeout.
• The background probe and the DnsTimeout middleware need to communicate somehow. I was planning on using a shared AtomicBool (is_empty) for this, but keen to hear other ideas.
• When DnsTimeout times out poll_ready() will return Ready(Ok) and the next call() will return a timeout error.

@conradludgate (or anyone else) what do you think?

from ginepro.

I'm going to take some time to fully digest what ginepro/tower/tonic are currently doing under the hood. Then I'll have a think about how this dnstimeout fits in

from ginepro.

Agreed!

After a few attempts of using the dead connection we can remove it and continue using the 2 remaining connections

I had to have a look, but I'm guessing this is because tonic's Reconnect will error from poll_ready() iff the connection broke and then the subsequent reconnection attempt also failed?

So, services can be removed from the balancer for one of two reasons:

1. Evicted by our GrpcServiceProbe (DNS stopped returning it).
2. Failing to become ready, most likely the reason above.

Cool. So yeah, either way we can end up with nothing to load balance across.

Of course, for DNS local to a kubernetes cluster, I would hope those DNS latencies would be low, so you could set the timeout for the DNS to be low but the timeout for the connection to be higher. Is that your understanding too?

Yep. I mean, how long DNS vs TCP + TLS vs actual requests will take can vary a lot, which is why separate timeouts are useful. E.g. we might be happy to wait for a really long request which can take 10s P99, but we wouldn't want to wait that long just for DNS or TCP + TLS. We already have (well, will soon have) a timeout for the TCP connection, so DNS seems like the last piece of the puzzle.

from ginepro.

Ok, just to confirm that I have a clear picture of what the issue is:

Let's say we have 3 connections in our balance queue, but 1 of them has gone offline. We don't need to wait for DNS in this case. After a few attempts of using the dead connection we can remove it and continue using the 2 remaining connections (not sure if tower::Balance does this. TBC).

Let's say those 2 connections go down, we now have 0 connections in our balance queue. Therefore we need to wait for DNS to get new connections. This is also the init case. When this happens, we want to timeout early waiting for DNS, which would be a different timeout from the unary request round trip.

Of course, for DNS local to a kubernetes cluster, I would hope those DNS latencies would be low, so you could set the timeout for the DNS to be low but the timeout for the connection to be higher. Is that your understanding too?

from ginepro.

After a few attempts of using the dead connection we can remove it

https://docs.rs/tower/0.4.13/tower/ready_cache/cache/struct.ReadyCache.html

If an error is returned, this indicats that the server failed and has been removed from the cache entirely.

from ginepro.

Cool. I'm all caught up then. I'll have a think about yours and some other solutions then.

In the end I think we already might have to re-implement some of tonic for #38 so I think we can move the dnstimeout further download the chain (in the Discover layer). But we'll still somehow have to surface the number of services registered to this layer

from ginepro.