Skip to content

Fail test if Node#awaitClose times out#85238

Merged
elasticsearchmachine merged 1 commit intoelastic:masterfrom
DaveCTurner:2022-03-22-node-close-timeout-assertion
Mar 22, 2022
Merged

Fail test if Node#awaitClose times out#85238
elasticsearchmachine merged 1 commit intoelastic:masterfrom
DaveCTurner:2022-03-22-node-close-timeout-assertion

Conversation

@DaveCTurner
Copy link
Copy Markdown
Member

@DaveCTurner DaveCTurner commented Mar 22, 2022

We rely on nodes stopping gracefully in integ tests, rather than timing
out and using ThreadPool#shutdownNow to ignore any remaining cleanup
work. Today we throw an IOException if the shutdown wasn't graceful.
With this commit we move to an AssertionError so we can be sure that
any such problems result in a test failure.

Relates #85131

@DaveCTurner
Fail test if Node#awaitClose times out
920e6e1 
We rely on nodes stopping gracefully in integ tests, rather than timing
out and using `ThreadPool#shutdownNow` to ignore any remaining cleanup
work. Today we throw an `IOException` if the shutdown wasn't graceful.
With this commit we move to an `AssertionError` so we can be sure that
any such problems result in a test failure.

Relates elastic#85131
@DaveCTurner DaveCTurner added >test Issues or PRs that are addressing/adding tests :Core/Infra/Node Lifecycle Node startup, bootstrapping, and shutdown v8.2.0 labels Mar 22, 2022
@DaveCTurner DaveCTurner mentioned this pull request Mar 22, 2022
Merged
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Mar 22, 2022
@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Mar 22, 2022

Pinging @elastic/es-core-infra (Team:Core/Infra)

original-brownbear
original-brownbear approved these changes Mar 22, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@original-brownbear original-brownbear left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM (assuming this becomes green)

@DaveCTurner DaveCTurner added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Mar 22, 2022
@elasticsearchmachine elasticsearchmachine merged commit 7805588 into elastic:master Mar 22, 2022
@DaveCTurner DaveCTurner deleted the 2022-03-22-node-close-timeout-assertion branch March 22, 2022 18:46
@original-brownbear original-brownbear mentioned this pull request Apr 25, 2022
Merged
original-brownbear added a commit that referenced this pull request May 17, 2022
@original-brownbear
Removing Blocking Wait for Close in RecoverySourceHandler (#86127)
8ff0189 
Remove the blocking wait when releasing safe commits or store references on the recovery source.
This seems safe enough these days with #85238 not tripping
and the assertion that makes sure we never submit the close task to an already shut-down pool

closes #85839
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@original-brownbear original-brownbear original-brownbear approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Core/Infra/Node Lifecycle Node startup, bootstrapping, and shutdown Team:Core/Infra Meta label for core/infra team >test Issues or PRs that are addressing/adding tests v8.2.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@DaveCTurner @elasticmachine @original-brownbear @elasticsearchmachine