Skip to content

Improved search timeout checking capabilities:#9168

Closed
markharwood wants to merge 1 commit intoelastic:masterfrom
markharwood:fix/9156
Closed

Improved search timeout checking capabilities:#9168
markharwood wants to merge 1 commit intoelastic:masterfrom
markharwood:fix/9156

Conversation

@markharwood
Copy link
Copy Markdown
Contributor

@markharwood markharwood commented Jan 6, 2015

  • Now accounts for time from creation of SearchContext
  • new “isTimedOut” helper method on SearchContext for use by known hotspots
  • IncludeExclude is a known hotspot and so added checks for timeouts
  • Fixed ThreadPool which was looking for a timer config setting name that its own validation code prohibited

Closes #9156

@markharwood markharwood mentioned this pull request Jan 9, 2015
Closed
@markharwood
Improved timeout checking capabilities:
76a580f 
* Now accounts for time from creation of SearchContext
* new “isTimedOut” helper method on SearchContext for use by known hotspots
* IncludeExclude is a known hotspot and so added checks for timeouts
* Fixed ThreadPool which was looking for a timer config setting name that its own validation code prohibited
* SignificantTerms hotspot in looking up background term frequencies is timed

Closes #9156
@markharwood markharwood added review and removed review labels Jan 9, 2015
@clintongormley
Copy link
Copy Markdown
Contributor

clintongormley commented Jan 28, 2015

@s1monw and @rmuir could you take a look at this scaled down PR for the improved timeouts please?

@markharwood
Copy link
Copy Markdown
Contributor Author

markharwood commented May 26, 2015

I took a look at rebasing this recently.
The problem areas this PR was tackling have undergone changes which made it difficult to merge and may have invalidated the purpose of the PR.

  • Configuration settings had undergone a significant rework which means that the timeout interval settings were renamed (and now may actually honour any custom settings in the config). If there is a residual issue this should be tackled with a more targeted issue/PR.
  • The IncludeExclude regex hotspot had fundamentally been rewritten to use Lucene automaton and as such hit a new error ("too many states") pretty quickly before it got into any real death-spins (see Terms agg fails with medium-large "exclude" clause lists #11176 )

@clintongormley
Copy link
Copy Markdown
Contributor

clintongormley commented May 26, 2015

thanks @markharwood - sounds like we can close this one then? (and #4586 as well?)

@clintongormley clintongormley mentioned this pull request May 26, 2015
Closed
@nemobis
Copy link
Copy Markdown

nemobis commented May 30, 2015

Configuration settings had undergone a significant rework which means that the timeout interval settings were renamed (and now may actually honour any custom settings in the config).

Thanks. In what release does this happen and could you be so kind as to link the docs for it?

@roykachouh
Copy link
Copy Markdown

roykachouh commented Nov 17, 2015

+1 I'd like to add this feature as well, but I'm having a tough discerning if it was actually implemented and in which version?

@sameerr25
Copy link
Copy Markdown

sameerr25 commented Dec 30, 2016

+1 Which PR improved the search timeout capabilities, can someone provide link to it?

@markharwood
Copy link
Copy Markdown
Contributor Author

markharwood commented Dec 30, 2016

Which PR improved the search timeout capabilities,

Improving timeout capabilities is a process of weaving checks into all the "hot loops".
The main hot loop is the the collecting logic for processing each doc and that is handled at a low level in Lucene's TimeLimitingCollector class.
We had a hot-loop in the regex handling of terms agg include clauses but that logic was rewritten to be much faster using Lucene-level classes here: aecd9ac

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Improved timeout implementation

5 participants

@markharwood @clintongormley @nemobis @roykachouh @sameerr25