On Sat, Aug 13, 2011 at 11:20 AM, Christian Schneider <ch...@die-schneider.net> wrote: > I am -1 on your last changes (CAMEL-4298). You effectively rolled back most > of my changes from CAMEL-4244. > > I intend to make the ThreadPoolProfile the main configuration method for > thread pools. This makes working with defaults much more natural than with > the more JDK style API. Your change pulls out the ThreadPoolProfile from > most of the API. You simply configure what you need in the ThreadPoolProfile > and the defaults aree filled in. My change also supported that the logic for > default values is not spread over the whole code but concentrated in the > ThreadPoolProfile class. >
ThreadPoolProfile is a tiny abstraction where people can configure settings for thread pools, and enlist them in the ExecutorServiceManager. A ThreadPoolProfile is a pure Camel API, and there is no such thing in the JDK. And ThreadPoolProfile is a concept the ExecutorServiceManager only knows about, as it manages the registered ThreadPoolProfiles. So the ThreadPoolProfile is associated with the ExecutorServiceManager, and not other places in the code base. We should not force people to use the concept of ThreadPoolProfile when they work with thread pools. Knowing the API of the JDK should be sufficient. They can chose to use a ThreadPoolProfile if they want, or just using what they already know from the JDK API. People who use thread pools should be able to use the JDK API and an API that reflect the API from the JDK which their are familiar with. So for example a ThreadPoolBuilder should create thread pools (aka ExecutorService in JDK terms). And not ThreadPoolProfiles. That is confusing. And btw the default values is not spread over the whole code. Try to put things in perspective! > My next step would have been to change the threads() DSL element to only > accept a ThreadPoolProfile for camel 3.0 so the DSL gets lighter. This is > not possible anymore after your change. You also removed the > ThreadPoolBuilder for profiles. I know we should rather name it > ThreadPoolProfileBuilder (which is a bit long) but we need this to have a > builder pattern for profiles. > The threads DSL should at least use the API of the JDK that people are familiar with. It already support a thread pool profile as you can refer to it <threads executorServiceRef="myProfileId"> ... And from Java DSL .threads().executorServiceRef("myProfileId") > We can dicuss about which changes to keep but simply reverting most of my > changes was quite rude. I posted my changes in a jira so we can dicuss them > and we agreed on them before I committed. I was on holidays during the last > two weeks so I could not react earlier. > Well your changes broke several things and made Camel 2.9 not backwards compatible. Frankly your bigger refactorings dont belong in a minor release, but in a new major release such as Camel 3.0. Also you should respect that Camel 2.x is 2+ years old and we should keep the API stable so people can rely on a stable and prove branch of the Apache Camel product. We cant go around changing the API all the time. For example to use ExecutorServiceManager the API was now totally changed from a JDK API which people are familiar with, to a bit confusing API with a ThreadPoolProfile. To use the API they now have to import 2 extra classes from 2 other packages - ThreadPoolProfile - ThreadPoolBuilder Just to for example to create a background task in their component. Also it was not clear anymore how to create a fixed, cached, or single thread pool anymore. The ThreadPoolBuilder did not make that straight forward. With the old API they need *0* extra imports and can just do getExecutorServiceStrategy().newScheduledThreadPool(this, "MyBackgroundTask"); And from trunk they still need *0* extra imports and can just do (method names uses the naming convention from the JDK) getExecutorServiceManager().newSingleThreadScheduledExecutor(this, "MyBackgroundTask"); With your API that is no longer possible and they would need to understand what the heck to do, and to go read about a ThreadPoolProfile and how to create that. So if Camel makes this confusing and not easy, then people will stop using the Camel API and just go for plain JDK. That works and is compatible in all JDKs and Camel versions. Its is very important for the Apache Camel project to ensure 3rd party Camel components is as much compatible in the 2.x line. In fact when we discussed Camel 3.0 API, we talked about the importance of 3rd party Camel components being able to run out of the box. So any API changes should be taken with care when it Camel to components. > I am +0 for having the ExecutorServiceStrategy for backwards compatibilty. I > do not think it is necessary but it does not hurt. > About 10-15 Camel components was affected by this change. Dont you think 3rd party components would have been as well? Changing the DSL is a bit more easier as people compile the Camel applications with the version they use. But people use 3rd party components from camel-extra, githib, inhouse developed, etc. And with a non backwards change, they would have to support multiple branches to support Camel 2.8 or older. And Camel 2.9+. That dont give them confidence in the Apache Camel project. And it is possible to keep the old API in place and mark it @deprecated and give people amble time to adjust if we really want to remove the deprecated API. In fact know its a just a facade which delegates to the new ExecutorServiceManager who does the real work. Also a testimony that the new API does indeed fully cover the old API as the old unit test was copied and it passes. > Christian > All together Christian I think you should create a wiki page with your ideas for API changes in this area. And then we can look at this for Camel 3.0 where the API is open for changes. Not in a minor Camel release such as 2.8 -> 2.9. And especially now that Camel 2.x is 2+ years old and the API should be stable so people can keep enjoy using Camel. > > Am 12.08.2011 17:29, schrieb Claus Ibsen: >> >> Hi >> >> Recently the ExecutorServiceStrategy had a bigger refactor >> (https://issues.apache.org/jira/browse/CAMEL-4244) >> Such as renaming the class to a better name ExecutorServiceManager. >> Also introducing a ThreadPoolFactory, ThreadFactory among others. The >> ThreadPoolFactory is a great addition, which makes it easier for 3rd >> party to just implement a custom factory, and then reply on the >> default manager. >> >> However the refactoring introduced a few issues such as configuring >> thread pools on EIPs was flawed. Well it also revealed we were short >> of an unit test to cover this. So I created a ticket, and committed a >> test to the 2.8 branch. >> https://issues.apache.org/jira/browse/CAMEL-4328 >> >> The refactoring did also make backwards compatibility an issue, so we >> will restore that but mark the old API as @deprecated. >> >> The changes are summarized as follows >> - ThreadPoolFactory is the light weight and easier for SPI >> implementators to create a custom thread pool provider, such as JEE >> servers with a WorkManager API. This API has much fewer methods and >> they have been adjusted to be 100% JDK API (There is no Camel API in >> there, such as ThreadPoolProfile). >> - ExecutorServiceManager is the full fledged SPI in case you need 100% >> control. But it has Camel APIs such as ThreadPoolProfiles and a number >> of more methods. Its is encouraged to just implement a custom >> ThreadPoolFactory instead. And keep using the >> DefaultExecutorServiceManager. >> - ThreadPoolBuilder is adjusted to create a thread pool on the build >> method. This is how end users would expect. A builder to create what >> its name implies. This also removes entanglement of ThreadPool and >> ThreadPoolProfile. The latter is Camel specific and is only part of >> the ExecutorServiceManager which manges a list of profiles. To help >> uniform and make it easy to adjust thread pool settings at a higher >> level. So ThreadPoolProfiles should only be party of the manager. >> - EIPs configured using an explicit thread pool by an >> executorServiceRef, will now act as expected. If the reference could >> not be found, an exception is thrown, stating the reference is not >> found >> - Will add the ExecutorServiceStrategy SPI to have Camel 2.9 being >> backwards compatible, but mark it as @deprecated. This gives end users >> some time to adjust their custom Camel components, and source code if >> needed. >> - Naming of the methods will use the naming convention used by the >> JDK. xxxThreadPool for a pool of threads, xxxExecutor if its a single >> threaded (eg used for background tasks) >> - Made the API of ExecutorServiceManager more similar to the old >> ExecutorServiceStrategy so migrating is a breeze, usually just change >> the getExecutorServiceStrategy() to getExecutorServiceManager() and >> you are settled. >> >> I ran a complete unit test on my local laptop before commiting the >> changes. >> There is a few TODO in the code, which I will work on as well, so >> expect a few more commits. The TODO is minor/cosmetic changes I have >> spotted, that can be improved. >> >> >> > > -- > Christian Schneider > http://www.liquid-reality.de > > Open Source Architect > http://www.talend.com > > -- Claus Ibsen ----------------- FuseSource Email: cib...@fusesource.com Web: http://fusesource.com Twitter: davsclaus, fusenews Blog: http://davsclaus.blogspot.com/ Author of Camel in Action: http://www.manning.com/ibsen/