Re: [HEADS UP] - Adjustments to ExecutorServiceManager on trunk
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
On Fri, Aug 19, 2011 at 2:22 PM, Christian Schneider 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: 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/
Re: [HEADS UP] - Adjustments to ExecutorServiceManager on trunk
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
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: 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/
Re: [HEADS UP] - Adjustments to ExecutorServiceManager on trunk
On Thu, Aug 18, 2011 at 2:07 PM, Christian Schneider wrote: > Am 18.08.2011 13:50, schrieb Claus Ibsen: >> >> On Thu, Aug 18, 2011 at 1:25 PM, Christian Schneider >> 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: cib...@fusesource.com Web: http://fusesource.com Twitter: davsclaus, fusenews Blog: http://d
Re: [HEADS UP] - Adjustments to ExecutorServiceManager on trunk
On Thu, Aug 18, 2011 at 2:07 PM, Christian Schneider wrote: > Am 18.08.2011 13:50, schrieb Claus Ibsen: >> >> On Thu, Aug 18, 2011 at 1:25 PM, Christian Schneider >> 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 c
Re: [HEADS UP] - Adjustments to ExecutorServiceManager on trunk
Am 18.08.2011 13:50, schrieb Claus Ibsen: On Thu, Aug 18, 2011 at 1:25 PM, Christian Schneider 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
On Thu, Aug 18, 2011 at 1:50 PM, Claus Ibsen wrote: > On Thu, Aug 18, 2011 at 1:25 PM, Christian Schneider > 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 > * > * 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 3r
Re: [HEADS UP] - Adjustments to ExecutorServiceManager on trunk
On Thu, Aug 18, 2011 at 1:25 PM, Christian Schneider 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 * * 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 on
Re: [HEADS UP] - Adjustments to ExecutorServiceManager on trunk
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
Re: [HEADS UP] - Adjustments to ExecutorServiceManager on trunk
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
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: 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/
Re: [HEADS UP] - Adjustments to ExecutorServiceManager on trunk
On Sat, Aug 13, 2011 at 11:20 AM, Christian Schneider 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 ... 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 c
Re: [HEADS UP] - Adjustments to ExecutorServiceManager on trunk
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
[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: 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/