Fix expiration time in async search response#55435
Conversation
This change ensures that we return the latest expiration time when retrieving the response from the index.
|
Pinging @elastic/es-search (:Search/Search) |
javanna
left a comment
There was a problem hiding this comment.
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
| 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.
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.
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.
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.
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.
yea I see, can you add a comment that this is intentionally undocumented?
...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. |
...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.