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

Projects
None yet
3 participants
@s1monw
Copy link
Contributor

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

Invalidate cached query results if query timed out
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 #22789
@colings86
Copy link
Member

left a comment

LGTM

@jpountz
Copy link
Contributor

left a comment

Makes sense to me!

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Jan 26, 2017

@elasticmachine test this please

@s1monw s1monw merged commit a475323 into elastic:master Jan 26, 2017

2 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci Build finished.
Details

@s1monw s1monw deleted the s1monw:issues/22789 branch Jan 26, 2017

s1monw added a commit that referenced this pull request Jan 26, 2017

Invalidate cached query results if query timed out (#22807)
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

Invalidate cached query results if query timed out (#22807)
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

Invalidate cached query results if query timed out (#22807)
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

Merge branch 'master' into replica-sequence-number-recovery
* 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
You can’t perform that action at this time.