Skip to content

Fix ThreadPoolMergeExecutorServiceTests testIORateIsAdjustedForRunningMergeTasks #126058

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Jun 16, 2025
Merged

Fix ThreadPoolMergeExecutorServiceTests testIORateIsAdjustedForRunningMergeTasks #126058

merged 10 commits into from
Jun 16, 2025

Conversation

@albertzaharovits
Copy link
Contributor

@albertzaharovits albertzaharovits commented Apr 1, 2025

Fixes #125842
Relates #120869

@albertzaharovits albertzaharovits added >test Issues or PRs that are addressing/adding tests :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. labels Apr 1, 2025
@albertzaharovits albertzaharovits self-assigned this Apr 1, 2025
@elasticsearchmachine elasticsearchmachine added the Team:Distributed Indexing Meta label for Distributed Indexing team label Apr 1, 2025
@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Apr 1, 2025

Pinging @elastic/es-distributed-indexing (Team:Distributed Indexing)

@albertzaharovits
Copy link
Contributor Author

albertzaharovits commented Apr 1, 2025

The only test failure (https://gradle-enterprise.elastic.co/s/f7fv2rsrf2mwy) was when some merge thread was interrupted...

henningandersen
henningandersen reviewed Apr 2, 2025
View reviewed changes
Copy link
Contributor

@henningandersen henningandersen left a comment

Choose a reason for hiding this comment

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

AFAICS from the test run, the order of things is opposite. The assertBusy below seems to time-out, which then causes us to interrupt the threads.

I think we've not found the root cause here?

@albertzaharovits
Copy link
Contributor Author

albertzaharovits commented Jun 12, 2025

I think we've not found the root cause here?

I haven't figured it out 100% what's happening here, but I've strengthen the assertions that the submitted task was indeed running or aborting before asserting the disk IO rate update on the set of all currently running merge tasks (this should rule out any interference between the loops in the test).

@albertzaharovits
Copy link
Contributor Author

albertzaharovits commented Jun 12, 2025

@henningandersen Can you take another look here, please?

henningandersen
henningandersen approved these changes Jun 13, 2025
View reviewed changes
Copy link
Contributor

@henningandersen henningandersen left a comment

Choose a reason for hiding this comment

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

LGTM

server/src/test/java/org/elasticsearch/index/engine/ThreadPoolMergeExecutorServiceTests.java
threadPoolMergeExecutorService.submitMergeTask(mergeTask);
long newIORate = threadPoolMergeExecutorService.getTargetIORateBytesPerSec();
// all currently running merge tasks must be IO throttled
boolean mergeTaskSubmitted = threadPoolMergeExecutorService.submitMergeTask(mergeTask);
Copy link
Contributor

@henningandersen henningandersen Jun 13, 2025

Choose a reason for hiding this comment

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

Can we assert that the runSemaphore has no permits here? I think that is an assumption.

Copy link
Contributor Author

@albertzaharovits albertzaharovits Jun 13, 2025

Choose a reason for hiding this comment

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

Pushed 5db6465

@albertzaharovits albertzaharovits added v9.0.3 v8.19.0 auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) auto-backport Automatically create backport pull requests when merged labels Jun 13, 2025
@elasticsearchmachine elasticsearchmachine merged commit d98327c into elastic:main Jun 16, 2025
24 checks passed
@albertzaharovits albertzaharovits deleted the fix-125842 branch June 16, 2025 18:00
@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Jun 16, 2025

💔 Backport failed

Status Branch Result
❌ 8.19 Commit could not be cherrypicked due to conflicts
❌ 9.0 Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 126058

albertzaharovits added a commit to albertzaharovits/elasticsearch that referenced this pull request Jun 17, 2025
@albertzaharovits
Fix ThreadPoolMergeExecutorServiceTests testIORateIsAdjustedForRunnin…
4b01793 
…gMergeTasks (elastic#126058)

Fixes elastic#125842 Relates
elastic#120869
@albertzaharovits albertzaharovits mentioned this pull request Jun 17, 2025
Merged
albertzaharovits added a commit to albertzaharovits/elasticsearch that referenced this pull request Jun 17, 2025
@albertzaharovits
Fix ThreadPoolMergeExecutorServiceTests testIORateIsAdjustedForRunnin…
94fb9de 
…gMergeTasks (elastic#126058)

Fixes elastic#125842 Relates
elastic#120869
@albertzaharovits albertzaharovits mentioned this pull request Jun 17, 2025
Merged
albertzaharovits added a commit that referenced this pull request Jun 17, 2025
@albertzaharovits
Fix ThreadPoolMergeExecutorServiceTests testIORateIsAdjustedForRunnin…
12411fd 
…gMergeTasks (#126058) (#129516)

Fixes #125842
Relates #120869
albertzaharovits added a commit that referenced this pull request Jun 17, 2025
@albertzaharovits
Fix ThreadPoolMergeExecutorServiceTests testIORateIsAdjustedForRunnin…
eb22fa1 
…gMergeTasks (#126058) (#129515)

Fixes #125842
Relates #120869
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@henningandersen henningandersen henningandersen approved these changes

Assignees

@albertzaharovits albertzaharovits

Labels

auto-backport Automatically create backport pull requests when merged auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. Team:Distributed Indexing Meta label for Distributed Indexing team >test Issues or PRs that are addressing/adding tests v8.19.0 v9.0.3 v9.1.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[CI] ThreadPoolMergeExecutorServiceTests testIORateIsAdjustedForRunningMergeTasks failing

3 participants

@albertzaharovits @elasticsearchmachine @henningandersen