Hi Phil, thanks a lot, that's the kind of feedback I was waiting for :) I don't have a single use case to submit to justify why keeping the synchronization feature, but something suggests me to propose to maintain it. Since, as you already noticed, we can avoid to maintain synchronized pool implementations anymore, I would give a chance to the proposal, we can remove the whole feature at all in a second time, if you agree.
I'm not sure I understood what you meant about (Keyed)ObjectPoolMinIdleTimerTask, can you provide me more info please? Many thanks in advance, have a nice day :) Simo http://people.apache.org/~simonetripodi/ http://www.99soft.org/ On Tue, Dec 28, 2010 at 8:32 PM, Phil Steitz <phil.ste...@gmail.com> wrote: > I am +0 for replacing the synchronized pools in PoolUtils with this > approach. The "0" is because the impls are already there and may perform > better (quite possibly not), the "+" because there is less code in the proxy > impl. I am also not 100% convinced that we should be providing these at > all. There is a warning to the user in the javadoc that we should keep if > we keep these. Interested in others' thoughts on this. > > Phil > > On Tue, Dec 28, 2010 at 11:36 AM, Simone Tripodi > <simone.trip...@gmail.com>wrote: > >> Hi again guys, >> just realized that pasted code is not so readable, I just copied it on >> pastie[1]. >> If you don't have any objections, I can start committing that stuff so >> you can review it. >> Many thanks in advance, >> Simo >> >> [1] http://pastie.org/1411379 >> >> http://people.apache.org/~simonetripodi/<http://people.apache.org/%7Esimonetripodi/> >> http://www.99soft.org/ >> >> >> >> On Sun, Dec 26, 2010 at 1:41 PM, Simone Tripodi >> <simone.trip...@gmail.com> wrote: >> > Hi Phil, >> > that's the way how I propose to synchronize pools using proxies >> > without implementing synchronized wrapper classes, if you don't see >> > issues about it I can start committing it let you all reviewing. >> > Please let me know, have a nice day!!! >> > Simo >> > >> > {{{ >> > >> > public static <T> ObjectPool<T> synchronizedPool(final ObjectPool<T> >> pool) { >> > return synchronizedObject(pool, ObjectPool.class); >> > } >> > >> > public static <K,V> KeyedObjectPool<K,V> synchronizedPool(final >> > KeyedObjectPool<K,V> keyedPool) { >> > return synchronizedObject(keyedPool, KeyedObjectPool.class); >> > } >> > >> > public static <T> PoolableObjectFactory<T> >> > synchronizedPoolableFactory(final PoolableObjectFactory<T> factory) { >> > return synchronizedObject(factory, PoolableObjectFactory.class); >> > } >> > >> > public static <K,V> KeyedPoolableObjectFactory<K,V> >> > synchronizedPoolableFactory(final KeyedPoolableObjectFactory<K,V> >> > keyedFactory) { >> > return synchronizedObject(keyedFactory, >> > KeyedPoolableObjectFactory.class); >> > } >> > >> > private static <T> T synchronizedObject(final T toBeSynchronized, >> > final Class<T> type) { >> > if (toBeSynchronized == null) { >> > throw new IllegalArgumentException("Object has to be >> > synchronized must not be null."); >> > } >> > >> > /** >> > * Used to synchronize method declared on the pool/factory >> > interface only. >> > */ >> > final Set<Method> synchronizedMethods = new HashSet<Method>(); >> > for (Method method : type.getDeclaredMethods()) { >> > synchronizedMethods.add(method); >> > } >> > >> > return type.cast(Proxy.newProxyInstance(type.getClassLoader(), >> > new Class<?>[] { type }, >> > new InvocationHandler() { >> > >> > private final Object lock = new Object(); >> > >> > public Object invoke(Object proxy, Method method, >> > Object[] args) throws Throwable { >> > if (synchronizedMethods.contains(method)) { >> > synchronized (this.lock) { >> > return method.invoke(toBeSynchronized, >> args); >> > } >> > } >> > return method.invoke(toBeSynchronized, args); >> > } >> > >> > } >> > )); >> > } >> > >> > }}} >> > >> > http://people.apache.org/~simonetripodi/<http://people.apache.org/%7Esimonetripodi/> >> > http://www.99soft.org/ >> > >> > >> > >> > On Fri, Dec 24, 2010 at 3:32 PM, Simone Tripodi >> > <simone.trip...@gmail.com> wrote: >> >> Hi Phil, >> >> thanks a lot for feedbacks and for mentoring, much more than >> appreciated. >> >> I take advantage to wish you all a Merry Christmas, all the best, >> >> Simo >> >> >> >> http://people.apache.org/~simonetripodi/<http://people.apache.org/%7Esimonetripodi/> >> >> http://www.99soft.org/ >> >> >> >> >> >> >> >> On Fri, Dec 24, 2010 at 4:32 AM, Phil Steitz <phil.ste...@gmail.com> >> wrote: >> >>> On Thu, Dec 23, 2010 at 2:28 AM, Simone Tripodi < >> simone.trip...@gmail.com>wrote: >> >>> >> >>>> Hi Phil, >> >>>> >> >>>> >> >> >>>> >> org.apache.commons.pool2.impl >> >>>> >> |---- generic >> >>>> >> |---- reference >> >>>> >> |---- stack >> >>>> >> >> >>>> >> common stuff could be included directly under impl. >> >>>> >> >> >>>> > >> >>>> > What exactly would that be? >> >>>> > >> >>>> >> >>>> just realized that there are no common stuff shared by different kind >> of >> >>>> pool :P >> >>>> >> >>>> > >> >>>> > I was going to propose dropping the stack pools altogether. The >> >>>> LIFO/FIFO >> >>>> > config option in the generic pools makes them mostly irrelevant >> (i.e., >> >>>> you >> >>>> > can get the same behavior with a suitably configured GOP / GKOP with >> much >> >>>> > more configurability) >> >>>> > >> >>>> >> >>>> I had the same feelings here, but felt a little shy on saying that. >> >>>> >> >>> >> >>> No reason for that :) >> >>> >> >>> >> >>>> You've my full support on this, I agree on dropping stack based pool >> >>>> implementations. >> >>>> >> >>>> Good. Lets do it. >> >>> >> >>> >> >>>> > I don't want to sound too conservative and I will certainly not >> stand in >> >>>> the >> >>>> > way of new / different pool implementations, but I would personally >> >>>> prefer >> >>>> > to keep the number of included pool impls as small as possible. >> >>>> > >> >>>> >> >>>> I propose a more democratic way, I mean, like you made us notice, >> >>>> keeping/adding the pool impl only if it makes sense to. >> >>>> I wouldn't think about the included pools in therms of "size" but >> >>>> rather in therms of "meaning" >> >>>> >> >>> >> >>> Agree this is completely up to the community and them who bring the >> >>> patches. The reason that I tend to be conservative is that more impls >> mean >> >>> more bugs, which makes fixing them all and getting releases out quickly >> >>> harder. Pooling code is tricky to maintain, so fewer pools with >> >>> less-exotic features might be better all around - not just in terms of >> >>> maintenance, but also approachability from the standpoint of users and >> >>> contributors. >> >>> >> >>>> >> >>>> > I think the first thing we need to do is to decide what >> implementations >> >>>> we >> >>>> > are going to a) keep or b) add for 2.0. I have been convinced that >> we >> >>>> need >> >>>> > to keep GKOP as well as GOP. As I said above, I would like to >> consider >> >>>> > dropping the stack-based pools. I think we should keep the >> >>>> reference-based >> >>>> > pool and I am open to the new ones you suggest, just don't have use >> cases >> >>>> > myself for them. >> >>>> >> >>>> I'm not ready to show use cases too, sorry :( But they can be added >> >>>> with a trivial refactory, moving the current SoftReferenceObjectPool >> >>>> implementation to an >> >>>> >> >>>> AbstractReferenceObjectPool<T, R extends Reference<T>> extends >> >>>> BaseObjectPool<T> implements ObjectPool<T> >> >>>> >> >>>> then in subclasses do the minimum. I can quickly provide a patch for >> it. >> >>>> >> >>> >> >>> I am OK with this as long as it does not complicate the code too much. >> >>> >> >>>> >> >>>> > There are quite a few impls buried in PoolUtils that might >> >>>> > make sense to pull out (or eliminate). >> >>>> > >> >>>> >> >>>> I just made a census (with proposals): >> >>>> >> >>>> * PoolableObjectFactoryAdaptor >> >>>> * KeyedPoolableObjectFactoryAdaptor >> >>>> * ObjectPoolAdaptor >> >>>> * KeyedObjectPoolAdaptor >> >>>> >> >>>> I propose to eliminate these adaptors, >> >>> >> >>> >> >>> That's the spirit - he he >> >>> >> >>> >> >>>> they implement a behavior that >> >>>> users can replicate with a trivial code and without build a >> >>>> pool/factory on top of an existing ones >> >>>> >> >>>> * CheckedObjectPool >> >>>> * CheckedKeyedObjectPool >> >>>> >> >>>> These can be eliminated too, having introduced the generics >> >>>> >> >>> >> >>> +1 - I think Gary may have mentioned this already. Lets get rid of >> them. >> >>> >> >>>> >> >>>> * ObjectPoolMinIdleTimerTask >> >>>> * KeyedObjectPoolMinIdleTimerTask >> >>>> >> >>>> I propose these pools can be pulled out and moved to a proper package >> >>>> >> >>>> I wonder if anyone uses these? I wonder also if it might be better to >> just >> >>> expose ensureMinIdle. >> >>> >> >>> >> >>>> * SynchronizedObjectPool >> >>>> * SynchronizedKeyedObjectPool >> >>>> * SynchronizedPoolableObjectFactory >> >>>> * SynchronizedKeyedPoolableObjectFactory >> >>>> >> >>>> These could be pulled out too, even if something suggests me that >> >>>> pools synchronization can be realized with just a Proxy, I'll do a >> >>>> little experiment to submit so you can evaluate. >> >>>> >> >>> >> >>> Interested in ideas on this. Here again, I am not sure how much use >> these >> >>> actually get. >> >>> >> >>>> >> >>>> > What might make sense is to replace "impl" with "instance" (or >> "object") >> >>>> and >> >>>> > "reference" (or "ref"). So you have o.a.c.p, o.a.c.p.instance, >> >>>> > o.a.c.p.reference. >> >>>> > >> >>>> >> >>>> sounds much better than keeping the intermediate "impl", I agree :) >> >>>> >> >>>> Should I have to write all these notes on the wiki and open issues >> >>>> before proceeding? >> >>>> >> >>> >> >>> I would say wait a bit to see if anyone has problems with above and if >> not, >> >>> go ahead and make the changes. >> >>> >> >>> >> >>>> Many thanks in advance, have a nice day! >> >>>> >> >>> >> >>> Thank you! >> >>> >> >>> Phil >> >>> >> >>>> Simo >> >>>> >> >>>> --------------------------------------------------------------------- >> >>>> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >> >>>> For additional commands, e-mail: dev-h...@commons.apache.org >> >>>> >> >>>> >> >>> >> >> >> > >> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >> For additional commands, e-mail: dev-h...@commons.apache.org >> >> > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
