Fix QueueResizableOpenSearchThreadPoolExecutorTests #18006
Conversation
andrross
requested review from
Bukhtawar,
CEHENKLE,
Rishikesh1159,
VachaShah,
anasalkouz,
ashking94,
bugmakerrrrrr,
cwperks,
dbwiddis,
gbbafna,
jainankitk,
kotwanikunal,
linuxpi,
mch2,
msfroh,
nknize,
owaiskazi19,
reta,
sachinpkale,
saratvemulapalli,
shwetathareja and
sohami
as code owners
github-actions
bot
added
>test-failure
|
❌ 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? |
|
❌ 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? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #18006 +/- ##
============================================
- Coverage 72.52% 72.50% -0.02%
+ Complexity 67163 67138 -25
============================================
Files 5473 5473
Lines 310092 310094 +2
Branches 45060 45061 +1
============================================
- Hits 224899 224840 -59
- Misses 66814 66891 +77
+ Partials 18379 18363 -16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ashking94
left a comment
ashking94
left a comment
There was a problem hiding this comment.
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?
...a/org/opensearch/common/util/concurrent/QueueResizableOpenSearchThreadPoolExecutorTests.java
Outdated
Show resolved
Hide resolved
...a/org/opensearch/common/util/concurrent/QueueResizableOpenSearchThreadPoolExecutorTests.java
Show resolved
Hide resolved
github-project-automation
bot
moved this to 👀 In review
in Cluster Manager Project Board
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. |
andrross
force-pushed
the
fix-QueueResizableOpenSearchThreadPoolExecutorTests
branch
from
a6990d9 to
210949d
Compare
|
❌ 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? |
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. Signed-off-by: Andrew Ross <andrross@amazon.com>
andrross
force-pushed
the
fix-QueueResizableOpenSearchThreadPoolExecutorTests
branch
from
210949d to
7ba1155
Compare
|
❌ 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? |
|
❌ 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? |
|
❌ 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? |
|
@ashking94 Can you take another look at this? Thanks! |
andrross
dismissed
ashking94’s stale review
Addressed concerns. No response after a few weeks.
github-project-automation
bot
moved this from 👀 In review
to ✅ Done
in Cluster Manager Project Board
…ct#18006) 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. Signed-off-by: Andrew Ross <andrross@amazon.com>
…ct#18006) 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. Signed-off-by: Andrew Ross <andrross@amazon.com>Signed-off-by: TJ Neuenfeldt <tjneu@amazon.com>
…ct#18006) 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. Signed-off-by: Andrew Ross <andrross@amazon.com>
4 participants
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
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.