refactor: make use of contexts in more places #1261
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1261 +/- ##
==========================================
- Coverage 93.92% 93.78% -0.14%
==========================================
Files 75 75
Lines 6041 6006 -35
==========================================
- Hits 5674 5633 -41
- Misses 284 289 +5
- Partials 83 84 +1 ☔ View full report in Codecov by Sentry. |
ThinkChaos
force-pushed
the
feat/resolve-ctx
branch
from
0dc01c3
to
18a25f5
Compare
|
Since the redis package is also heavily context based and has the somewhat worst implementation(sorry for that 🫣) would you consider also refactoring it? Or is this out of scope and should be done in a follow up PR? |
|
Yeah there's a couple other places where we should do a pass. I have more changes locally but didn't want to make a too giant PR since this is already big! |
| response, rtt, err = r.tcpClient.ExchangeContext(ctx, msg, upstreamURL) | ||
| if err != nil && r.udpClient != nil { | ||
| // try UDP as fallback | ||
| var opErr *net.OpError | ||
| if errors.As(err, &opErr) && opErr.Op == "dial" && r.udpClient != nil { | ||
| return r.udpClient.Exchange(msg, upstreamURL) | ||
| if errors.As(err, &opErr) && opErr.Op == "dial" { | ||
| return r.udpClient.ExchangeContext(ctx, msg, upstreamURL) |
There was a problem hiding this comment.
This changes the behavior: we now have a single timeout for both calls. Meaning if the first one fails because of a timeout, then the second one won't even really start because the context's deadline has already been reached.
I think the best solution would be to start both queries at the same time (or remember which protocol worked last time and use something like weighted random -- but that's more complex and more state).
That's more work than I want to do in this PR though, so I'll make an issue and get back to it.
In the mean I'll write a test to trigger that condition and do a very ugly thing: ignore the passed-in context for the second query.
There was a problem hiding this comment.
How about doubling the context timout and splitting it to different contexts for each request? 🤔
There was a problem hiding this comment.
How about doubling the context timout and splitting it to different contexts for each request? 🤔
I'd rather not mess with the timeout value the user provides if we can.
When writing the test, I realized this isn't an issue: the only case we retry is in case of an error during "dial", aka connection refused which shouldn't take long so we'll be fine.
I still think racing TCP and UDP could be worth it and will create an issue.
ThinkChaos
force-pushed
the
feat/resolve-ctx
branch
from
6d6562f
to
69fd160
Compare
- `CacheControl.FlushCaches` - `Querier.Query` - `Resolver.Resolve` Besides all the API churn, this leads to `ParallelBestResolver`, `StrictResolver` and `UpstreamResolver` simplification: timeouts only need to be setup in one place, `UpstreamResolver`. We also benefit from using HTTP request contexts, so if the client closes the connection we stop processing on our side.
ThinkChaos
force-pushed
the
feat/resolve-ctx
branch
from
be5fa5c
to
674bbce
Compare
I split the changes to make reviewing easier since most of the changes are just adding
ctxto function definitions and calls. Here's the interesting commits:ctxchangesctxstuff but did it at the same timeI'd say it's easier to read the commits mentioned above individually, and then scan through the whole lot of changes more quickly.
Progress #1264