Skip to content
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

getRemoteClusterClient does not need a ThreadPool #104557

Conversation

DaveCTurner
Copy link
Contributor

@DaveCTurner DaveCTurner commented Jan 18, 2024

Following #104536 this argument is now unused.

We necessarily have access to a `TransportService` when building a
remote-cluster client, and the `TransportService` itself has a
`ThreadPool`, so there's no need to pass in a specific `ThreadPool` when
building a remote-cluster client.

Relates elastic#104536
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Jan 18, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@rjernst
Copy link
Member

rjernst commented Jan 18, 2024

What is the harm in being explicit here? Piggy-backing access to services from other services has made refactoring in the past especially difficult.

@DaveCTurner
Copy link
Contributor Author

Ultimately there's no need for a remote-cluster client to have a threadpool at all, see #104536, this is really just to pre-empt some of the noise from that PR.

In fact org.elasticsearch.client.internal.support.AbstractClient#threadPool is itself only there to pass on to other services, it's not used within the client at all. I'd love to get rid of that too.

@DaveCTurner DaveCTurner requested a review from rjernst January 18, 2024 20:43
@rjernst
Copy link
Member

rjernst commented Jan 18, 2024

The number of places extracting threadpool from client is daunting...though the two main uses seem to be (from a skim of findings in intellij) constructing another client, and getting at the threadcontext. For the latter, I wonder if we should rethink how to get at threadcontext. It's only loosely related to threadpool, in that we create a single threadpool, and it has to do with threading. You could imagine the threadcontext itself being available as a static or threadlocal, or better yet, something like the new ScopedValues that are in preview in newer versions of Java.

With that all said, I realize the above is not trivial, so I think this is ok as a stepping stone.

@DaveCTurner DaveCTurner added auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) and removed auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) labels Jan 18, 2024
@DaveCTurner
Copy link
Contributor Author

On reflection if we do #104536 first then that removes the objections to this change, because that PR renders this ThreadPool arg unused.

@DaveCTurner DaveCTurner added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Jan 22, 2024
@elasticsearchmachine elasticsearchmachine merged commit 2d073f4 into elastic:main Jan 22, 2024
15 checks passed
@DaveCTurner DaveCTurner deleted the 2024/01/18/getRemoteClusterClient-threadpool branch January 22, 2024 10:51
@DaveCTurner DaveCTurner restored the 2024/01/18/getRemoteClusterClient-threadpool branch June 17, 2024 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Core/Infra/Transport API Transport client API >refactoring Team:Core/Infra Meta label for core/infra team v8.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants