camel-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Christian Schneider <>
Subject Re: [HEADS UP] - Adjustments to ExecutorServiceManager on trunk
Date Sat, 13 Aug 2011 09:20:45 GMT
I am -1 on your last changes (CAMEL-4298). You effectively rolled back 
most of my changes from CAMEL-4244.

I intend to make the ThreadPoolProfile the main configuration method for 
thread pools. This makes working with defaults much more natural than 
with the more JDK style API. Your change pulls out the ThreadPoolProfile 
from most of the API. You simply configure what you need in the 
ThreadPoolProfile and the defaults aree filled in. My change also 
supported that the logic for default values is not spread over the whole 
code but concentrated in the ThreadPoolProfile class.

My next step would have been to change the threads() DSL element to only 
accept a ThreadPoolProfile for camel 3.0 so the DSL gets lighter. This 
is not possible anymore after your change. You also removed the 
ThreadPoolBuilder for profiles. I know we should rather name it 
ThreadPoolProfileBuilder (which is a bit long) but we need this to have 
a builder pattern for profiles.

We can dicuss about which changes to keep but simply reverting most of 
my changes was quite rude. I posted my changes in a jira so we can 
dicuss them and we agreed on them before I committed. I was on holidays 
during the last two weeks so I could not react earlier.

I am +0 for having the ExecutorServiceStrategy for backwards 
compatibilty. I do not think it is necessary but it does not hurt.


Am 12.08.2011 17:29, schrieb Claus Ibsen:
> Hi
> Recently the ExecutorServiceStrategy had a bigger refactor
> (
> Such as renaming the class to a better name ExecutorServiceManager.
> Also introducing a ThreadPoolFactory, ThreadFactory among others. The
> ThreadPoolFactory is a great addition, which makes it easier for 3rd
> party to just implement a custom factory, and then reply on the
> default manager.
> However the refactoring introduced a few issues such as configuring
> thread pools on EIPs was flawed. Well it also revealed we were short
> of an unit test to cover this. So I created a ticket, and committed a
> test to the 2.8 branch.
> The refactoring did also make backwards compatibility an issue, so we
> will restore that but mark the old API as @deprecated.
> The changes are summarized as follows
> - ThreadPoolFactory is the light weight and easier for SPI
> implementators to create a custom thread pool provider, such as JEE
> servers with a WorkManager API. This API has much fewer methods and
> they have been adjusted to be 100% JDK API (There is no Camel API in
> there, such as ThreadPoolProfile).
> - ExecutorServiceManager is the full fledged SPI in case you need 100%
> control. But it has Camel APIs such as ThreadPoolProfiles and a number
> of more methods. Its is encouraged to just implement a custom
> ThreadPoolFactory instead. And keep using the
> DefaultExecutorServiceManager.
> - ThreadPoolBuilder is adjusted to create a thread pool on the build
> method. This is how end users would expect. A builder to create what
> its name implies. This also removes entanglement of ThreadPool and
> ThreadPoolProfile. The latter is Camel specific and is only part of
> the ExecutorServiceManager which manges a list of profiles. To help
> uniform and make it easy to adjust thread pool settings at a higher
> level. So ThreadPoolProfiles should only be party of the manager.
> - EIPs configured using an explicit thread pool by an
> executorServiceRef, will now act as expected. If the reference could
> not be found, an exception is thrown, stating the reference is not
> found
> - Will add the ExecutorServiceStrategy SPI to have Camel 2.9 being
> backwards compatible, but mark it as @deprecated. This gives end users
> some time to adjust their custom Camel components, and source code if
> needed.
> - Naming of the methods will use the naming convention used by the
> JDK. xxxThreadPool for a pool of threads, xxxExecutor if its a single
> threaded (eg used for background tasks)
> - Made the API of ExecutorServiceManager more similar to the old
> ExecutorServiceStrategy so migrating is a breeze, usually just change
> the getExecutorServiceStrategy() to getExecutorServiceManager() and
> you are settled.
> I ran a complete unit test on my local laptop before commiting the changes.
> There is a few TODO in the code, which I will work on as well, so
> expect a few more commits. The TODO is minor/cosmetic changes I have
> spotted, that can be improved.

Christian Schneider

Open Source Architect

View raw message