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

Backport of PRs for profile related changes in concurrent search path #10898

Merged
merged 4 commits into from
Oct 31, 2023

Conversation

sohami
Copy link
Collaborator

@sohami sohami commented Oct 24, 2023

Description

This PR backports following PRs from main to 2.x as discussed here. There were no new changes needed other than updating the CHANGELOG.md file.

#9248
#10111
#10547
#10352

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)
  • Public documentation issue/PR created

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
Copy link
Contributor

github-actions bot commented Oct 24, 2023

Compatibility status:

Checks if related components are compatible with change b7bec57

Incompatible components

Incompatible components: [https://github.com/opensearch-project/performance-analyzer.git]

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/custom-codecs.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/neural-search.git]

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@sohami sohami force-pushed the backport-profile-prs branch from fa7454f to 1bd7fe1 Compare October 30, 2023 18:09
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@ticheng-aws
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

This is a known bug #10798

Tests with failures:
 - org.opensearch.search.suggest.CompletionSuggestSearchIT.testSkipDuplicates {p0={"search.concurrent_segment_search.enabled":"false"}}
 - org.opensearch.search.suggest.CompletionSuggestSearchIT.testSkipDuplicates {p0={"search.concurrent_segment_search.enabled":"true"}}

4535 tests completed, 2 failed, 34 skipped

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@sohami
Copy link
Collaborator Author

sohami commented Oct 30, 2023

DCO was failing with below. For some reason it was trying to find 51488860+ticheng-aws@users.noreply.github.com instead of ticheng@amazon.com. From commit I can see Signed-Off message has correct username. So have made DCO to pass manually.

Commit sha: [2796681](https://github.com/opensearch-project/OpenSearch/pull/10898/commits/2796681accb95c0b9345473287081171dc031e21), Author: Ticheng Lin, Committer: Sorabh Hamirwasia; Expected "Ticheng Lin [51488860+ticheng-aws@users.noreply.github.com](mailto:51488860+ticheng-aws@users.noreply.github.com)", but got "Ticheng Lin [ticheng@amazon.com](mailto:ticheng@amazon.com)".
Commit sha: [1bd7fe1](https://github.com/opensearch-project/OpenSearch/pull/10898/commits/1bd7fe1e836f466d43b2a9b70c85bc5321b2e177), Author: Ticheng Lin, Committer: Sorabh Hamirwasia; Can not find "Ticheng Lin [51488860+ticheng-aws@users.noreply.github.com](mailto:51488860+ticheng-aws@users.noreply.github.com)", in ["Ticheng Lin [ticheng@amazon.com](mailto:ticheng@amazon.com)", "Ticheng Lin [ticheng@amazon.com](mailto:ticheng@amazon.com)", "Ticheng Lin [ticheng@amazon.com](mailto:ticheng@amazon.com)"].

@sohami
Copy link
Collaborator Author

sohami commented Oct 31, 2023

Gradle Check (Jenkins) Run Completed with:

Created tracking issue for this flaky test: #11032

@sohami
Copy link
Collaborator Author

sohami commented Oct 31, 2023

@reta @ticheng-aws Can you check this PR ? It should be good to merge now.

@reta
Copy link
Collaborator

reta commented Oct 31, 2023

@reta @ticheng-aws Can you check this PR ? It should be good to merge now.

Thanks @sohami , please get the green Gradle check

@sohami
Copy link
Collaborator Author

sohami commented Oct 31, 2023

@reta @ticheng-aws Can you check this PR ? It should be good to merge now.

Thanks @sohami , please get the green Gradle check

@reta the test failure is due to shrink API. I have created a tracking issue for the same. Since the changes are in search path I don't see both to be related.

@reta
Copy link
Collaborator

reta commented Oct 31, 2023

@reta the test failure is due to shrink API. I have created a tracking issue for the same. Since the changes are in search path I don't see both to be related.

@sohami We have tons of flaky tests but the checks have to pass before merge, we specifically added the task in the description to record those.

ticheng-aws and others added 4 commits October 31, 2023 22:20
…h-project#9248)

* Add support for query profiler with concurrent aggregation (opensearch-project#9248)

Signed-off-by: Ticheng Lin <ticheng@amazon.com>

* Refactor and work on the PR comments

Signed-off-by: Ticheng Lin <ticheng@amazon.com>

* Update collectorToLeaves mapping for children breakdowns post profile metric collection and before creating the results

Signed-off-by: Sorabh Hamirwasia <sohami.apache@gmail.com>

* Refactor logic to compute the slice level breakdown stats and query level breakdown stats for concurrent search case

Signed-off-by: Sorabh Hamirwasia <sohami.apache@gmail.com>

* Fix QueryProfilePhaseTests and QueryProfileTests, and parameterize QueryProfilerIT with concurrent search enabled

Signed-off-by: Ticheng Lin <ticheng@amazon.com>

* Handle the case when there are no leaf context to compute the profile stats to return default stats
for all breakdown type along with min/max/avg values. Replace queryStart and queryEnd time with queryNodeTime

Signed-off-by: Sorabh Hamirwasia <sohami.apache@gmail.com>

* Add UTs for ConcurrentQueryProfileBreakdown

Signed-off-by: Sorabh Hamirwasia <sohami.apache@gmail.com>

* Add concurrent search stats test into the QueryProfilerIT

Signed-off-by: Ticheng Lin <ticheng@amazon.com>

* Address review comments

Signed-off-by: Sorabh Hamirwasia <sohami.apache@gmail.com>

---------

Signed-off-by: Ticheng Lin <ticheng@amazon.com>
Signed-off-by: Sorabh Hamirwasia <sohami.apache@gmail.com>
Co-authored-by: Sorabh Hamirwasia <sohami.apache@gmail.com>
…or slices (opensearch-project#10111)

* Fix NPE in ConcurrentQueryProfile while computing the breakdown map for slices.

There can be cases where one or more slice may not have timing related information for its leaves in
contexts map. During creation of slice and query level breakdown map it needs to handle such cases by using
default values correctly. Also updating the min/max/avg sliceNodeTime to not include time to create
weight and wait times by slice threads. It will reflect the min/max/avg execution time of each slice
whereas totalNodeTime will reflect the total query time.

Signed-off-by: Sorabh Hamirwasia <sohami.apache@gmail.com>

* Address review comments

Signed-off-by: Sorabh Hamirwasia <sohami.apache@gmail.com>

---------

Signed-off-by: Sorabh Hamirwasia <sohami.apache@gmail.com>
…egment search path and support concurrency during rewrite and create weight (opensearch-project#10352)

* Fix timer race condition in profile rewrite and create weight for concurrent segment search (opensearch-project#10352)

Signed-off-by: Ticheng Lin <ticheng@amazon.com>

* Refactor and work on the PR comments (opensearch-project#10352)

Signed-off-by: Ticheng Lin <ticheng@amazon.com>

---------

Signed-off-by: Ticheng Lin <ticheng@amazon.com>
@sohami sohami force-pushed the backport-profile-prs branch from 1bd7fe1 to b7bec57 Compare October 31, 2023 22:21
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.cluster.routing.allocation.decider.DiskThresholdDeciderIT.testIndexCreateBlockIsRemovedWhenAnyNodesNotExceedHighWatermarkWithAutoReleaseEnabled

Copy link

codecov bot commented Oct 31, 2023

Codecov Report

Merging #10898 (b7bec57) into 2.x (8721082) will decrease coverage by 0.13%.
Report is 1 commits behind head on 2.x.
The diff coverage is 93.50%.

@@             Coverage Diff              @@
##                2.x   #10898      +/-   ##
============================================
- Coverage     70.90%   70.77%   -0.13%     
- Complexity    58750    58768      +18     
============================================
  Files          4847     4850       +3     
  Lines        277654   277897     +243     
  Branches      40711    40734      +23     
============================================
- Hits         196868   196686     -182     
- Misses        64051    64457     +406     
- Partials      16735    16754      +19     
Files Coverage Δ
...ensearch/search/internal/ContextIndexSearcher.java 73.68% <100.00%> (+3.86%) ⬆️
...ch/search/profile/AbstractInternalProfileTree.java 98.11% <100.00%> (+0.03%) ⬆️
...earch/search/profile/AbstractProfileBreakdown.java 90.00% <100.00%> (+0.52%) ⬆️
.../java/org/opensearch/search/profile/Profilers.java 100.00% <100.00%> (ø)
...main/java/org/opensearch/search/profile/Timer.java 78.37% <100.00%> (+6.94%) ⬆️
...ofile/aggregation/AggregationProfileBreakdown.java 0.00% <ø> (ø)
...search/profile/query/InternalQueryProfileTree.java 100.00% <100.00%> (+17.64%) ⬆️
...opensearch/search/profile/query/ProfileWeight.java 78.57% <100.00%> (+1.64%) ⬆️
...opensearch/search/profile/query/QueryProfiler.java 83.33% <100.00%> (+1.51%) ⬆️
...rch/search/profile/ContextualProfileBreakdown.java 75.00% <50.00%> (-25.00%) ⬇️
... and 4 more

... and 496 files with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants