fix explain action on query rewrite#17286
Conversation
|
❌ Gradle check result for 3b159cd: FAILURE 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? |
There was a problem hiding this comment.
This looks like a re-submittal of #17277 which has unresolved comments. Please reference earlier PRs when re-submitting them so that reviewers have the full context of previous comments (and can make sure they are resolved).
Code looks like it addresses the problem, but it's overly complex chaining an action listener with a null that gets ignored. Suggest moving the null check up to the top of the method and calling super.doExecute() (basically the existing code, but wrapped in a null check with a shortcut return before creating more objects). Only if the query is non-null do you need to instantiate the actionListener and chain that code.
Also looks like some checks are failing like DCO and Change log.
server/src/main/java/org/opensearch/action/explain/TransportExplainAction.java
Outdated
Show resolved
Hide resolved
|
❌ Gradle check result for 2dcc19b: FAILURE 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? |
|
❌ Gradle check result for a111f2f: 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? |
|
❌ Gradle check result for 9e7e026: FAILURE 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? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17286 +/- ##
============================================
- Coverage 72.47% 72.46% -0.01%
- Complexity 65693 65697 +4
============================================
Files 5304 5304
Lines 304981 304990 +9
Branches 44238 44239 +1
============================================
- Hits 221033 221023 -10
+ Misses 65808 65792 -16
- Partials 18140 18175 +35 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Fen Qin <mfenqin@amazon.com>
Signed-off-by: Fen Qin <mfenqin@amazon.com>
|
❕ Gradle check result for a5778cb: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
dbwiddis
left a comment
There was a problem hiding this comment.
LGTM with some style nits that I won't hold up approval on.
server/src/internalClusterTest/java/org/opensearch/explain/ExplainActionIT.java
Show resolved
Hide resolved
server/src/internalClusterTest/java/org/opensearch/explain/ExplainActionIT.java
Show resolved
Hide resolved
…earch-project#17286) Signed-off-by: Fen Qin <mfenqin@amazon.com>
…earch-project#17286) Signed-off-by: Fen Qin <mfenqin@amazon.com> Signed-off-by: Vinay Krishna Pudyodu <vinkrish.neo@gmail.com>
Description
There is an exception when call explain in "by_doc_id" mode. Response looks like this:
The error were caught from
TransportExplainAction -> Rewriteable.javabecause there still be asyncActions left when the flagassertNoAsyncTasksreturned as true.The conflict is down to:
The NeuralQueryBuilder or TermsQueryBuilder
doRewritemethod introduces asynchronous actions viaqueryRewriteContext.registerAsyncAction. So it then returns a context with potentially unresolved async tasks.Rewritable.rewritethen checksassertNoAsyncTasksflag, throws the exception if there exists any async tasks left.Fix in this PR is resolve this conflict:
TransportExplainAction.TermsQueryBuilderRelated Issues
Resolves - opensearch-project/neural-search#1126
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.