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

Updating ESSingleNodeTestCase to ensure that all free_context actions have been consumed before tearDown #110595

Conversation

pmpailis
Copy link
Contributor

@pmpailis pmpailis commented Jul 8, 2024

Relates to #109830

Added a take to ensure that all free_context actions have been consumed by the generic threadpool before performing any checks on the state of open readers in ESSingleNodeTestCase.

@pmpailis pmpailis added >test Issues or PRs that are addressing/adding tests :Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team labels Jul 8, 2024
@pmpailis pmpailis requested a review from a team as a code owner July 8, 2024 15:33
@elasticsearchmachine
Copy link
Collaborator

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

Copy link
Member

@benwtrent benwtrent left a comment

Choose a reason for hiding this comment

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

I might be missing important context, but this seems like way to much. Why can't we wait for the tasks to be completed in the effected test class directly?

@@ -130,6 +132,8 @@ public void tearDown() throws Exception {
logger.trace("[{}#{}]: cleaning up after test", getTestClass().getSimpleName(), getTestName());
awaitIndexShardCloseAsyncTasks();
ensureNoInitializingShards();
ensureAllFreeContextActionsAreConsumed();
Copy link
Member

Choose a reason for hiding this comment

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

Why can't we await for the tasks to disappear? We should be able to wait for the tasks to be completed.

Is this causing more tests to fail other than the single test case that this was originally flagged for?

I would think adding something like this to the sub-class tear down:

logger.info("--> waiting for all ongoing tasks to complete within a reasonable time");
        safeGet(clusterAdmin().prepareListTasks().setActions(FREE_CONTEXT_ACTION_NAME.name() + "*").setWaitForCompletion(true).execute());

would suffice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems to suffice indeed! Thanks for the great alternative @benwtrent ! Really wanted to avoid adding some much boilerplate code, but wasn't certain of the available feasible options for the mocked single node case, so I went with this.

Will remove all other changes in favor of this one-line :D

Copy link
Member

Choose a reason for hiding this comment

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

@pmpailis sorry to harp on this, but does this really need to be in test/framework/src/main/java/org/elasticsearch/test/ESSingleNodeTestCase.java ? Why not only focus on the individual tests that have the issue? Are way more tests than just SearchProgressActionListenerIT failing?

Copy link
Member

Choose a reason for hiding this comment

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

@pmpailis if no other tests than SearchProgressActionListenerIT are failing because of this, I would much prefer a tearDown() override, waiting there, and then calling super.tearDown()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately there were various tests failing for the same (seemingly) reason:

It seems that this is an issue for the whole suite of ESSingleNodeTestCase tests, hence why I opted for the class tearDown override.

Copy link
Member

Choose a reason for hiding this comment

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

@pmpailis fabulous. I was not aware. Please make sure that this PR is mentioned in those various tests.

…deTestCase.java

Co-authored-by: Benjamin Trent <ben.w.trent@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team >test Issues or PRs that are addressing/adding tests v8.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants