Fix QueueResizableOpenSearchThreadPoolExecutorTests

[andrross]

There was a race condition in testResizeQueueDown() where depending on
random parameters we could submit up to 1002 tasks into an executor with
a queue size of 900. That introduced a race condition where if the tasks
didn't execute fast enough then a rejected execution exception could
happen and fail the test. The fix is to resize down to a queue size of
1500 to ensure there is enough capacity even if all tasks are submitted
before any can be executed.

And finally I refactored the tests to reduce duplication of code and
ensure the executor gets shutdown properly even in case of a test
failure. This will avoid the spurious thread leak failure if a test case
exits because of a failure.

Related Issues

Resolves #14297

Check List

• Functionality includes testing.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

❌ Gradle check result for 7e471e1: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❌ Gradle check result for cc0563c: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[reta]

I guess I don't know what the intent of testResizeQueueSameSize() is because the class does not allow you to call resize and pass the same size.

@andrross I think the name of the method may be confusing; just looking into the test, what it does is that

1. create a pool with a queue of capacity 2000
2. resize the queue capacity to 1000
3. submit bunch of tasks and checks that queue size it at most 1000 (the same) or less

Is it helpful?

[andrross]

I guess I don't know what the intent of testResizeQueueSameSize() is because the class does not allow you to call resize and pass the same size.

@andrross I think the name of the method may be confusing; just looking into the test, what it does is that

1. create a pool with a queue of capacity 2000
2. resize the queue capacity to 1000
3. submit bunch of tasks and checks that queue size it at most 1000 (the same) or less

Is it helpful?

Got it. I think we can keep the test as it is.

[github-actions]

❌ Gradle check result for 2a562b9: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❌ Gradle check result for 2a562b9: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❌ Gradle check result for 7279456: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❌ Gradle check result for 7279456: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❌ Gradle check result for 7279456: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❌ Gradle check result for a6990d9: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❌ Gradle check result for a6990d9: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

✅ Gradle check result for a6990d9: SUCCESS

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.59%. Comparing base (6afec2a) to head (a6990d9).
Report is 6 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #18006      +/-   ##
============================================
+ Coverage     72.57%   72.59%   +0.02%     
- Complexity    67160    67170      +10     
============================================
  Files          5478     5478              
  Lines        310130   310132       +2     
  Branches      45087    45087              
============================================
+ Hits         225068   225147      +79     
+ Misses        66661    66573      -88     
- Partials      18401    18412      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ashking94]

There was a race condition in testResizeQueueDown() where depending on random parameters we could submit up to 1002 tasks into an executor with a queue size of 900. That introduced a race condition where if the tasks didn't execute fast enough then a rejected execution exception could happen and fail the test.

I am unable to see on what fixed this flakiness in the test testResizeQueueDown?

[ashking94]

Objects.requireNonNull() is designed for reference types only. For primitive data types, this check would always pass. Did you intend to do something else? may be value check?

[andrross]

Good catch! This was leftover from a previous version of the code where it actually made sense.

[ashking94]

for testExceptionThrowingTask, the runnable wrapper was earlier exceptionalWrapper() which is changed to fastWrapper. What's the reason for this change?

[andrross]

Another good catch, this got mixed up in the refactoring. I'll fix it.

[andrross]

There was a race condition in testResizeQueueDown() where depending on random parameters we could submit up to 1002 tasks into an executor with a queue size of 900. That introduced a race condition where if the tasks didn't execute fast enough then a rejected execution exception could happen and fail the test.

I am unable to see on what fixed this flakiness in the test testResizeQueueDown?

Instead of resizing down to 900, it resizes down to 1500 which guarantees that the executor has enough capacity to not reject anything if all tasks are submitted before any are able to be executed.

[github-actions]

❌ Gradle check result for 210949d: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❌ Gradle check result for 7ba1155: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❌ Gradle check result for 7ba1155: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?