health: use blocking queries for near query parameter #10068
Conversation
dnephin
added
the
pr/no-changelog
github-actions
bot
added
theme/health-checks
|
🤔 This PR has changes in the |
|
Would a test be feasible? |
|
Ya, probably good to setup a table test for this. Ideally this could be tested entirely in the Testing the HTTP handler is unfortunately not as easy as it should be, because it doesn't use an interface for the backend. |
Setting this field to a value is equivalent to using the 'near' query paramter. The intent is to sort the results by proximity to the node requesting them. However with connect we send the results to envoy, which doesn't care about the order, so setting this field is increasing the work performed for no gain. It is necessary to unset this field now because we would like connect to use streaming, but streaming does not support sorting by proximity.
This will be much easier once agent/cache is removed from streaming, so putting this on hold for now.
| @@ -81,8 +81,16 @@ func (c *Client) Notify( | |||
| ch chan<- cache.UpdateEvent, | |||
| ) error { | |||
| cacheName := c.CacheName | |||
| if req.Ingress { | |||
| cacheName = c.CacheNameIngress | |||
| if req.Ingress || req.Source.Node != "" { | |||
There was a problem hiding this comment.
It's not a straight replacement, but it would be neat if you could share this potentially variable conditional line with the same line in UseStreaming to avoid drift should either one pick up another clause.
| return false | ||
| } | ||
|
|
||
| return req.QueryOptions.UseCache || req.QueryOptions.MinQueryIndex > 0 |
There was a problem hiding this comment.
why is args.QueryOptions.UseCache here? It wasn't here in the pre-PR equivalent, and the place it was present is used differently when evaluating the chain of boolean expressions.
There was a problem hiding this comment.
I realized while working on this that it will be easier to do after the change to remove agent/cache from streaming, so I marked this PR as a draft.
The goal here is to move all of the "backend selection" logic to a single place. Right now it is still split across two places. The other places (the http handler) included UseCache, as did another condition in this file above.
Once that is done I should also be able to remove the duplication you called out in the other comment.
|
This change was merged as part of #10112, because that change made it possible to test this one. |
Effectively reverts the changes in #9758
There's no reason to error here. We can still satisfy the request using blocking queries.