On Thu, Aug 18, 2011 at 1:50 PM, Claus Ibsen <claus.ib...@gmail.com> wrote:
> 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.
>
That said, since ThreadPoolFactory is a *new* API, then I would be +0
if you changed it to
your needs to make this as a compromise. However I personally thing
that using the JDK terms
is the best.
Likewise the ThreadPoolProfile is a simple getter/setter bean, and
people can easily configure it as such.
So I dont think it needs a builder either.
ThreadPoolProfile profile = new ...
profile.setQueueSize(200);
profile.setXXX
profile.setYYY
And in light of this I think the ThreadPoolFactory should be kept as
is, its using the JDK terms.
Then people can use the ExecutorServiceManager to create a thread pool
from their ThreadPoolProfile.
There is already API for that.
ExecutorService newThreadPool(Object source, String name,
ThreadPoolProfile profile);
It may lack a newScheduledThreadPool method.
So all together what I could see being changed
- the API of ThreadPoolFactory since its a *new* API. However I prefer
it to not use Camel API but rely on JDK terms
- add newScheduledThreadPool to ExecutorServiceManager to create a
scheduler from a ThreadPoolProfile
I do not see a reason for changing/adding
- ThreadPoolBuilder
- ThreadPoolProfileBuilder (as ThreadPoolProfile is a simple get/set
bean, and thus very easy to build manually)
>
>
>> 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/
>
--
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/
