Skip to content

Conversation

@afoucret
Copy link
Contributor

#132324 introduced some check during query planning to make it sure that it is executed in one of the following thread pool: ThreadPool.Names.SEARCH, ThreadPool.Names.SEARCH_COORDINATION
ThreadPool.Names.SYSTEM_READ.

#134777 fixed a bug caused by the InferenceResolver that were using an inference threadpool to ruturn its response (implicitly because using the client listener).

This PR:

  • improves the fix for the InferenceResolver
  • ensures that the InferenceRunner will not be affected if used during the query planning (which will be the case)
  • clean some unused thread pool from tests setup

@elasticsearchmachine
Copy link
Collaborator

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

}, listener::onFailure));
final CountDownActionListener countdownListener = new CountDownActionListener(
inferenceIds.size(),
ActionListener.wrap(_r -> listener.onResponse(inferenceResolutionBuilder.build()), listener::onFailure)
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: could be simplified with

Suggested change
ActionListener.wrap(_r -> listener.onResponse(inferenceResolutionBuilder.build()), listener::onFailure)
listener.delegateFailureAndWrap((l, v) -> l.onResponse(inferenceResolutionBuilder.build()))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the advice.

I end up using delegateFailureIgnoreResponseAndWrap instead of delegateFailureIgnoreResponseAndWrap

@afoucret afoucret force-pushed the esql-inference-threadpool-consolidation branch from 76a089c to 5ac62cc Compare September 19, 2025 12:08
@afoucret afoucret requested a review from ioanatia September 22, 2025 06:13
Copy link
Member

@carlosdelest carlosdelest left a comment

Choose a reason for hiding this comment

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

LGTM

@afoucret afoucret merged commit 3dcec80 into elastic:main Sep 24, 2025
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/ES|QL AKA ESQL >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v9.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants