-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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] [2.x] [Concurrent Segment Search]: Add support for query profiler with concurrent aggregation (#9248) #9835
Conversation
Compatibility status:Checks if related components are compatible with change a2552db Incompatible componentsIncompatible components: [https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/cross-cluster-replication.git] Skipped componentsCompatible componentsCompatible components: [https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/performance-analyzer-rca.git] |
Gradle Check (Jenkins) Run Completed with:
|
a2552db
to
42bd603
Compare
Compatibility status:Checks if related components are compatible with change 42bd603 Incompatible componentsIncompatible components: [https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/neural-search.git] Skipped componentsCompatible componentsCompatible 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/observability.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/performance-analyzer-rca.git] |
Gradle Check (Jenkins) Run Completed with:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sohami @ticheng-aws holding the merge, we have NPE here #9815 and another NPE variation in this pull request, we cannot let it sneak into the release:
java.lang.NullPointerException: Cannot invoke "org.opensearch.action.search.SearchResponse.getFailedShards()" because "profileResponse" is null
at __randomizedtesting.SeedInfo.seed([E68DEFD659967F87:B55838D408DE9DFB]:0)
at org.opensearch.search.profile.query.QueryProfilerIT.testProfileMatchesRegular(QueryProfilerIT.java:195)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:568)
at com.carrotsearch.randomizedtesting.RandomizedRunner.invoke(RandomizedRunner.java:1750)
at com.carrotsearch.randomizedtesting.RandomizedRunner$8.evaluate(RandomizedRunner.java:938)
at com.carrotsearch.randomizedtesting.RandomizedRunner$9.evaluate(RandomizedRunner.java:974)
at com.carrotsearch.randomizedtesting.RandomizedRunner$10.evaluate(RandomizedRunner.java:988)
at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
@reta We looked into this bug. The NPE is happening because of an issue observed in concurrent search path of profile where rewrite is happening during execution phase as well (this use case was unknown). This issue existed even before these new changes were done but it is showing up now since we enabled the |
…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> Signed-off-by: Ticheng Lin <ticheng@amazon.com>
42bd603
to
5f1c7c4
Compare
Just realized this is a separate issue and is happening in the new code path. We will not back-port the new changes to 2.10 in that case. |
Compatibility status:Checks if related components are compatible with change 5f1c7c4 Incompatible componentsIncompatible components: [https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/neural-search.git] Skipped componentsCompatible componentsCompatible 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/observability.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/security-analytics.git] |
Gradle Check (Jenkins) Run Completed with:
|
This PR is stalled because it has been open for 30 days with no activity. |
Closed this PR as it's backported from #10898. |
(cherry picked from commit a737446)
Description
At present, the query profiler is the total timing obtained by summing all segments. However, this does not accurately represent the detailed timing of the query, including queue time and gap time, during concurrent execution. This PR will address the problem.
Related Issues
Resolves #8330 #7354
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.