Async search: create internal index only before storing initial response by javanna · Pull Request #54619 · elastic/elasticsearch

javanna · 2020-04-01T22:43:52Z

We currently create the .async-search index if necessary before performing any action (index, update or delete). Truth is that this is needed only before storing the initial response. The other operations are either update or delete, which will anyways not find the document to update/delete even if the index gets created when missing. This also caused testCancellation failures as we were trying to delete the document twice from the .async-search index, once from TransportDeleteAsyncSearchAction and once as a consequence of the search task being completed. The latter may be called after the test is completed, but before the cluster is shut down and causing problems to the after test checks, for instance if it happens after all the indices have been cleaned up. It is totally fine to try to delete a response that is no longer found, but not quite so if such call will also trigger an index creation.

With this commit we remove all the calls to createIndexIfNecessary from the update/delete operation, and we leave one call only from storeInitialResponse which is where the index is expected to be created.

Closes #54180

…initial response We currently create the .async-search index if necessary before performing any action (index, update or delete). Truth is that this is needed only before storing the initial response. The other operations are either update or delete, which will anyways not find the document to update/delete even if the index gets created when missing. This also caused `testCancellation` failures as we were trying to delete the document twice from the .async-search index, once from `TransportDeleteAsyncSearchAction` and once as a consequence of the search task being completed. The latter may be called after the test is completed, but before the cluster is shut down and causing problems to the after test checks, for instance if it happens after all the indices have been cleaned up. It is totally fine to try to delete a response that is no longer found, but not quite so if such call will also trigger an index creation. With this commit we remove all the calls to createIndexIfNecessary from the update/delete operation, and we leave one call only from storeInitialResponse which is where the index is expected to be created. Closes elastic#54180

elasticmachine · 2020-04-01T22:43:54Z

Pinging @elastic/es-search (:Search/Search)

javanna · 2020-04-01T22:45:08Z

...lugin/async-search/src/main/java/org/elasticsearch/xpack/search/AsyncSearchIndexService.java

-                        logger.error(() -> new ParameterizedMessage("failed to clean async-search [{}]", searchId.getEncoded()), exc);
-                        listener.onFailure(exc);
-                    })),
-                listener::onFailure));


I tried to make this logic more readable. I found the boolean flag hard to reason about especially as it was provided true only once. I moved the listener wrapping to the callers, where each caller needs to do something different.

javanna · 2020-04-01T22:45:51Z

...async-search/src/main/java/org/elasticsearch/xpack/search/TransportGetAsyncSearchAction.java

+                                //don't even log when: the async search document or its index is not found. That can happen if an invalid
+                                //search id is provided and no async search initial response has been stored yet.
+                                if (exc.getCause() instanceof DocumentMissingException == false
+                                    && exc.getCause() instanceof IndexNotFoundException == false) {


I was wondering if we are sure about exc.getCause here. What's the top level exception?

A RemoteTransportException. Maybe we can replace the instanceof with ExceptionsHelper.status(exc) != RestStatus.NOT_FOUND ?

the problem I have with ExceptionsHelper.status(exc) is that it does not look at the cause at all and it gives 500 if it does not know any better. How about ExceptionsHelper.status(ExceptionsHelper.unwrapCause(exc)) ?

jimczi

This makes sense to me. I left some comments.

jimczi · 2020-04-03T08:24:30Z

...lugin/async-search/src/main/java/org/elasticsearch/xpack/search/AsyncSearchIndexService.java

-                        logger.error(() -> new ParameterizedMessage("failed to clean async-search [{}]", searchId.getEncoded()), exc);
-                        listener.onFailure(exc);
-                    })),
-                listener::onFailure));


jimczi · 2020-04-03T08:43:42Z

...async-search/src/main/java/org/elasticsearch/xpack/search/TransportGetAsyncSearchAction.java

+                                //don't even log when: the async search document or its index is not found. That can happen if an invalid
+                                //search id is provided and no async search initial response has been stored yet.
+                                if (exc.getCause() instanceof DocumentMissingException == false
+                                    && exc.getCause() instanceof IndexNotFoundException == false) {


A RemoteTransportException. Maybe we can replace the instanceof with ExceptionsHelper.status(exc) != RestStatus.NOT_FOUND ?

jimczi · 2020-04-03T08:47:07Z

...nc-search/src/main/java/org/elasticsearch/xpack/search/TransportDeleteAsyncSearchAction.java

+                    r -> listener.onResponse(new AcknowledgedResponse(true)),
+                    exc -> {
+                        //the index may not be there (no initial async search response stored yet?): we still want to return 200
+                        if (exc.getCause() instanceof IndexNotFoundException) {


We shouldn't fail If the document is missing:

Suggested change

if (exc.getCause() instanceof IndexNotFoundException) {

if (ExceptionsHelper.status(exc) == RestStatus.NOT_FOUND) {

we don't, document missing does not come back as a failure. Yet, I agree on changing the condition as exc.getCause() instanceof IndexNotFoundException is not great

…nse (#54619) We currently create the .async-search index if necessary before performing any action (index, update or delete). Truth is that this is needed only before storing the initial response. The other operations are either update or delete, which will anyways not find the document to update/delete even if the index gets created when missing. This also caused `testCancellation` failures as we were trying to delete the document twice from the .async-search index, once from `TransportDeleteAsyncSearchAction` and once as a consequence of the search task being completed. The latter may be called after the test is completed, but before the cluster is shut down and causing problems to the after test checks, for instance if it happens after all the indices have been cleaned up. It is totally fine to try to delete a response that is no longer found, but not quite so if such call will also trigger an index creation. With this commit we remove all the calls to createIndexIfNecessary from the update/delete operation, and we leave one call only from storeInitialResponse which is where the index is expected to be created. Closes #54180

javanna added >test Issues or PRs that are addressing/adding tests :Search/Search Search-related issues that do not fall into other categories v8.0.0 v7.7.0 v7.8.0 labels Apr 1, 2020

javanna requested a review from jimczi April 1, 2020 22:43

javanna commented Apr 1, 2020

View reviewed changes

Merge branch 'master' into enhancement/async_search_create_index

1e087db

jimczi reviewed Apr 3, 2020

View reviewed changes

javanna added 3 commits April 10, 2020 11:34

Merge branch 'master' into enhancement/async_search_create_index

5bbde18

iter

f739f19

checkstyle

b2f956d

javanna merged commit 1b69477 into elastic:master Apr 10, 2020

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

	if (exc.getCause() instanceof IndexNotFoundException) {
	if (ExceptionsHelper.status(exc) == RestStatus.NOT_FOUND) {

Conversation

javanna commented Apr 1, 2020

Uh oh!

elasticmachine commented Apr 1, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimczi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants