Improve search job placer logging #5514
Conversation
guilload
force-pushed
the
guilload/search-job-placer-logging
branch
from
93d3915 to
36b0fa8
Compare
guilload
force-pushed
the
guilload/search-job-placer-logging
branch
from
36b0fa8 to
941c43b
Compare
| cluster" | ||
| ); | ||
| } | ||
| if !excluded_addrs.is_empty() && excluded_addrs.len() < all_nodes.len() { |
There was a problem hiding this comment.
The condition excluded_addrs.len() < all_nodes.len() seems broken.
if there are more excluded addresses than nodes, we don't filter anything
The tests below don't cover excluded_addrs
There was a problem hiding this comment.
this seems to be on purpose. Per function doc:
/// When exclude_addresses filters all clients it is ignored.
There was a problem hiding this comment.
When excluded_addrs.len() >= all_nodes.len(), we will likely filter out all the nodes, so it's probably better not to exclude any node. I assume that was the original intent of whoever wrote this code in the first place.
Another grief I have with excluded_addrs.len() < all_nodes.len(), is that if most of the nodes are excluded, then very few candidates will remain.
Anyway, the purpose of this PR is merely to improve logging. I don't plan on fixing the exclusion logic here. @PSeitz, feel free to open an issue and suggest ways to improve the search retry logic in general. I'd be curious to know how our other systems do it.
Description
Per title. Fixes #5507.
How was this PR tested?
Ran
quickwit-searchunit tests.