quickwit-oss / quickwit-oss/quickwit

Fix the leaf search retry logic

Open
#3,650 0 comments 0 reactions 1 assignee View on GitHub

@trinity-1686a is already working on this.

Since Jul 17, 2023.

bug
Dominant language
Rust
Stars
11.7k
Forks
597
Avg merge
2d 22h
Merged PRs (30d)
37

Description

Investigating our leaf search retry logic, I found several bugs, code efficiency, and logical problems.
This calls for a bit of refactoring. The refactoring may overlap with some of the work from @trinity-1686a.

The problem

Problem 1: redudant merge code

Currently, cluster_client::leaf_search includes some retry logic.
As a result it includes its own merge logic that is independent from the root code... and also a little bit incorrect.
For instance, partial hits are simply concatenated. Downstream if some code assumed partial hits to never exceed K where K is the total size this could lead to problems.

Problem 2: merge logic happens in the tokio runtime.

The retry merging logic is happening on the tokio runtime. For large aggregation this could be problematic.

Problem 3: Strange handlind of errors.

Our handling of errors is a little bit odd.
Let's assume two initial leaf request initiated.
Assuming both return partial results, the outcome is different depending on where the error are happening.

For instance, if we have

  • req1: initial fail, retry success and 10 missing split.
  • req2: initial fail, retry success and 10 missing split.
    Both cluster.leaf_search calls will be considered successful and the root search will merge the results, ending with 2 missing splts. Currently a check downstream returns an error if we have >0 missing splits.
    This downstream check will shuld be removed (see below).

By contrast,

  • req1: initial fail, retry fail and 1 missing split.
  • req2: initial success and missing 0 split.

Will result in req1 returning an error, and the whole cluster logic failing.

Problem 4: Ignoring retryability.

We cuirrently ignore the retryability declared in the leaf search request.

Problem 5: Behavior on partial failure

For most usage, if a few splits are missing, it woudl be better to return a list of results computed from
a partial list of splits, and return the list of failed splits.
We could have this behavior declared as a query string parameter for instance.

Problem 6: Suboptimal retry requests. (less important)

If the different leaf request end up spawning several retry requests, we will reemit as many retry requests, only using the first failed split as information for placement.

We lose here an opportunity for placing these retry requests in a smarter way..
In fact, just grouping them is a large opportunity.

If two retry requests end up targetting the same node, that node may have several optimization opportunities, in aggregation/in filtering search for instance.


Problem 7: Retry results do not benefit from some optimization. (for instance, topK when sorted by date) (optional)


Solution suggestion:

We should remove the retry logic from the cluster client.
Instead, the root node should be in charge of the retry logic.

Still we want retry to happen soon after as we receive the failed leaf results.
(We may want to wait a little to net some of these retries, but probably not wait for all leaf requests to terminate).

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.