Conversation
Each sub-search clears the thread-local search_cutoff before it runs, so the flag do_union reads once at the end belongs to whichever sub-search happened to go last. A union spends one time budget across all of them and reports one flag, so a sub-search that ran out of it has to be visible in that flag however the later ones fared — and a search that finds nothing its filter admits returns before it reads the clock at all, so the last one is not a reliable place to read it from. The same flag gates the 408 a union returns when the budget left it nothing, so a union could answer 200 with an empty result set and search_cutoff false after timing out.
CollectionManager::do_union handled a 408 from Collection::do_union by returning result_op, which is always ok at that point, so the multi_search handler never saw the timeout and answered 200 with an empty body. It now returns the 408. Because of that, the handler's 408 branch for a union had never run, and it neither marked the response final nor streamed it, unlike every other branch of post_multi_search. It does both now, so the client gets the 408 rather than no response at all. SearchCutoffCoversEverySubSearch expected the empty body and now expects the 408. The new core API test checks the status, the message and that the response is final.
| } | ||
|
|
||
| std::shared_ptr<http_req> req = std::make_shared<http_req>(); | ||
| std::shared_ptr<http_res> res = std::make_shared<http_res>(nullptr); |
There was a problem hiding this comment.
this uses http_res(nullptr), so stream_response() returns immediately. the test would still pas if we removed the new streaming call. could we cover the actual response delivery too?
There was a problem hiding this comment.
Restructured, and added an API-level test for this. Fixed some related bugs at the same time.
| // The first sub-search is cut off before it finds anything and the second | ||
| // has nothing its filter admits, so the union has nothing to return. It | ||
| // answers 408 only if the flag it reads says a sub-search was cut off. | ||
| ASSERT_FALSE(search_op.ok()); |
There was a problem hiding this comment.
could we also keep a pinned hit in the first search and assert search_cutoff: true when the last search matches nothing?
There was a problem hiding this comment.
Added, and restructured so the tests no longer depend on the last sub-search happening not to read the clock. Only the first sub-search has its budget spent (search_cutoff_ms: 0 in its own body); the second runs with the default budget and returns real hits. The union has to report search_cutoff: true with found exactly 1 pinned + 20 real hits.
| // Two collections for one union: the first holds documents the search has | ||
| // to work through, the second holds none that filter_by admits, so its | ||
| // search returns before it ever reads the clock. | ||
| void setupCutoffCollections() { |
There was a problem hiding this comment.
this only used once, in one test, so this can be inlined in the test itself
|
|
||
| // Every sub-search clears search_cutoff before it runs, so the flag left | ||
| // behind belongs to the last one. The union spends one budget across all of | ||
| // them and reports one flag, which has to mean any of them ran out of it. |
There was a problem hiding this comment.
Super pedantic and could be ignored, but this could be rewritten as:
// each sub-search resets search_cutoff. preserve if any search was cut off.
Add a union test where an earlier sub-search is cut off but a later one still returns hits, so the cutoff has to be carried into the response, check that a cut-off union's 408 is not served from the response cache, and add an api_tests suite that checks the same union over HTTP, including delivery of the 408. Also give 408 a reason phrase and answer any failed union with its error rather than only a 408.
|
@tharropoulos can I ask for a quick re-review? |
| Option<bool> union_op = CollectionManager::do_union(req->params, req->embedded_params_vec, searches, | ||
| response, req->conn_ts, union_remove_duplicates); | ||
| if(!union_op.ok() && union_op.code() == 408) { | ||
| if(!union_op.ok()) { |
There was a problem hiding this comment.
could we keep the original 408 condition here? do_union only returns a failed option for 408 right now, so the broader check and nested if don’t change behavior.
| } | ||
|
|
||
| describe(Phases.SINGLE_FRESH, () => { | ||
| it("create the union cutoff collections", async () => { |
There was a problem hiding this comment.
this could be changed for a beforeAll hook instead of a test
Also restore the 408-only check on a failed union in the handler, since that is the only failure do_union propagates.
2 participants
search_cutofffrom its last sub-search only, so an earlier sub-search running out of budget went unreported.CollectionManager::do_unionreturnedresult_oprather than the 408, and the handler's union 408 branch never finalized its response.