Remove some unused dependencies from Reindex transport actions #104307
Conversation
We didn't use the cluster state in one spot, from that it followed that we can do away with the clusterservice as a dependency in many spots. Also we already pass around clients that reference the thread-pool, no need to separately pass it around either.
original-brownbear
added
>non-issue
:Distributed Indexing/Reindex
elasticsearchmachine
added
Team:Distributed (Obsolete)
|
Pinging @elastic/es-distributed (Team:Distributed) |
| @@ -167,7 +161,7 @@ public abstract class AbstractAsyncBulkByScrollAction< | |||
| this.logger = logger; | |||
| this.searchClient = searchClient; | |||
| this.bulkClient = bulkClient; | |||
| this.threadPool = threadPool; | |||
| this.threadPool = searchClient.threadPool(); | |||
There was a problem hiding this comment.
Is there a difference between the searchClient.threadPool() and choosing bulkClient.threadPool()?
There was a problem hiding this comment.
No not really :) it's always the same instance.
There was a problem hiding this comment.
Would be nice to add an assertion that their thread pools are the same just to ensure
There was a problem hiding this comment.
I think that is org.elasticsearch.threadpool.ThreadPool that allows to access any actual ExecutorService.
Not sure I like this change. It hides the fact that we depend on the ThreadPool and instead "steals" it from the other dependency. Hypothetically, what happens if we change searchClient to not have the threadPool or at least not expose it any more?
There was a problem hiding this comment.
After seeing @idegtiarenko 's comment, I'm removing my approval, just to confirm the implications around thread pool is OK.
@original-brownbear , checking also other actions, I see that thread pool is indeed passed around a lot, even if there's a client argument. I am unaware of why there's both passed, if conceptually we could have taken the thread pool from the client. I just would like to ensure it's not a problem as well. I cannot think of any problems at the moment, only that perhaps the thread pools might be different (a different one is passed, different than the client's), but I'm not sure that's happening anywhere?
There was a problem hiding this comment.
LGTM but it's hard for me to understand from this PR where the previous threadPool argument was coming from in all the involved actions, so I trust you @original-brownbear when saying it's equivalent to use the client's thread pool. If you're not 100% certain, please tell me to look further or seek more reviewers.
| @@ -167,7 +161,7 @@ public abstract class AbstractAsyncBulkByScrollAction< | |||
| this.logger = logger; | |||
| this.searchClient = searchClient; | |||
| this.bulkClient = bulkClient; | |||
| this.threadPool = threadPool; | |||
| this.threadPool = searchClient.threadPool(); | |||
There was a problem hiding this comment.
Would be nice to add an assertion that their thread pools are the same just to ensure
| @@ -53,7 +53,6 @@ public TransportEnrichReindexAction( | |||
| super( | |||
| EnrichReindexAction.NAME, | |||
| settings, | |||
| threadPool, | |||
There was a problem hiding this comment.
You can remove it from the constructor as well then?
kingherc
dismissed
their stale review
Removing approval until we're completely sure we should access the thread pool from the client
There was a problem hiding this comment.
There is only one threadpool instance in a production node. However, I think it's a bug that the Client carries a threadpool along with it, I've been looking at removing that dependency, so I'd rather not add more usages as suggested here. Let's keep on passing it around explicitly please.
elasticsearchmachine
added
v9.1.0
Team:Distributed Indexing
|
Pinging @elastic/es-distributed-obsolete (Team:Distributed (Obsolete)) |
|
Pinging @elastic/es-distributed-indexing (Team:Distributed Indexing) |
We didn't use the cluster state in one spot, from that it followed that we can do away with the clusterservice as a dependency in many spots. Also we already pass around clients that reference the thread-pool, no need to separately pass it around either.