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

Change INDEX_SEARCHER threadpool to resizable to support task resource tracking #7502

Merged
merged 1 commit into from
May 17, 2023

Conversation

jed326
Copy link
Collaborator

@jed326 jed326 commented May 10, 2023

Description

Change the INDEX_SEARCHER threadpool type to QueueResizableOpenSearchThreadPoolExecutor to support task resource tracking for concurrent segment search.

  • Added new ConcurrentSearchTasksIT test to validate that multiple threads are reporting resource stats for the same task with concurrent search enabled. Note that this is separate from [Concurrent Segment Search] Enable Search ITs with Concurrent Segment Search #7440.
  • Added new UT testStartingTrackingHandlesMultipleThreadsPerTask to TaskResourceTrackingServiceTests to check that TaskResourceTrackingService properly handles multiple threads per task.

We already have existing tests for passing thread context between threadpools so I did not add any more tests related to TaskAwareRunnable.

Related Issues

#7425

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.

Resolves #7425

@jed326 jed326 force-pushed the cs-resource-tracking branch 2 times, most recently from cd88594 to 68beb41 Compare May 10, 2023 02:54
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@codecov-commenter
Copy link

codecov-commenter commented May 10, 2023

Codecov Report

Merging #7502 (3e11e38) into main (27dbcd5) will decrease coverage by 0.11%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##               main    #7502      +/-   ##
============================================
- Coverage     70.54%   70.44%   -0.11%     
+ Complexity    59713    59674      -39     
============================================
  Files          4896     4896              
  Lines        286798   286798              
  Branches      41331    41331              
============================================
- Hits         202334   202033     -301     
- Misses        67761    68098     +337     
+ Partials      16703    16667      -36     
Impacted Files Coverage Δ
...ain/java/org/opensearch/threadpool/ThreadPool.java 82.47% <0.00%> (-0.58%) ⬇️

... and 534 files with indirect coverage changes

@jed326 jed326 force-pushed the cs-resource-tracking branch 2 times, most recently from 222c08f to e8f742a Compare May 10, 2023 04:42
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.cluster.allocation.AwarenessAllocationIT.testThreeZoneOneReplicaWithForceZoneValueAndLoadAwareness

@jed326
Copy link
Collaborator Author

jed326 commented May 10, 2023

Seems like a known flaky test #7401

@jed326 jed326 force-pushed the cs-resource-tracking branch 3 times, most recently from 6c013b5 to 56790fb Compare May 15, 2023 18:19
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@reta
Copy link
Collaborator

reta commented May 17, 2023

Ah I see. If numThreads is large then the same thread may pick up multiple Runnables for the same task id. The other problem is task.getResourceStats() has a max size of the number of threads in the threadpool.

Have we figure out (and fixed) the flakiness part? thank you

@jed326
Copy link
Collaborator Author

jed326 commented May 17, 2023

@reta Yep the flakiness and your last round of comments are both addressed now.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

…e tracking

Signed-off-by: Jay Deng <jayd0104@gmail.com>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.remotestore.SegmentReplicationRemoteStoreIT.testReplicaHasDiffFilesThanPrimary
      1 org.opensearch.remotestore.SegmentReplicationRemoteStoreIT.classMethod

@reta reta merged commit 054cccd into opensearch-project:main May 17, 2023
@reta reta added the backport 2.x Backport to 2.x branch label May 17, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request May 17, 2023
…e tracking (#7502)

Signed-off-by: Jay Deng <jayd0104@gmail.com>
(cherry picked from commit 054cccd)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@reta reta added v3.0.0 Issues and PRs related to version 3.0.0 and removed backport 2.x Backport to 2.x branch labels May 17, 2023
@jed326 jed326 deleted the cs-resource-tracking branch May 18, 2023 22:03
jed326 added a commit to jed326/OpenSearch that referenced this pull request May 25, 2023
…ort task resource tracking (opensearch-project#7502)

(cherry picked from commit 054cccd)
Signed-off-by: Jay Deng <jayd0104@gmail.com>
jed326 added a commit to jed326/OpenSearch that referenced this pull request May 25, 2023
…ort task resource tracking (opensearch-project#7502)

(cherry picked from commit 054cccd)

Signed-off-by: Jay Deng <jayd0104@gmail.com>
reta pushed a commit that referenced this pull request May 25, 2023
…ort task resource tracking (#7502) (#7765)

(cherry picked from commit 054cccd)

Signed-off-by: Jay Deng <jayd0104@gmail.com>
stephen-crawford pushed a commit to stephen-crawford/OpenSearch that referenced this pull request May 31, 2023
gaiksaya pushed a commit to gaiksaya/OpenSearch that referenced this pull request Jun 26, 2023
…ort task resource tracking (opensearch-project#7502) (opensearch-project#7765)

(cherry picked from commit 054cccd)

Signed-off-by: Jay Deng <jayd0104@gmail.com>
shiv0408 pushed a commit to Gaurav614/OpenSearch that referenced this pull request Apr 25, 2024
…e tracking (opensearch-project#7502)

Signed-off-by: Jay Deng <jayd0104@gmail.com>
Signed-off-by: Shivansh Arora <hishiv@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v3.0.0 Issues and PRs related to version 3.0.0
Projects
None yet
4 participants