OAK-12054: Refactor creation of ThreadPoolExecutors #2679
Conversation
.../src/main/java/org/apache/jackrabbit/oak/commons/internal/concurrent/NamedThreadFactory.java
Outdated
Show resolved
Hide resolved
oak-commons/src/main/java/org/apache/jackrabbit/oak/commons/concurrent/NamedThreadFactory.java
Outdated
Show resolved
Hide resolved
oak-commons/src/main/java/org/apache/jackrabbit/oak/commons/concurrent/ExecutorHelper.java
Outdated
Show resolved
Hide resolved
oak-commons/src/main/java/org/apache/jackrabbit/oak/commons/concurrent/ExecutorHelper.java
Outdated
Show resolved
Hide resolved
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/index/IndexHelper.java
Outdated
Show resolved
Hide resolved
...ic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexStatistics.java
Outdated
Show resolved
Hide resolved
oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/ExtractedTextCache.java
Outdated
Show resolved
Hide resolved
|
@bhabegger good catch, thanks for the PR; can you create a dedicated OAK ticket for it? |
bhabegger
force-pushed
the
fix-executor-service
branch
from
4318b53 to
e61cba0
Compare
Well, in fact it was hinted to me by @thomasmueller as a pending task that I took for my onboarding :) So, I don't have much credit for catching this;) |
bhabegger
force-pushed
the
fix-executor-service
branch
from
e61cba0 to
33271ab
Compare
bhabegger
changed the title
bhabegger
force-pushed
the
fix-executor-service
branch
from
33271ab to
eb16e23
Compare
| * ThreadFactory which names the threads based on a pattern. The pattern should contain a %d which is replaced by | ||
| * a unique number for each thread generated by this factory. | ||
| */ | ||
| public class NamedThreadFactory implements ThreadFactory { |
There was a problem hiding this comment.
Why not use the BasicThreadFactory from commons-lang3 ?
There was a problem hiding this comment.
We could indeed. However, the dependency to commons-lang3 is not yet in oak-commons module and this would pull it in transitively in other modules where in many of them have it marked as "provided". Being new to this project, I'm not clear on why that could be and if it would be a problem to add the dependency.
There was a problem hiding this comment.
we should add commons-lang3 as a dependency in oak-commons. @reschke wdyt ?
There was a problem hiding this comment.
I probably wouldn't add the dependency. (I wouldn't add indirection or a dependency if the benefit is tiny, as it is here.)
There was a problem hiding this comment.
@thomasmueller commons-lang3 is already part of oak-commons but as a test dependency. So we are already using it.
bhabegger
force-pushed
the
fix-executor-service
branch
from
eb16e23 to
28c2927
Compare
thomasmueller
left a comment
thomasmueller
left a comment
There was a problem hiding this comment.
There is a risk if we change the current behavior, even if the current behavior is not what was intended originally: we could have out-of-memory, concurrency issues etc.
So either the PR should not change the current behavior, or at least, we should have a way to switch back to the old behavior, in case of issues. E.g. with a feature toggle.
Given that we don't currently see issue with the current behavior (other than it's not intended, and can confuse people, etc.), I think we should keep the current behavior.
bhabegger
force-pushed
the
fix-executor-service
branch
3 times, most recently
from
687a16b to
443d867
Compare
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/index/IndexHelper.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndexProviderService.java
Outdated
Show resolved
Hide resolved
...ic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexStatistics.java
Outdated
Show resolved
Hide resolved
oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/ExtractedTextCache.java
Outdated
Show resolved
Hide resolved
...mons/src/main/java/org/apache/jackrabbit/oak/commons/internal/concurrent/ExecutorHelper.java
Show resolved
Hide resolved
…e in fact only running one thread)
bhabegger
force-pushed
the
fix-executor-service
branch
from
5f19daa to
60ee9fa
Compare
rishabhdaim
requested review from
joerghoh,
reschke,
rishabhdaim and
thomasmueller
5 participants
Some instantiations of ThreadPoolExecutor were setting a different corePoolSize and maxPoolSize even though they were using an unbounded LinkedBlockingQueue. The pattern was repeated multiple times. This PR attempts to fix and mutualize the ThreadPoolExecutor creation code.