Don't ignore errors on query status API #1489
Conversation
We may be observing issues trying to run Hybrid at scale that are not visible to us due to the nature how query status handles those. If error is not a mismatch, it is just silently ignored. This could lead to a variety of problems - most importantly it will mask the failures and made other shards think that everything is OK, and they can proceed with finalizing results. This PR changes this and makes this API return an error if there is at least one error that is not a status mismatch. If all of them are, the behaviour is the same as before
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1489 +/- ##
==========================================
+ Coverage 93.06% 93.07% +0.01%
==========================================
Files 239 237 -2
Lines 43546 43509 -37
==========================================
- Hits 40526 40496 -30
+ Misses 3020 3013 -7 ☔ View full report in Codecov by Sentry. |
|
yeah, this seems like a good thing to fix |
| .into_iter() | ||
| .filter_map(|(_si, e)| Self::get_state_from_error(e)) | ||
| .collect(); | ||
| status = states.into_iter().fold(status, min_status); |
There was a problem hiding this comment.
for my education, why does this cause errors to be ignored? What is a minimum status and what would folding an iterator of failures even do?
There was a problem hiding this comment.
Yea it is a bit subtle here. filter_map filters out all errors that are not status mismatch error. The closure maps them to None and filter_map excludes them
|
Dont we need to handle this error in the RC loop in some different way? Are all error non-recoverable? |
We may be observing issues trying to run Hybrid at scale that are not visible to us due to the nature how query status handles those. If error is not a mismatch, it is just silently ignored. This could lead to a variety of problems - most importantly it will mask the failures and made other shards think that everything is OK, and they can proceed with finalizing results.
This PR changes this and makes this API return an error if there is at least one error that is not a status mismatch. If all of them are, the behaviour is the same as before