Skip to content

[ML] Ensuring only a single request executor object is created#133424

Merged
jonathan-buttner merged 5 commits intoelastic:mainfrom
jonathan-buttner:ml-fix-request-executor-thread-bug
Aug 27, 2025
Merged

[ML] Ensuring only a single request executor object is created#133424
jonathan-buttner merged 5 commits intoelastic:mainfrom
jonathan-buttner:ml-fix-request-executor-thread-bug

Conversation

@jonathan-buttner
Copy link
Contributor

@jonathan-buttner jonathan-buttner commented Aug 23, 2025

This PR makes sure only a single HttpRequestSender and RequestExecutor object is created. There's been a bug where the first request to each integration would start a new RequestExecutorService which would cause new threads to be scheduled.

@jonathan-buttner jonathan-buttner added >non-issue :ml Machine learning Team:ML Meta label for the ML team auto-backport Automatically create backport pull requests when merged v9.0.4 v9.2.0 v8.17.11 v8.18.6 v8.19.3 v9.1.4 labels Aug 23, 2025
@jonathan-buttner jonathan-buttner marked this pull request as ready for review August 25, 2025 15:55
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/ml-core (Team:ML)

@elasticsearchmachine
Copy link
Collaborator

Hi @jonathan-buttner, I've created a changelog YAML for you.

}

public Sender createSender() {
return httpRequestSender;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't really need this method or the class anymore. We can do additional refactoring but I wanted to keep the fix in a small PR. This method is used a lot throughout the tests.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah maybe we should (eventually) replace this whole class with a Lazy or a SetOnce so we don't have to have a countdown latch in a constructor, but I see that we do that in SenderService regardless so there's no change there I suppose

}

public Sender createSender() {
return httpRequestSender;
Copy link
Member

Choose a reason for hiding this comment

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

Yeah maybe we should (eventually) replace this whole class with a Lazy or a SetOnce so we don't have to have a countdown latch in a constructor, but I see that we do that in SenderService regardless so there's no change there I suppose

Copy link
Member

@davidkyle davidkyle left a comment

Choose a reason for hiding this comment

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

LGTM

@jonathan-buttner jonathan-buttner merged commit d5a9343 into elastic:main Aug 27, 2025
33 checks passed
jonathan-buttner added a commit to jonathan-buttner/elasticsearch that referenced this pull request Aug 27, 2025
…ic#133424)

* Ensuring only a single request executor thread is started

* Reverting test changes

* Update docs/changelog/133424.yaml
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
9.0 Commit could not be cherrypicked due to conflicts
8.17 Commit could not be cherrypicked due to conflicts
9.1
8.18 Commit could not be cherrypicked due to conflicts
8.19

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

elasticsearchmachine pushed a commit that referenced this pull request Aug 27, 2025
…) (#133670)

* Ensuring only a single request executor thread is started

* Reverting test changes

* Update docs/changelog/133424.yaml
elasticsearchmachine pushed a commit that referenced this pull request Aug 27, 2025
…) (#133669)

* Ensuring only a single request executor thread is started

* Reverting test changes

* Update docs/changelog/133424.yaml
jonathan-buttner added a commit to jonathan-buttner/elasticsearch that referenced this pull request Aug 28, 2025
…ic#133424)

* Ensuring only a single request executor thread is started

* Reverting test changes

* Update docs/changelog/133424.yaml

(cherry picked from commit d5a9343)

# Conflicts:
#	x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/external/amazonbedrock/AmazonBedrockRequestSenderTests.java
jonathan-buttner added a commit to jonathan-buttner/elasticsearch that referenced this pull request Aug 28, 2025
…ic#133424)

* Ensuring only a single request executor thread is started

* Reverting test changes

* Update docs/changelog/133424.yaml

(cherry picked from commit d5a9343)

# Conflicts:
#	x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/external/amazonbedrock/AmazonBedrockRequestSenderTests.java
@jonathan-buttner
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
9.0
8.18
8.17

Questions ?

Please refer to the Backport tool documentation

jonathan-buttner added a commit to jonathan-buttner/elasticsearch that referenced this pull request Aug 28, 2025
…ic#133424)

* Ensuring only a single request executor thread is started

* Reverting test changes

* Update docs/changelog/133424.yaml

(cherry picked from commit d5a9343)

# Conflicts:
#	x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/external/http/sender/HttpRequestSender.java
#	x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/external/amazonbedrock/AmazonBedrockRequestSenderTests.java
elasticsearchmachine pushed a commit that referenced this pull request Aug 28, 2025
…) (#133715)

* Ensuring only a single request executor thread is started

* Reverting test changes

* Update docs/changelog/133424.yaml

(cherry picked from commit d5a9343)

# Conflicts:
#	x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/external/amazonbedrock/AmazonBedrockRequestSenderTests.java
elasticsearchmachine pushed a commit that referenced this pull request Aug 28, 2025
…) (#133716)

* Ensuring only a single request executor thread is started

* Reverting test changes

* Update docs/changelog/133424.yaml

(cherry picked from commit d5a9343)

# Conflicts:
#	x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/external/amazonbedrock/AmazonBedrockRequestSenderTests.java
elasticsearchmachine pushed a commit that referenced this pull request Aug 28, 2025
…) (#133721)

* Ensuring only a single request executor thread is started

* Reverting test changes

* Update docs/changelog/133424.yaml

(cherry picked from commit d5a9343)

# Conflicts:
#	x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/external/http/sender/HttpRequestSender.java
#	x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/external/amazonbedrock/AmazonBedrockRequestSenderTests.java
sarog pushed a commit to portsbuild/elasticsearch that referenced this pull request Sep 11, 2025
…ic#133424) (elastic#133670)

* Ensuring only a single request executor thread is started

* Reverting test changes

* Update docs/changelog/133424.yaml
sarog pushed a commit to portsbuild/elasticsearch that referenced this pull request Sep 19, 2025
…ic#133424) (elastic#133670)

* Ensuring only a single request executor thread is started

* Reverting test changes

* Update docs/changelog/133424.yaml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Automatically create backport pull requests when merged >bug :ml Machine learning Team:ML Meta label for the ML team v8.17.11 v8.18.7 v8.19.4 v9.0.4 v9.1.4 v9.2.0

5 participants