Support task resource tracking in OpenSearch #3982
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3982 +/- ##
============================================
+ Coverage 70.50% 70.71% +0.20%
- Complexity 56848 57041 +193
============================================
Files 4583 4585 +2
Lines 273931 274122 +191
Branches 40158 40178 +20
============================================
+ Hits 193146 193845 +699
+ Misses 64561 64001 -560
- Partials 16224 16276 +52 ☔ View full report in Codecov by Sentry. |
Yes, I'm working towards running performance benchmarks. |
Bukhtawar
reviewed
Jul 25, 2022
Gradle Check (Jenkins) Run Completed with:
|
Bukhtawar
reviewed
Jul 27, 2022
Gradle Check (Jenkins) Run Completed with:
|
Reopens changes from opensearch-project#2639 (reverted in opensearch-project#3046) to add a framework for task resource tracking. Currently, SearchTask and SearchShardTask support resource tracking but it can be extended to any other task. Changes since opensearch-project#2639: * Replaced the usage of AutoQueueAdjustingExecutorBuilder with ResizableExecutorBuilder * Resolved merge conflicts * Fixed broken tests Signed-off-by: Ketan Verma <[email protected]>
…re stopped Signed-off-by: Ketan Verma <[email protected]>
Signed-off-by: Ketan Verma <[email protected]>
Signed-off-by: Ketan Verma <[email protected]>
ketanv3
force-pushed
the
feature/resource-tracking-framework
branch
from
dcdaf6e to
a09a60a
Compare
Gradle Check (Jenkins) Run Completed with:
|
|
Comparing the accuracy of both approaches – (1) no waiting for task's threads to complete, and (2) using a callback to keep track of active threads – before marking the task as unregistered. An existing integration test was re-used to perform large number of search requests with predictable CPU/memory usage. Measurements were taken for:
Both tests executed 500 search queries and reported resource usages for ~9020 tasks.
Based on these results, approach (2) has been used for the implementation as it gives better accuracy. |
nssuresh2007
reviewed
Aug 1, 2022
nssuresh2007
approved these changes
Aug 1, 2022
Bukhtawar
reviewed
Aug 1, 2022
Gradle Check (Jenkins) Run Completed with:
|
Benchmark results
Baseline commit: 740f75d |
…ion listener Signed-off-by: Ketan Verma <[email protected]>
ketanv3
force-pushed
the
feature/resource-tracking-framework
branch
from
4e77476 to
d89de65
Compare
Gradle Check (Jenkins) Run Completed with:
|
Bukhtawar
reviewed
Aug 2, 2022
Comment on lines
415
to
417
| public void addResourceTrackingCompletionListener(NotifyOnceListener<Task> listener) { | ||
| resourceTrackingCompletionListeners.add(listener); | ||
| } |
There was a problem hiding this comment.
We shouldn't addResourceTrackingCompletionListener if the count is zero, else its possible to that newly added listener is never called.
There was a problem hiding this comment.
Fair point, updated.
There was a problem hiding this comment.
Though there is still a rare possibility of race-condition:
- Task resource tracking is completed (num threads = 0), and existing completion listeners are invoked.
- Delayed thread execution starts for the task (num threads = 1)
- New completion listener added at this point may succeed.
- Delayed thread execution stops for the task (num threads = 0)
- New completion listener is invoked.
To solve this, we may have to bring back the isResourceTrackingCompleted atomic boolean into the Task. It's not a concern at the moment as listeners are added during Task creation, not in the middle of execution.
There was a problem hiding this comment.
On a different note, this may not even be a problem because a (delayed) thread is still a part of the task, and the newly added listener would just receive the more recent/accurate usage stats.
There was a problem hiding this comment.
Yeah we haven't also strictly synchronised adding listeners and invoking them, so I am fine with this limitation as long as it doesn't overcomplicate the use case
Signed-off-by: Ketan Verma <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-3982-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 5eac54d4ade73f6d0ed80b0f2408a104a98e3232
# Push it to GitHub
git push --set-upstream origin backport/backport-3982-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.xThen, create a pull request where the |
ketanv3
added a commit
to ketanv3/OpenSearch
that referenced
this pull request
Aug 2, 2022
* Support task resource tracking in OpenSearch * Reopens changes from opensearch-project#2639 (reverted in opensearch-project#3046) to add a framework for task resource tracking. Currently, SearchTask and SearchShardTask support resource tracking but it can be extended to any other task. * Fixed a race-condition when Task is unregistered before its threads are stopped * Improved error handling and simplified task resource tracking completion listener * Avoid registering listeners on already completed tasks Signed-off-by: Ketan Verma <[email protected]>
Bukhtawar
pushed a commit
that referenced
this pull request
Aug 2, 2022
* [Backport 2.x] Support task resource tracking in OpenSearch * Reopens changes from #2639 (reverted in #3046) to add a framework for task resource tracking. Currently, SearchTask and SearchShardTask support resource tracking but it can be extended to any other task. * Fixed a race-condition when Task is unregistered before its threads are stopped * Improved error handling and simplified task resource tracking completion listener * Avoid registering listeners on already completed tasks Signed-off-by: Ketan Verma <[email protected]>
PritLadani
added a commit
to PritLadani/OpenSearch
that referenced
this pull request
Sep 6, 2022
Backporting pull requests opensearch-project#2089 and opensearch-project#3982 Signed-off-by: PritLadani <[email protected]>
This was referenced Nov 9, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Reopens changes from #2639 (reverted in #3046) to add a framework for task resource tracking.
Currently, SearchTask and SearchShardTask support resource tracking but it can be extended to any other task in the future.
Changes since #2639:
Signed-off-by: Ketan Verma [email protected]
Issues Resolved
#1179
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.