Skip to content

Conversation

@idegtiarenko
Copy link
Contributor

This change forks execution to search thread after preMapper to avoid running on transport and ml threads.

This is a simpler version of #132981 that is intended to be backported to 8.19

Related to: #131799
Related to: #133312

@idegtiarenko idegtiarenko added >enhancement Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) auto-backport Automatically create backport pull requests when merged :Analytics/ES|QL AKA ESQL v8.19.0 v9.2.0 labels Aug 21, 2025
@elasticsearchmachine
Copy link
Collaborator

Hi @idegtiarenko, I've created a changelog YAML for you.

@idegtiarenko idegtiarenko marked this pull request as ready for review August 21, 2025 14:31
@idegtiarenko idegtiarenko requested review from dnhatn and nik9000 August 21, 2025 14:31
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

@idegtiarenko idegtiarenko changed the title Do not run on transport thread Do not run ES|QL planning and scheduling on transport thread Aug 21, 2025
Copy link
Member

@dnhatn dnhatn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small ask, but LGTM. Thanks @idegtiarenko.

private void queryRewrite(LogicalPlan plan, ActionListener<LogicalPlan> listener) {
QueryBuilderResolver.resolveQueryBuilders(plan, services, listener);
// see https://github.com/elastic/elasticsearch/issues/133312
// ThreadedActionListener might be removed if above issue is resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use SubscribableListener instead of ThreadedActionListener so that we only fork after going async?

        SubscribableListener<LogicalPlan> sub = new SubscribableListener<>();
        QueryBuilderResolver.resolveQueryBuilders(plan, services, sub);
        sub.addListener(listener, searchExecutor, null);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me try this

Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'm happy when Nhat's happy.

ThreadPool.Names.SEARCH,
ThreadPool.Names.SEARCH_COORDINATION,
MachineLearning.NATIVE_INFERENCE_COMMS_THREAD_POOL_NAME
ThreadPool.Names.SEARCH_COORDINATION
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@idegtiarenko idegtiarenko merged commit f1778c3 into elastic:main Aug 22, 2025
34 checks passed
@idegtiarenko idegtiarenko deleted the do_not_run_on_transport_thread branch August 22, 2025 08:54
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
8.19 Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 133313

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/ES|QL AKA ESQL auto-backport Automatically create backport pull requests when merged >enhancement Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.19.0 v9.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants