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: 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/
