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

Invalidate cached query results if query timed out #22807

Merged
merged 1 commit into from
Jan 26, 2017

Conversation

s1monw
Copy link
Contributor

@s1monw s1monw commented Jan 26, 2017

Today we cache query results even if the query timed out. This is obviously
problematic since results are not complete. Yet, the decision if a query timed
out or not happens too late to simply not cache the result since if we'd just throw
an exception all currently waiting requests with the same request / cache key would
fail with the same exception without the option to access the result or to re-execute.
Instead, this change will allow the request to enter the cache but invalidates it immediately.
Concurrent request might not get executed and return the timed out result which is not absolutely
correct but very likely since identical requests will likely timeout as well. As a side-effect
we won't hammer the node with concurrent slow searches but rather only execute one of them
and return shortly cached result.

Closes #22789

Today we cache query results even if the query timed out. This is obviously
problematic since results are not complete. Yet, the decision if a query timed
out or not happens too late to simply not cache the result since if we'd just throw
an exception all currently waiting requests with the same request / cache key would
fail with the same exception without the option to access the result or to reexecute.
Instead, this change will allow the request to enter the cache but invalidates it immediately.
Concurrent request might not get executed and return the timed out result which is not absolutely
correct but very likely since idendical requests will likey timeout as well. As a side-effect
we won't hammer the node with concurrent slow searches but rather only execute one of them
and return shortly cached result.

Closes elastic#22789
Copy link
Contributor

@colings86 colings86 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

Makes sense to me!

@s1monw
Copy link
Contributor Author

s1monw commented Jan 26, 2017

@elasticmachine test this please

@s1monw s1monw merged commit a475323 into elastic:master Jan 26, 2017
@s1monw s1monw deleted the issues/22789 branch January 26, 2017 15:45
s1monw added a commit that referenced this pull request Jan 26, 2017
Today we cache query results even if the query timed out. This is obviously
problematic since results are not complete. Yet, the decision if a query timed
out or not happens too late to simply not cache the result since if we'd just throw
an exception all currently waiting requests with the same request / cache key would
fail with the same exception without the option to access the result or to re-execute.
Instead, this change will allow the request to enter the cache but invalidates it immediately.
Concurrent request might not get executed and return the timed out result which is not absolutely
correct but very likely since identical requests will likely timeout as well. As a side-effect
we won't hammer the node with concurrent slow searches but rather only execute one of them
and return shortly cached result.

Closes #22789
s1monw added a commit that referenced this pull request Jan 26, 2017
Today we cache query results even if the query timed out. This is obviously
problematic since results are not complete. Yet, the decision if a query timed
out or not happens too late to simply not cache the result since if we'd just throw
an exception all currently waiting requests with the same request / cache key would
fail with the same exception without the option to access the result or to re-execute.
Instead, this change will allow the request to enter the cache but invalidates it immediately.
Concurrent request might not get executed and return the timed out result which is not absolutely
correct but very likely since identical requests will likely timeout as well. As a side-effect
we won't hammer the node with concurrent slow searches but rather only execute one of them
and return shortly cached result.

Closes #22789
s1monw added a commit that referenced this pull request Jan 26, 2017
Today we cache query results even if the query timed out. This is obviously
problematic since results are not complete. Yet, the decision if a query timed
out or not happens too late to simply not cache the result since if we'd just throw
an exception all currently waiting requests with the same request / cache key would
fail with the same exception without the option to access the result or to re-execute.
Instead, this change will allow the request to enter the cache but invalidates it immediately.
Concurrent request might not get executed and return the timed out result which is not absolutely
correct but very likely since identical requests will likely timeout as well. As a side-effect
we won't hammer the node with concurrent slow searches but rather only execute one of them
and return shortly cached result.

Closes #22789
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jan 26, 2017
* master: (47 commits)
  Remove non needed import
  use expectThrows instead of manually testing exception
  Fix checkstyle and a test
  Update after review
  Read ec2 discovery address from aws instance tags
  Invalidate cached query results if query timed out (elastic#22807)
  Add remaining generated painless API
  Generate reference links for painless API (elastic#22775)
  [TEST] Fix ElasticsearchExceptionTests
  Add parsing method for ElasticsearchException.generateThrowableXContent() (elastic#22783)
  Improve connection closing in `RemoteClusterConnection` (elastic#22804)
  Docs: Cluster allocation explain should be on one page
  Remove DFS_QUERY_AND_FETCH as a search type (elastic#22787)
  Add repository-url module and move URLRepository (elastic#22752)
  fix date-processor to a new default year for every new pipeline execution. (elastic#22601)
  Add tests for top_hits aggregation (elastic#22754)
  [TEST] Added this for 93a28b0 submitted via elastic#22772
  Fix typo in comment in OsProbe.java
  Add new ruby search library to community clients doc (elastic#22765)
  RangeQuery WITHIN case now normalises query (elastic#22431)
  ...
@s1monw s1monw added v5.1.3 and removed v5.1.1 labels Jan 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants