-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
fix explain action on query rewrite #17286
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
if (request.query() == null) { | ||
rewriteListener.onResponse(request.query()); |
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.
This completes the rewriteListener
with null
. I'm not sure that's what you intend. Following the code path it looks like it just eventually calls super.doExecute(task, request, listener);
so perhaps it's better to put that line here rather than chaining the listener.
That would allow you to simplify your code by just putting the wrapped ActionListener in the rewriteAndFetch()
branch.
❌ 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? |
Signed-off-by: Fen Qin <[email protected]>
❌ 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? |
Description
There is an exception when call explain in "by_doc_id" mode. Response looks like this:
The error were caught from
TransportExplainAction -> Rewriteable.java
because there still be asyncActions left when the flagassertNoAsyncTasks
returned as true.The conflict is down to:
The NeuralQueryBuilder or TermsQueryBuilder
doRewrite
method introduces asynchronous actions viaqueryRewriteContext.registerAsyncAction
. So it then returns a context with potentially unresolved async tasks.Rewritable.rewrite
then checksassertNoAsyncTasks
flag, throws the exception if there exists any async tasks left.Fix in this PR is resolve this conflict:
TransportExplainAction.
TermsQueryBuilder
Related Issues
Resolves #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.