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

Disable search context release checks after every test #56673

Merged
merged 5 commits into from
May 14, 2020

Conversation

jimczi
Copy link
Contributor

@jimczi jimczi commented May 13, 2020

This change adds a way to disable the checks that search contexts are all released
after each test in ESTestCase.
This is used by ESIntegTestCase to disable these checks if the cluster is shared
across all methods (SUITE scope). In this case, the contexts are checked at the end
when the cluster is cleared. This fixes integration tests that starts components that
perform search requests internally with a fixed delay (e.g.: async search maintenance service).

Fixes #55988

This change adds a way to disable the checks that search contexts are all released
after each test in ESTestCase.
This is used by ESIntegTestCase to disable these checks if the cluster is shared
across all methods (SUITE scope). In this case, the contexts are checked at the end
when the cluster is cleared. This fixes integration tests that starts components that
perform search requests internally with a fixed delay (e.g.: async search maintenance service).

Fixes elastic#55988
@jimczi jimczi added >non-issue :Search/Search Search-related issues that do not fall into other categories >test-failure Triaged test failures from CI labels May 13, 2020
@jimczi jimczi requested a review from javanna May 13, 2020 10:23
@elasticmachine
Copy link
Collaborator

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

@elasticmachine elasticmachine added the Team:Search Meta label for search team label May 13, 2020
Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

Another way to address the failures would be to plug in a no-op maintenance service in this test. I am not sure how much trouble that would be though, hence I am also ok with this solution.

@javanna
Copy link
Member

javanna commented May 13, 2020

Another way to address the failures would be to plug in a no-op maintenance service in this test.

I meant in the async search tests that are failing due to this check. I wonder if there are other cases where the check may cause problems, and whether it is useful today in between tests, probably not.

@jimczi
Copy link
Contributor Author

jimczi commented May 13, 2020

The initial fix doesn't work, we always check the releasing of search contexts before the node is stopped so the maintenance service continues to fire delete by query request.
I pushed a new change that stop/start the maintenance service between each test in order to ensure that we stop sending delete by query requests before the check is done.
Can you take another look @javanna ?

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

still LGTM but even better than before :)

@@ -71,6 +75,25 @@ public SearchTestPlugin() {}
}
}

@Before
public void startMaitenanceService() {
Copy link
Member

Choose a reason for hiding this comment

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

s/Maitenance/Maintenance here and below

@jimczi jimczi merged commit 39a2dec into elastic:master May 14, 2020
@jimczi jimczi deleted the scope_search_context branch May 14, 2020 09:45
jimczi added a commit that referenced this pull request May 14, 2020
This change ensures that the maintenance service that is responsible for deleting the expired response is stopped between each test. This is needed since we check that no search context are in-flight after each test method.

Fixes #55988
jimczi added a commit that referenced this pull request May 14, 2020
This change ensures that the maintenance service that is responsible for deleting the expired response is stopped between each test. This is needed since we check that no search context are in-flight after each test method.

Fixes #55988
jimczi added a commit that referenced this pull request May 15, 2020
This change ensures that the maintenance service that is responsible for deleting the expired response is stopped between each test. This is needed since we check that no search context are in-flight after each test method.

Fixes #55988
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>non-issue :Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team >test-failure Triaged test failures from CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] AsyncSearchActionIT failing with assertion error
4 participants