-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Fix expiration time in async search response #55435
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
Conversation
This change ensures that we return the latest expiration time when retrieving the response from the index.
Pinging @elastic/es-search (:Search/Search) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for fixing, I left a bunch of comments, mainly questions.
...nc-search/src/main/java/org/elasticsearch/xpack/search/TransportSubmitAsyncSearchAction.java
Outdated
Show resolved
Hide resolved
...in/async-search/src/test/java/org/elasticsearch/xpack/search/GetAsyncSearchRequestTests.java
Show resolved
Hide resolved
|
||
/** | ||
* A service that runs a periodic cleanup over the async-search index. | ||
*/ | ||
class AsyncSearchMaintenanceService implements Releasable, ClusterStateListener { | ||
private static final Logger logger = LogManager.getLogger(AsyncSearchMaintenanceService.class); | ||
|
||
public static final Setting<TimeValue> ASYNC_SEARCH_CLEANUP_INTERVAL_SETTING = | ||
Setting.timeSetting("async_search.index_cleanup_interval", TimeValue.timeValueHours(1), Setting.Property.NodeScope); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we plan on using this new setting only in our tests? Or can it be useful for users too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe for debug but I don't think it's useful for users, only for tests at the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no way to somehow make this "private" or for use only in our tests? I worry mostly about naming, and the fact that users may end up using it although we would not want them to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default value is quite large (1h) so maybe there's some value to change it. I am not sure, I can make it private but that would work only in fantasy integration tests (single vm) so this will come back. I was expecting that the lack of documentation would make this setting an expert thing that users would never heard from ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea I see, can you add a comment that this is intentionally undocumented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed 09afaca
...async-search/src/main/java/org/elasticsearch/xpack/search/AsyncSearchMaintenanceService.java
Show resolved
Hide resolved
...async-search/src/main/java/org/elasticsearch/xpack/search/AsyncSearchMaintenanceService.java
Show resolved
Hide resolved
...lugin/async-search/src/main/java/org/elasticsearch/xpack/search/AsyncSearchIndexService.java
Show resolved
Hide resolved
x-pack/plugin/async-search/src/main/java/org/elasticsearch/xpack/search/AsyncSearchTask.java
Outdated
Show resolved
Hide resolved
...lugin/core/src/main/java/org/elasticsearch/xpack/core/search/action/AsyncSearchResponse.java
Show resolved
Hide resolved
...ugin/core/src/main/java/org/elasticsearch/xpack/core/search/action/GetAsyncSearchAction.java
Show resolved
Hide resolved
Thanks for looking @javanna , I pushed some changes to address your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
...ck/plugin/async-search/src/test/java/org/elasticsearch/xpack/search/AsyncSearchActionIT.java
Show resolved
Hide resolved
@elasticmachine run elasticsearch-ci/1 |
This change ensures that we return the latest expiration time when retrieving the response from the index. This commit also fixes a bug that stops the garbage collection of saved responses if the async search index is deleted.
This change ensures that we return the latest expiration time when retrieving the response from the index. This commit also fixes a bug that stops the garbage collection of saved responses if the async search index is deleted.
This change ensures that we return the latest expiration time when retrieving the response from the index.
This commit also fixes a bug that stops the garbage collection of saved responses if the async search index is deleted.