Fix flakiness in MasterServiceTests.testThrottlingForMultipleTaskTypes

[andrross]

The test configured a timeout duration of zero for certain tasks and asserted that all tasks were throttled or timed out. This is not a valid assertion because it is possible for a task to complete before the asynchronous timeout operation runs, which means the task would complete successfully. The fix is to adjust the assertion to allow for successful tasks in this case.

Related Issues

Resolves #5958

Check List

• New functionality includes testing.
  • All tests pass
• New functionality has been documented.
  • New functionality has javadoc added
• Commits are signed per the DCO using --signoff
• Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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 (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/21031/
• CommitID: 818468b
  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?

[sohami]

@andrross I think we should instead handle timeout of 0 to fail the task immediately instead of scheduling the asynchronous task to remove it from the queue on timeout.

[andrross]

@andrross I think we should instead handle timeout of 0 to fail the task immediately instead of scheduling the asynchronous task to remove it from the queue on timeout.

@sohami I don't love the inherent non-determinism here, but is a zero timeout useful in practice here? I'm assuming not (why schedule a task that is already timed out?), so I also don't love putting in additional logic for this case. The test would also just be testing this additional logic of the zero timeout case and not testing the normal timeout path, so I don't think it helps with coverage. Does that make sense or is there a different way to handle this case?

[andrross]

@sohami I think it is probably better to deterministically test the timeout case in a separate test case. I'm thinking something like:

• create an instance using a single-threaded thread pool
• start one task and have it block on a latch
• start a second task with a short timeout and verify it times out (because the thread pool is fully occupied)
• unblock the first task and clean everything up

What do you think?

[sohami]

I don't love the inherent non-determinism here, but is a zero timeout useful in practice here? I'm assuming not (why schedule a task that is already timed out?), so I also don't love putting in additional logic for this case.

I don't foresee any use case with 0 timeout but seems like we are allowing that to be set anyways. If we want to allow it than handling it separately would avoid adding extra timeout tasks for which most of the time behavior will be indeterministic. The other option is if there is no use case for timeout of 0 should we fail the task setting it to that value instead ? This will also help to catch issues probably where it is set to 0 incorrectly.

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/21107/
• CommitID: c6a8457
  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?

[sohami]

@sohami I think it is probably better to deterministically test the timeout case in a separate test case. I'm thinking something like:

• create an instance using a single-threaded thread pool
• start one task and have it block on a latch
• start a second task with a short timeout and verify it times out (because the thread pool is fully occupied)
• unblock the first task and clean everything up

What do you think?

Sounds good to me. thanks!

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/21118/
• CommitID: 951ff07
  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 (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/21119/
• CommitID: 8c5f7e0
  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 (Jenkins) Run Completed with:

• RESULT: UNSTABLE ❕
• TEST FAILURES:

      1 org.opensearch.index.engine.NRTReplicationReaderManagerTests.testUpdateSegmentsWhileRefreshing

• URL: https://build.ci.opensearch.org/job/gradle-check/21130/
• CommitID: 8c5f7e0
  Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[codecov]

Codecov Report

Merging #8901 (8c5f7e0) into main (91bc891) will decrease coverage by 0.06%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #8901      +/-   ##
============================================
- Coverage     71.05%   70.99%   -0.06%     
  Complexity    57198    57198              
============================================
  Files          4758     4758              
  Lines        269863   269863              
  Branches      39484    39484              
============================================
- Hits         191755   191596     -159     
- Misses        61982    62128     +146     
- Partials      16126    16139      +13     

see 473 files with indirect coverage changes