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

Fix PrioritizedThrottledTaskRunnerTests #93446

Conversation

DaveCTurner
Copy link
Contributor

@DaveCTurner DaveCTurner commented Feb 2, 2023

These tests try and execute maxThreads concurrent tasks to ensure that the rest of the executor's queue has been processed, but due to #93443 (and the executor's zero timeout) this sometimes doesn't work. This commit fixes the problem by making every thread a core thread so that they do not time out.

Closes #92910
Closes #92747

These tests try and execute `maxThreads` concurrent tasks to ensure that
the rest of the executor's queue has been processed, but due to elastic#93443
(and the executor's zero timeout) this sometimes doesn't work. This
commit fixes the problem by making every thread a core thread so that
they do not time out.

Closes elastic#92910
@DaveCTurner DaveCTurner added >test Issues or PRs that are addressing/adding tests :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v8.7.0 labels Feb 2, 2023
@elasticsearchmachine elasticsearchmachine added the Team:Distributed Meta label for distributed team label Feb 2, 2023
@elasticsearchmachine
Copy link
Collaborator

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

@@ -39,7 +39,7 @@ public class PrioritizedThrottledTaskRunnerTests extends ESTestCase {
public void setUp() throws Exception {
super.setUp();
maxThreads = between(1, 10);
executor = EsExecutors.newScaling("test", 1, maxThreads, 0, TimeUnit.MILLISECONDS, false, threadFactory, threadContext);
executor = EsExecutors.newScaling("test", maxThreads, maxThreads, 0, TimeUnit.NANOSECONDS, false, threadFactory, threadContext);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please also apply same to:

final var executor = randomBoolean()
? EsExecutors.newScaling("test", 1, maxThreads, 0, TimeUnit.MILLISECONDS, true, threadFactory, threadContext)
: EsExecutors.newFixed("test", maxThreads, between(1, 5), threadFactory, threadContext, false);

to close #92747

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 good catch

@DaveCTurner DaveCTurner merged commit 2ef63a4 into elastic:main Feb 2, 2023
@DaveCTurner DaveCTurner deleted the 2023-02-02-PrioritizedThrottledTaskRunnerTests-executor-timeout branch February 2, 2023 10:29
Copy link
Contributor

@fcofdez fcofdez left a comment

Choose a reason for hiding this comment

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

LGTM 2

mark-vieira pushed a commit to mark-vieira/elasticsearch that referenced this pull request Feb 2, 2023
These tests try and execute `maxThreads` concurrent tasks to ensure that
the rest of the executor's queue has been processed, but due to elastic#93443
(and the executor's zero timeout) this sometimes doesn't work. This
commit fixes the problem by making every thread a core thread so that
they do not time out.

Closes elastic#92910
Closes elastic#92747
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Feb 6, 2023
elasticsearchmachine pushed a commit that referenced this pull request Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs Team:Distributed Meta label for distributed team >test Issues or PRs that are addressing/adding tests v8.7.0
Projects
None yet
4 participants