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
