Skip to content

Wait for task completion in IndexLifecycleRunnerTests#94901

Merged
elasticsearchmachine merged 3 commits intoelastic:mainfrom
DaveCTurner:2023-03-30-IndexLifecycleRunnerTests-wait-for-task-completion
Mar 30, 2023
Merged

Wait for task completion in IndexLifecycleRunnerTests#94901
elasticsearchmachine merged 3 commits intoelastic:mainfrom
DaveCTurner:2023-03-30-IndexLifecycleRunnerTests-wait-for-task-completion

Conversation

@DaveCTurner
Copy link
Copy Markdown
Member

These tests submit tasks to the master service and wait for the state to be applied, but not for the publication to complete, and without that wait they may clean up too soon. This commit makes them wait for the master service to finish its work before cleaning up.

Relates #94325
Closes #94900

These tests submit tasks to the master service and wait for the state to
be applied, but not for the publication to complete, and without that
wait they may clean up too soon. This commit makes them wait for the
master service to finish its work before cleaning up.

Relates elastic#94325
Closes elastic#94900
@DaveCTurner DaveCTurner added >test Issues or PRs that are addressing/adding tests :Data Management/ILM+SLM DO NOT USE. Use ":StorageEngine/ILM" or ":Distributed Coordination/SLM" instead. v8.8.0 labels Mar 30, 2023
@elasticsearchmachine elasticsearchmachine added the Team:Data Management (obsolete) DO NOT USE. This team no longer exists. label Mar 30, 2023
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-data-management (Team:Data Management)

@gmarouli gmarouli self-requested a review March 30, 2023 12:12
Copy link
Copy Markdown
Contributor

@gmarouli gmarouli left a comment

Choose a reason for hiding this comment

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

Thank you for fixing this @DaveCTurner, much appreciated. LGTM, I have one comment and a nit :).

As I was going through the change I noticed one more attempt to close things at line 309. Should we add it there too?

Nit: This test already has a set up and a tear down method, I am wondering if we could use them instead of doing the same things in many of these tests. I am happy to look into this as a follow up of this PR, if you think it's a good idea.

@DaveCTurner
Copy link
Copy Markdown
Member Author

As I was going through the change I noticed one more attempt to close things at line 309. Should we add it there too?

Thanks, yes I think that makes sense too. Added in 980f90e.

@DaveCTurner DaveCTurner added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Mar 30, 2023
@DaveCTurner
Copy link
Copy Markdown
Member Author

I am happy to look into this as a follow up of this PR, if you think it's a good idea.

Yes please. In fact I think that it'd be simpler if most of these tests just waited for the master service to complete its work instead of using the per-task latch as done at the moment.

@gmarouli
Copy link
Copy Markdown
Contributor

I am happy to look into this as a follow up of this PR, if you think it's a good idea.

Yes please. In fact I think that it'd be simpler if most of these tests just waited for the master service to complete its work instead of using the per-task latch as done at the moment.

Makes sense, I will look into it.

@elasticsearchmachine elasticsearchmachine merged commit bec25f0 into elastic:main Mar 30, 2023
@DaveCTurner DaveCTurner deleted the 2023-03-30-IndexLifecycleRunnerTests-wait-for-task-completion branch March 30, 2023 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Data Management/ILM+SLM DO NOT USE. Use ":StorageEngine/ILM" or ":Distributed Coordination/SLM" instead. Team:Data Management (obsolete) DO NOT USE. This team no longer exists. >test Issues or PRs that are addressing/adding tests v8.8.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CI] IndexLifecycleRunnerTests testRunStateChangePolicyWithNextStep failing

3 participants