Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug-1942406: Fix search_phase_execution_exception: Result window is too large #6867

Merged
merged 1 commit into from
Jan 21, 2025

Conversation

relud
Copy link
Member

@relud relud commented Jan 17, 2025

In ES 8+ index.max_result_window defaults to 10,000: https://www.elastic.co/guide/en/elasticsearch/reference/8.17/index-modules.html

@relud relud requested a review from a team as a code owner January 17, 2025 23:48
@relud relud requested a review from willkg January 17, 2025 23:48
@willkg willkg self-assigned this Jan 21, 2025
Copy link
Contributor

@willkg willkg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bummer! This has some pretty big consequences. For example, this prevents us from getting more than 10,000 crash ids for a given search. Anecdotally, I've definitely done this before and the symbol scraping scripts have, too. We have no metrics to tell us how often it comes up.

I think we should land this to keep moving forward, but we're going to need to do follow-up work and this should get tracked:

  1. add some documentation to the Supersearch API about breaking up search requests by date range to get around the max result size issue
  2. improve crashstats-tools somehow to break up search requests by date range
  3. change the SuperSearch API to support scrolling and then change whatever needs to be changed in the webapp to use scrolling instead of paging

Maybe there are other things we can/should do, too?

@relud
Copy link
Member Author

relud commented Jan 21, 2025

I'll merge this as-is to reduce error volume in stage, and file a follow-up bug to consider using the search-after feature to get deeper results

edit: https://bugzilla.mozilla.org/show_bug.cgi?id=1942885

@relud relud added this pull request to the merge queue Jan 21, 2025
@willkg
Copy link
Contributor

willkg commented Jan 21, 2025

Oo--search after is probably a better idea. +1 to that.

Merged via the queue into main with commit 259e802 Jan 21, 2025
1 check passed
@relud relud deleted the relud-bug-1942406-max-result-window branch January 21, 2025 18:19
@biancadanforth
Copy link
Contributor

Interesting... Was this not a problem for ES 1.4? Doesn't look like this specific setting exists in 1.4: https://www.elastic.co/guide/en/elasticsearch/reference/1.4/index-modules.html

If there is no such limit in 1.4, I imagine a large query would use a bunch of heap memory and run slowly? Would increasing the default value of index.max_result_window also be an option? I don't see that config in https://www.elastic.co/guide/en/cloud/current/ec-add-user-settings.html#ec-es-elasticsearch-settings, so perhaps not...

@relud
Copy link
Member Author

relud commented Jan 21, 2025

increasing the limit is an option, 10,000 is only the default, but the docs indicating an explicit solution for this problem suggests the performance of increasing the limit is subpar and we should do things the "standard" way if we can

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants