-
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 #17277
Conversation
Signed-off-by: Fen Qin <“[email protected]”>
@@ -101,7 +104,21 @@ public TransportExplainAction( | |||
@Override | |||
protected void doExecute(Task task, ExplainRequest request, ActionListener<ExplainResponse> listener) { | |||
request.nowInMillis = System.currentTimeMillis(); | |||
super.doExecute(task, request, listener); | |||
// super.doExecute(task, request, listener); |
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.
I think you want to remove this line.
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.
sure, will remove this line in next revision
super.doExecute(task, request, listener); | ||
// super.doExecute(task, request, listener); | ||
LongSupplier timeProvider = () -> request.nowInMillis; | ||
ActionListener<QueryBuilder> rewriteListener = ActionListener.wrap(rewrittenQuery -> { |
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.
Why don't we simply switch from rewrite to rewriteAndFetch since now know this process can have async actions? Now basically we are doing rewriteAndFetch here and later I think we will still will do rewrite again.
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.
here is Rewriteable javadoc - https://artifacts.elastic.co/javadoc/org/elasticsearch/elasticsearch/8.17.1/org.elasticsearch.server/org/elasticsearch/index/query/Rewriteable.html
-
rewrite
-
Rewrites each element of the list until it doesn't change and returns a new list iff there is at least one element of the list that changed during it's rewrite. -
rewriteAndFetch
-
Rewrites the given rewriteable and fetches pending async tasks for each round before rewriting again. -
so we would need
rewrite
to be executed here
❌ Gradle check result for b1f53c0: 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 11ebc0d: 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? |
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
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.
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.