You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@camel.apache.org by Claus Ibsen <cl...@gmail.com> on 2011/08/12 17:29:13 UTC
[HEADS UP] - Adjustments to ExecutorServiceManager on trunk
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.
--
Claus Ibsen
-----------------
FuseSource
Email: cibsen@fusesource.com
Web: http://fusesource.com
Twitter: davsclaus, fusenews
Blog: http://davsclaus.blogspot.com/
Author of Camel in Action: http://www.manning.com/ibsen/
Re: [HEADS UP] - Adjustments to ExecutorServiceManager on trunk
Posted by Christian Schneider <ch...@die-schneider.net>.
Hi Claus,
sorry for getting back this is so late but I had to work on other
projects in the mean time.
2.9
Your ideas for camel 3.0 are rather small changes that can mostly be
done in 2.9.
> We could move ThreadPoolProfile from org.apache.camel.spi to
org.apache.camel and have it in the root package
As you wrote we can do this in 2.9. I will take care of it
> We could introduce a ThreadPoolProfileBuilder for support builder
style creation of profiles.
I already implemented this and will shortly commit. As this is a pure
addition it does not have to wait for 3.0
>The API for ExecutorServiceManagement could add support for creating
new thread pools based on a profile. Then people can use the builder
style to set the settings they want for the thread pool, and have the
ExecutorServiceManagement create the thread pool.
This already works using ExecutorService newThreadPool(Object
source, String name, ThreadPoolProfile profile);
Additionally I would like to centralize the logic for using default
profiles again. I had very nice logic in place that you rolled back. At
the moment this logic is spread over several places.
3.0
So I would rather like to discuss the more involved changes we could do
in 3.0. Of course implementation for this has to wait a bit but it is
important to start designing this now.
Some goals from my side:
- Have a common model for default thread pool profiles that works with
all dsl elements and components
- Keep the threading config out of the individual dsl elements and
components as far as possible or at least make it work in the same way
for all
- Keep the whole thread handling out of the processors and the compoents
as far as possible.
This is much more difficult than the previous step but could give
us a lot more advantages. We could try to introduce something like an
actor model so the implementations do not have to use an executorService
at all. Unfortunately I am not so experienced with this.
Christian
Am 13.08.2011 12:44, schrieb Claus Ibsen:
> I took the liberty of adding some notes to the Camel 3.0 roadmap
> https://cwiki.apache.org/confluence/display/CAMEL/Camel+3.0+-+Roadmap
>
> See the section - Improvements to ThreadPoolProfile for thread management
>
> In fact I think we could possible make a compromise with the changes
> from Christian and the old/current API in the next Camel 2.9 release.
>
>
> A minor API change IMHO could be to move the ThreadPoolProfile from
> the spi package to the root package.
> This is to empower its importance. Also its a class now, it used to be
> an interface.
> We can make it final and serializable as well.
>
> I would be okay with this API change between 2.8 -> 2.9 as no Camel
> components or 3rd party components were using them to create thread
> pools and thus no really breakage here. The XML DSL will not be
> affected as the factory beans in camel-core-xml can just be adjusted
> accordingly.
>
> However it could also be okay to leave it as is. But its a bit odd
> since ThreadPoolProfile is now a class in the SPI package. SPI is
> really only for interfaces, for 3rd party to plugin custom behavior.
>
>
> As Christian noted a bit that the ThreadPoolBuilder should have been
> named ThreadPoolProfileBuilder.
> In fact why not introduced a new ThreadPoolProfileBuilder that only
> creates profiles. And keep the existing ThreadPoolBuilder as is.
> And then people can use this ThreadPoolProfileBuilder builder to
> create profiles, which they can use the new ExecutorServiceManager to
> create thread pools. Then people can pick and chose. And there is no
> backwards compatibility issues as old and new API can co-exist.
>
>
> Also as Christian noted he wanted the threads DSL to accept a
> ThreadPoolProfile. We can add that to the fluent builder, so you can
> do this from the Java DSL. (XML DSL can already support this using the
> executorServiceRef).
>
> So in Java DSL you can do
>
> ThreadPoolProfile myProfile = ...
> .threads(myProfile)
>
>
> Will this help Christian?
>
>
--
--
Christian Schneider
http://www.liquid-reality.de
Open Source Architect
Talend Application Integration Division http://www.talend.com
Re: [HEADS UP] - Adjustments to ExecutorServiceManager on trunk
Posted by Claus Ibsen <cl...@gmail.com>.
I took the liberty of adding some notes to the Camel 3.0 roadmap
https://cwiki.apache.org/confluence/display/CAMEL/Camel+3.0+-+Roadmap
See the section - Improvements to ThreadPoolProfile for thread management
In fact I think we could possible make a compromise with the changes
from Christian and the old/current API in the next Camel 2.9 release.
A minor API change IMHO could be to move the ThreadPoolProfile from
the spi package to the root package.
This is to empower its importance. Also its a class now, it used to be
an interface.
We can make it final and serializable as well.
I would be okay with this API change between 2.8 -> 2.9 as no Camel
components or 3rd party components were using them to create thread
pools and thus no really breakage here. The XML DSL will not be
affected as the factory beans in camel-core-xml can just be adjusted
accordingly.
However it could also be okay to leave it as is. But its a bit odd
since ThreadPoolProfile is now a class in the SPI package. SPI is
really only for interfaces, for 3rd party to plugin custom behavior.
As Christian noted a bit that the ThreadPoolBuilder should have been
named ThreadPoolProfileBuilder.
In fact why not introduced a new ThreadPoolProfileBuilder that only
creates profiles. And keep the existing ThreadPoolBuilder as is.
And then people can use this ThreadPoolProfileBuilder builder to
create profiles, which they can use the new ExecutorServiceManager to
create thread pools. Then people can pick and chose. And there is no
backwards compatibility issues as old and new API can co-exist.
Also as Christian noted he wanted the threads DSL to accept a
ThreadPoolProfile. We can add that to the fluent builder, so you can
do this from the Java DSL. (XML DSL can already support this using the
executorServiceRef).
So in Java DSL you can do
ThreadPoolProfile myProfile = ...
.threads(myProfile)
Will this help Christian?
--
Claus Ibsen
-----------------
FuseSource
Email: cibsen@fusesource.com
Web: http://fusesource.com
Twitter: davsclaus, fusenews
Blog: http://davsclaus.blogspot.com/
Author of Camel in Action: http://www.manning.com/ibsen/
Re: [HEADS UP] - Adjustments to ExecutorServiceManager on trunk
Posted by Claus Ibsen <cl...@gmail.com>.
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: cibsen@fusesource.com
Web: http://fusesource.com
Twitter: davsclaus, fusenews
Blog: http://davsclaus.blogspot.com/
Author of Camel in Action: http://www.manning.com/ibsen/
Re: [HEADS UP] - Adjustments to ExecutorServiceManager on trunk
Posted by Christian Schneider <ch...@die-schneider.net>.
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.
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
http://www.talend.com
Re: [HEADS UP] - Adjustments to ExecutorServiceManager on trunk
Posted by Claus Ibsen <cl...@gmail.com>.
On Thu, Aug 18, 2011 at 1:50 PM, Claus Ibsen <cl...@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: cibsen@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: cibsen@fusesource.com
Web: http://fusesource.com
Twitter: davsclaus, fusenews
Blog: http://davsclaus.blogspot.com/
Author of Camel in Action: http://www.manning.com/ibsen/
Re: [HEADS UP] - Adjustments to ExecutorServiceManager on trunk
Posted by bvahdat <ba...@swissonline.ch>.
Like the one here:
http://svn.apache.org/viewvc?view=revision&revision=1159323
@Christian please don't blame me posting here as I simply couldn't stop
myself commenting on this.
--
View this message in context: http://camel.465427.n5.nabble.com/HEADS-UP-Adjustments-to-ExecutorServiceManager-on-trunk-tp4693698p4715665.html
Sent from the Camel Development mailing list archive at Nabble.com.
Re: [HEADS UP] - Adjustments to ExecutorServiceManager on trunk
Posted by Claus Ibsen <cl...@gmail.com>.
On Fri, Aug 19, 2011 at 2:22 PM, Christian Schneider
<ch...@die-schneider.net> wrote:
> Committed the changes.
>
> I even built before I committed to show extra professionalism :-)
>
Ah that is maybe too professional. Sometimes you have to leave easy
mistakes in for others to fix :)
> Christian
>
> Am 19.08.2011 10:00, schrieb Claus Ibsen:
>>
>> The latest commits look good.
>>
>> Only a few cosmetic changes that would be nice to do
>> - Add missing/fix javadoc to ExecutorServiceManager (its a SPI
>> interface so full javadoc shows professionalism)
>> - Add missing/fix javadoc to ThreadPoolFactory (its a SPI interface so
>> full javadoc shows professionalism)
>> - ThreadPoolProfile as you override clone() you could consider
>> implementing Cloneable interface as well
>> - The parameter name for addDefaults could imho be improved to be
>> other instead of defaultProfile2 which seems a bit odd name
>> Also the javadoc parameter is not described
>>
>>
>
>
> --
> --
> Christian Schneider
> http://www.liquid-reality.de
>
> Open Source Architect
> Talend Application Integration Division http://www.talend.com
>
>
--
Claus Ibsen
-----------------
FuseSource
Email: cibsen@fusesource.com
Web: http://fusesource.com
Twitter: davsclaus, fusenews
Blog: http://davsclaus.blogspot.com/
Author of Camel in Action: http://www.manning.com/ibsen/
Re: [HEADS UP] - Adjustments to ExecutorServiceManager on trunk
Posted by Christian Schneider <ch...@die-schneider.net>.
Committed the changes.
I even built before I committed to show extra professionalism :-)
Christian
Am 19.08.2011 10:00, schrieb Claus Ibsen:
> The latest commits look good.
>
> Only a few cosmetic changes that would be nice to do
> - Add missing/fix javadoc to ExecutorServiceManager (its a SPI
> interface so full javadoc shows professionalism)
> - Add missing/fix javadoc to ThreadPoolFactory (its a SPI interface so
> full javadoc shows professionalism)
> - ThreadPoolProfile as you override clone() you could consider
> implementing Cloneable interface as well
> - The parameter name for addDefaults could imho be improved to be
> other instead of defaultProfile2 which seems a bit odd name
> Also the javadoc parameter is not described
>
>
--
--
Christian Schneider
http://www.liquid-reality.de
Open Source Architect
Talend Application Integration Division http://www.talend.com
Re: [HEADS UP] - Adjustments to ExecutorServiceManager on trunk
Posted by Claus Ibsen <cl...@gmail.com>.
The latest commits look good.
Only a few cosmetic changes that would be nice to do
- Add missing/fix javadoc to ExecutorServiceManager (its a SPI
interface so full javadoc shows professionalism)
- Add missing/fix javadoc to ThreadPoolFactory (its a SPI interface so
full javadoc shows professionalism)
- ThreadPoolProfile as you override clone() you could consider
implementing Cloneable interface as well
- The parameter name for addDefaults could imho be improved to be
other instead of defaultProfile2 which seems a bit odd name
Also the javadoc parameter is not described
--
Claus Ibsen
-----------------
FuseSource
Email: cibsen@fusesource.com
Web: http://fusesource.com
Twitter: davsclaus, fusenews
Blog: http://davsclaus.blogspot.com/
Author of Camel in Action: http://www.manning.com/ibsen/
Re: [HEADS UP] - Adjustments to ExecutorServiceManager on trunk
Posted by Claus Ibsen <cl...@gmail.com>.
On Thu, Aug 18, 2011 at 2:07 PM, Christian Schneider
<ch...@die-schneider.net> wrote:
> Am 18.08.2011 13:50, schrieb Claus Ibsen:
>>
>> 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.
>>
>> 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.
>
> The class ThreadPoolProfile is not much more than a c struct to combine the
> attributes of a thread pool into a class.
> The implementor of a camel spi always has a dependency to the camel API as
> the spi interface is part of the API. So I see no real problem in having
> this in the interface.
> Most spi interfaces reference other camel api classes and many of those
> classes are much more problematic. The ThreadPoolProfile is self contained
> so I see absolutely no problem in having it there.
> The similarity to the jdk is important for the ExecutorServiceManager as
> very many people use this. The spi factory will only be used very seldomly.
> So I don´t think the similarity is very important.
> In fact it rather hurts as the user has to implement more methods.
>>
That is a fair argument. I will be okay with that. Also considering
ThreadPoolFactory is a new API and thus no backwards issue.
Fell free to change it to your needs.
Btw even if user have to implement more methods, they are more precise
what they are asking for: fixed vs cached etc.
Also people can often delegate to another method with defaults. So the
implementation is easy. See for example the JDK source
code in the Executors which is the JDK factory for thread pools. It
have many methods and there is a lot of delegates.
>>
>>> 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.
>
> Your arguments are true and I have no problems with the fact that you want
> the camel 2.x branch to be quite compatible. The problem I had was rather
> with the way you did this. You changed the code and then asked for a heads
> up. For my changes I first offered a patch and we discussed it.
>>
>>
>> We have Camel 3.0 where the API can be more open for changes.
>
> The problem with incompatible changes in major versions only is that they
> accumulate and make the major version very fragile and makes the development
> on that version take a very long time. So I think we should try to do as
> much in minor versions as we can and accept some minor incompatiblities. Of
> course it is very diffcult to judge what is minor.
>
>
> Christian
>
>
> --
> --
> Christian Schneider
> http://www.liquid-reality.de
>
> Open Source Architect
> Talend Application Integration Division http://www.talend.com
>
>
--
Claus Ibsen
-----------------
FuseSource
Email: cibsen@fusesource.com
Web: http://fusesource.com
Twitter: davsclaus, fusenews
Blog: http://davsclaus.blogspot.com/
Author of Camel in Action: http://www.manning.com/ibsen/
Re: [HEADS UP] - Adjustments to ExecutorServiceManager on trunk
Posted by Claus Ibsen <cl...@gmail.com>.
On Thu, Aug 18, 2011 at 2:07 PM, Christian Schneider
<ch...@die-schneider.net> wrote:
> Am 18.08.2011 13:50, schrieb Claus Ibsen:
>>
>> 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.
>>
>> 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.
>
> The class ThreadPoolProfile is not much more than a c struct to combine the
> attributes of a thread pool into a class.
> The implementor of a camel spi always has a dependency to the camel API as
> the spi interface is part of the API. So I see no real problem in having
> this in the interface.
> Most spi interfaces reference other camel api classes and many of those
> classes are much more problematic. The ThreadPoolProfile is self contained
> so I see absolutely no problem in having it there.
> The similarity to the jdk is important for the ExecutorServiceManager as
> very many people use this. The spi factory will only be used very seldomly.
> So I don´t think the similarity is very important.
> In fact it rather hurts as the user has to implement more methods.
>>
>>
>>> 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.
>
> Your arguments are true and I have no problems with the fact that you want
> the camel 2.x branch to be quite compatible. The problem I had was rather
> with the way you did this. You changed the code and then asked for a heads
> up. For my changes I first offered a patch and we discussed it.
>>
The refactoring of the threading API has not been requested by the community.
To my recollection there has been no questions on the @user list, or
JIRA tickets, or tweets,
blog posts etc. where people have said. Hey I think this API should be
changed to Y.
For a product that is 2+ years old, its important to keep the API stable.
We should refrain from changing the API because we just fell like it.
Of course there is a balance, and sometimes it makes sense to change API.
However backwards compatibility is important for a 2+ year old product.
What I dont like is the fact you attach a 265kb patch and expect
people to be able to
review and understand what you are doing. We all have busy work days
and have trust in our peers.
And then you commit the changes not long before you go on holiday.
When asked to keep the old API to ensure backwards compatible, you said, it was
on purpose the old API was removed. Giving us no impression that you
would do something
about to restore backwards compatibility. In fact you seem to not care
about that.
Because of your changes I spent the greater of a day to restore
backwards compatibility and
ensure an API that makes migration from the @deprecated ExecutorServiceStrategy
to ExecutorServiceManager much easier, in most cases by just changing
from strategy to manager.
And in other cases the method name have a slight name change. The
orders of the parameters is kept the same.
For example you flipped ordering of some of the parameters for no good
reason. Just making migration more painful.
There is about 10+ Camel components from the project itself which was
affected by this change.
There is X+ components out in the 3rd party communities that would be
affected as well.
Having the old API in this release is very important. So I am glad we
both seem to agree on this now.
At least the old API is in there and there is an unit test to cover
its usage. So at least if any changes
is done to the current API we have some belief the old API still works
due the unit tests.
>>
>> We have Camel 3.0 where the API can be more open for changes.
>
> The problem with incompatible changes in major versions only is that they
> accumulate and make the major version very fragile and makes the development
> on that version take a very long time. So I think we should try to do as
> much in minor versions as we can and accept some minor incompatiblities. Of
> course it is very diffcult to judge what is minor.
>
There is always a trade off.
But changing APIs between every Camel release makes people harder to
upgrade, even between minor release.
>
> Christian
>
>
> --
> --
> Christian Schneider
> http://www.liquid-reality.de
>
> Open Source Architect
> Talend Application Integration Division http://www.talend.com
>
>
--
Claus Ibsen
-----------------
FuseSource
Email: cibsen@fusesource.com
Web: http://fusesource.com
Twitter: davsclaus, fusenews
Blog: http://davsclaus.blogspot.com/
Author of Camel in Action: http://www.manning.com/ibsen/
Re: [HEADS UP] - Adjustments to ExecutorServiceManager on trunk
Posted by Christian Schneider <ch...@die-schneider.net>.
Am 18.08.2011 13:50, schrieb Claus Ibsen:
> 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.
>
> 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.
The class ThreadPoolProfile is not much more than a c struct to combine
the attributes of a thread pool into a class.
The implementor of a camel spi always has a dependency to the camel API
as the spi interface is part of the API. So I see no real problem in
having this in the interface.
Most spi interfaces reference other camel api classes and many of those
classes are much more problematic. The ThreadPoolProfile is self
contained so I see absolutely no problem in having it there.
The similarity to the jdk is important for the ExecutorServiceManager as
very many people use this. The spi factory will only be used very
seldomly. So I don´t think the similarity is very important.
In fact it rather hurts as the user has to implement more methods.
>
>
>> 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.
Your arguments are true and I have no problems with the fact that you
want the camel 2.x branch to be quite compatible. The problem I had was
rather with the way you did this. You changed the code and then asked
for a heads up. For my changes I first offered a patch and we discussed it.
>
>
> We have Camel 3.0 where the API can be more open for changes.
The problem with incompatible changes in major versions only is that
they accumulate and make the major version very fragile and makes the
development on that version take a very long time. So I think we should
try to do as much in minor versions as we can and accept some minor
incompatiblities. Of course it is very diffcult to judge what is minor.
Christian
--
--
Christian Schneider
http://www.liquid-reality.de
Open Source Architect
Talend Application Integration Division http://www.talend.com
Re: [HEADS UP] - Adjustments to ExecutorServiceManager on trunk
Posted by Claus Ibsen <cl...@gmail.com>.
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: cibsen@fusesource.com
Web: http://fusesource.com
Twitter: davsclaus, fusenews
Blog: http://davsclaus.blogspot.com/
Author of Camel in Action: http://www.manning.com/ibsen/
Re: [HEADS UP] - Adjustments to ExecutorServiceManager on trunk
Posted by Christian Schneider <ch...@die-schneider.net>.
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.
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.
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