On Thu, Aug 18, 2011 at 1:25 PM, Christian Schneider
<ch...@die-schneider.net> wrote:
> Hi Claus,
>
> I just saw that you also changed the ThreadPoolFactory interface to not use
> the profiles and instead have several methods again. This completely defeats
> the purpose of having a small factory interface.
> I will revert that back.
>
The ThreadPoolFactory has an API that is fully non Camel specific.
There is no Camel API at all in that interface.
package org.apache.camel.spi;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.RejectedExecutionHandler;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ThreadFactory;
import java.util.concurrent.TimeUnit;
/**
* Factory to crate {@link ExecutorService} and {@link
ScheduledExecutorService} instances
* <p/>
* This interface allows to customize the creation of these objects to
adapt Camel
* for application servers and other environments where thread pools should
* not be created with the JDK methods, as provided by the {@link
org.apache.camel.impl.DefaultThreadPoolFactory}.
*
* @see ExecutorServiceManager
*/
public interface ThreadPoolFactory {
It relies purely on well known JDK terms for thread pools and thus
makes it much easier for 3rd party to implement a custom factory if
they need.
All 4 methods are similar to the JDK Executors API and therefore easy
for people to understand and implement.
Your API was a Camel thingy with the ThreadPoolProfile (API from
Camel) and thus people would have to drag Camel API in their custom
implementations. Likewise when people needed a cached thread pool,
then the ThreadPoolProfile would not be able to indicate that.
I would oppose against putting Camel's API into the SPI when its not necessary.
> I worked on these changes quite a long time so the fact that you simply
> changed things back after we discussed on them and agreed on the changes
> makes me a bit sad. It also causes me a lot of unplanned work now. I agree
> with you on some of the things you mentioned.
> Like for example that it makes sense to offer an API on the
> ExecutorServiceManager that people are familiar with. So I think using the
> almost same API as in java is a good thing. I also like the fact that the
> change on the components is now really small after your change and also that
> there is a completely compatible ExecutorServiceStrategy as a fallback. That
> really makes sense for all external components.
>
> The problem is that with your changes you also rolled back good things I did
> that I now have to spend a lot of time bringing in again.
>
The trunk is not an experimental branch where people can commit big
refactorings or changes that break backwards compatible.
Likewise there is nobody in the community that requires a change in
the ExecutorServiceStrategy API. This is something you
came up with because you wanted to adjust the API. This is sometimes
tempting to do, but is very dangerous. The API should be kept
stable, especially for Camel 2.x that is 2+ years old, and for a
feature that has been in there for 18+ months.
We have Camel 3.0 where the API can be more open for changes.
> Christian
>
> Am 12.08.2011 17:29, schrieb Claus Ibsen:
>>
>> Hi
>>
>> Recently the ExecutorServiceStrategy had a bigger refactor
>> (https://issues.apache.org/jira/browse/CAMEL-4244)
>> 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.
>> https://issues.apache.org/jira/browse/CAMEL-4328
>>
>> 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
> http://www.liquid-reality.de
>
> Open Source Architect
> Talend Application Integration Division http://www.talend.com
>
>
--
Claus Ibsen
-----------------
FuseSource
Email: cib...@fusesource.com
Web: http://fusesource.com
Twitter: davsclaus, fusenews
Blog: http://davsclaus.blogspot.com/
Author of Camel in Action: http://www.manning.com/ibsen/
