On Sat, Aug 13, 2011 at 11:20 AM, Christian Schneider
<ch...@die-schneider.net> wrote:
> 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.
>
ThreadPoolProfile is a tiny abstraction where people can configure
settings for thread pools, and enlist them in the
ExecutorServiceManager. A ThreadPoolProfile is a pure Camel API, and
there is no such thing in the JDK.
And ThreadPoolProfile is a concept the ExecutorServiceManager only
knows about, as it manages the registered ThreadPoolProfiles. So the
ThreadPoolProfile is associated with the ExecutorServiceManager, and
not other places in the code base.
We should not force people to use the concept of ThreadPoolProfile
when they work with thread pools. Knowing the API of the JDK should be
sufficient. They can chose to use a ThreadPoolProfile if they want, or
just using what they already know from the JDK API. People who use
thread pools should be able to use the JDK API and an API that reflect
the API from the JDK which their are familiar with.
So for example a ThreadPoolBuilder should create thread pools (aka
ExecutorService in JDK terms).
And not ThreadPoolProfiles. That is confusing.
And btw the default values is not spread over the whole code.
Try to put things in perspective!
> 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.
>
The threads DSL should at least use the API of the JDK that people are
familiar with.
It already support a thread pool profile as you can refer to it
<threads executorServiceRef="myProfileId">
...
And from Java DSL
.threads().executorServiceRef("myProfileId")
> 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.
>
Well your changes broke several things and made Camel 2.9 not
backwards compatible.
Frankly your bigger refactorings dont belong in a minor release, but
in a new major release such as Camel 3.0.
Also you should respect that Camel 2.x is 2+ years old and we should
keep the API stable so people can rely on a stable and prove branch of
the Apache Camel product. We cant go around changing the API all the
time.
For example to use ExecutorServiceManager the API was now totally
changed from a JDK API
which people are familiar with, to a bit confusing API with a ThreadPoolProfile.
To use the API they now have to import 2 extra classes from 2 other packages
- ThreadPoolProfile
- ThreadPoolBuilder
Just to for example to create a background task in their component.
Also it was not clear anymore how to create a fixed, cached, or single
thread pool anymore.
The ThreadPoolBuilder did not make that straight forward.
With the old API they need *0* extra imports and can just do
getExecutorServiceStrategy().newScheduledThreadPool(this, "MyBackgroundTask");
And from trunk they still need *0* extra imports and can just do
(method names uses the naming convention from the JDK)
getExecutorServiceManager().newSingleThreadScheduledExecutor(this,
"MyBackgroundTask");
With your API that is no longer possible and they would need to
understand what the heck to do, and to go read about a
ThreadPoolProfile and how to create that.
So if Camel makes this confusing and not easy, then people will stop
using the Camel API and just go for plain JDK.
That works and is compatible in all JDKs and Camel versions.
Its is very important for the Apache Camel project to ensure 3rd party
Camel components is as much compatible in the 2.x line.
In fact when we discussed Camel 3.0 API, we talked about the
importance of 3rd party Camel components being able to run out of the
box. So any API changes should be taken with care when it Camel to
components.
> I am +0 for having the ExecutorServiceStrategy for backwards compatibilty. I
> do not think it is necessary but it does not hurt.
>
About 10-15 Camel components was affected by this change. Dont you
think 3rd party components would have been as well?
Changing the DSL is a bit more easier as people compile the Camel
applications with the version they use.
But people use 3rd party components from camel-extra, githib, inhouse
developed, etc. And with a non backwards change, they would have to
support multiple branches to support Camel 2.8 or older. And Camel
2.9+. That dont give them confidence in the Apache Camel project.
And it is possible to keep the old API in place and mark it
@deprecated and give people amble time to adjust if we really want to
remove the deprecated API. In fact know its a just a facade which
delegates to the new ExecutorServiceManager who does the real work.
Also a testimony that the new API does indeed fully cover the old API
as the old unit test was copied and it passes.
> Christian
>
All together Christian I think you should create a wiki page with your
ideas for API changes in this area.
And then we can look at this for Camel 3.0 where the API is open for
changes. Not in a minor Camel release such as 2.8 -> 2.9.
And especially now that Camel 2.x is 2+ years old and the API should
be stable so people can keep enjoy using Camel.
>
> 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
> 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/
