You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Liviu Tudor <li...@gmail.com> on 2012/07/10 23:51:49 UTC
[pool] Question / potential improvement on KeyedObjectPool
Hi guys,
I have a question regarding [pool] -- which might turned out to be either a
request for change or a potential improvement patch (though it could be the
case I suppose that I've misunderstood certain things, in which case it will
just turn out into my wasting people's time -- hopefully not though :)
So a bit of context: I'm using [pool] in one of the projects I'm working on,
more specifically I'm using a GenericKeyedObjectPool. This is actually used
as a "create (instances) and cache (them) ahead" in the sense that it's used
to create in advance N instances of certain classes and pool them. All these
classes are "script classes" -- in the sense that we do allow in this
application a few hooks where we can execute user-written Java code, which
obviously are not known ahead and because the application is a rather
high-throughput one, rather than creating these instances on demand (when
execution of the program reaches the "hook" point and needs to run the
"script") I've decided to have a pool-based implementation which will create
in advance N instances of these "script" classes and then when the "hook"
point is used, simply use one of the already-created objects from the pool
thus decreasing the execution time. Without going into details, some of
these instances can be re-used and added back to the pool, while others are
simply a one-off execution, after which the class needs to be destroyed
however, in both cases, I would like to keep around N instances pre-created
in the pool, for each class type.
Now, my idea and this is where (finally!) my question/suggestion comes in
is to have a background thread (a ScheduledExecutorService, scheduled at
fixed intervals) which kicks in and goes through all the keys in the pool
and for each such key it uses the KeyedObjectPoolableFactory to create up to
N instances for each key (based of course also on the getNumIdle(Key)
return).
The problem with that as I found out soon is that there is no way to
access the set of keys in a KeyedObjectPool! I was expecting something upon
the lines of standard JDK collections :
Set<K> keySet();
or similar, however, there isn't anything like that around. So question
number one is : Should we consider making such a method available? (As far
as I can see, the GenericKeyedObjectPool uses a poolMap member internally so
we could easily wrap up the keySet() into an unmodifiable set or similar if
that makes sense.)
Alternatively and this is question two, and where this email can turn into
a proposal for a commit of a new component/class does it make sense to
have a specialised KeyedObjectPool which does what I described above? The
way I'm doing the pre population is by keeping a set of all the keys that
were requested/removed (through borrowObject/preparePool/etc) -- and the
background thread iterates through these keys and creates instances where
necessary. (I am happy of course to provide this implementation to ASF as a
patch request or similar.)
Hopefully I managed to explain what I'm trying to achieve here but happy
to provide more details where necessary.
Many thanks in advance,
Liv
Liviu Tudor
E: liviu.tudor@gmail.com <ma...@gmail.com>
C: +1 650-2833815 (USA)
M: +44 (0)7591540313 (UK, Europe)
W: http://about.me/liviutudor <http://about.me/liviutudor>
Skype: liviutudor
I'm nobody, nobody's perfect -- therefore I'm perfect!
Re: [pool] Question / potential improvement on KeyedObjectPool
Posted by Liviu Tudor <li...@gmail.com>.
Hi Phil,
Sorry to get onto this so late! I've just checked out the source code in
order to provide a patch at the same time as the JIRA ticket being
created, however, I see [pool] now is moving to pool2 -- my initial
suggestion was based on pool-1.6.0 (which is what I use in my
application).
Are there any maintenance releases planned for pool-1.6 or should I
concentrate entirely (and apply this patch to) pool2?
Many thanks,
Liv
Liviu Tudor
E: liviu.tudor@gmail.com
C: +1 650-2833815 (USA)
M: +44 (0)7591540313 (UK, Europe)
W: http://about.me/liviutudor
Skype: liviutudor
I'm nobody, nobody's perfect -- therefore I'm perfect!
On 11/07/2012 21:21, "Phil Steitz" <ph...@gmail.com> wrote:
>On 7/10/12 2:51 PM, Liviu Tudor wrote:
>> Hi guys,
>>
>> I have a question regarding [pool] -- which might turned out to be
>>either a
>> request for change or a potential improvement patch (though it could be
>>the
>> case I suppose that I've misunderstood certain things, in which case it
>>will
>> just turn out into my wasting people's time -- hopefully not though :)
>>
>> So a bit of context: I'm using [pool] in one of the projects I'm
>>working on,
>> more specifically I'm using a GenericKeyedObjectPool. This is actually
>>used
>> as a "create (instances) and cache (them) ahead" in the sense that it's
>>used
>> to create in advance N instances of certain classes and pool them. All
>>these
>> classes are "script classes" -- in the sense that we do allow in this
>> application a few hooks where we can execute user-written Java code,
>>which
>> obviously are not known ahead ‹ and because the application is a rather
>> high-throughput one, rather than creating these instances on demand
>>(when
>> execution of the program reaches the "hook" point and needs to run the
>> "script") I've decided to have a pool-based implementation which will
>>create
>> in advance N instances of these "script" classes and then when the
>>"hook"
>> point is used, simply use one of the already-created objects from the
>>pool ‹
>> thus decreasing the execution time. Without going into details, some of
>> these instances can be re-used and added back to the pool, while others
>>are
>> simply a one-off execution, after which the class needs to be destroyed
>>‹
>> however, in both cases, I would like to keep around N instances
>>pre-created
>> in the pool, for each class type.
>>
>> Now, my idea ‹ and this is where (finally!) my question/suggestion
>>comes in
>> ‹ is to have a background thread (a ScheduledExecutorService, scheduled
>>at
>> fixed intervals) which kicks in and goes through all the keys in the
>>pool
>> and for each such key it uses the KeyedObjectPoolableFactory to create
>>up to
>> N instances for each key (based of course also on the getNumIdle(Key)
>> return).
>>
>> The problem with that ‹ as I found out soon ‹ is that there is no way to
>> access the set of keys in a KeyedObjectPool! I was expecting something
>>upon
>> the lines of standard JDK collections :
>>
>> Set<K> keySet();
>>
>> or similar, however, there isn't anything like that around. So question
>> number one is : Should we consider making such a method available? (As
>>far
>> as I can see, the GenericKeyedObjectPool uses a poolMap member
>>internally so
>> we could easily wrap up the keySet() into an unmodifiable set or
>>similar if
>> that makes sense.)
>
>This seems a reasonable enhancement request. Can you please open a
>JIRA for it. That way you can also attach patches ;)
>
>The tricky bit will be how to do this with integrity but without
>performance impact.
>
>Phil
>>
>> Alternatively ‹ and this is question two, and where this email can turn
>>into
>> a proposal for a commit of a new component/class ‹ does it make sense to
>> have a specialised KeyedObjectPool which does what I described above?
>>The
>> way I'm doing the pre population is by keeping a set of all the keys
>>that
>> were requested/removed (through borrowObject/preparePool/etc) -- and the
>> background thread iterates through these keys and creates instances
>>where
>> necessary. (I am happy of course to provide this implementation to ASF
>>as a
>> patch request or similar.)
>>
>> Hopefully I managed to explain what I'm trying to achieve here ‹ but
>>happy
>> to provide more details where necessary.
>>
>> Many thanks in advance,
>>
>>
>>
>> Liv
>>
>>
>> Liviu Tudor
>>
>> E: liviu.tudor@gmail.com <ma...@gmail.com>
>> C: +1 650-2833815 (USA)
>> M: +44 (0)7591540313 (UK, Europe)
>> W: http://about.me/liviutudor <http://about.me/liviutudor>
>> Skype: liviutudor
>>
>> I'm nobody, nobody's perfect -- therefore I'm perfect!
>>
>>
>>
>>
>>
>>
>
>
>
>---------------------------------------------------------------------
>To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [pool] Question / potential improvement on KeyedObjectPool
Posted by Liviu Tudor <li...@gmail.com>.
Thanks, Mark, that makes sense!
In that case I will apply all the patches to 1.5 too.
Thanks everyone for helping with this!
Liv
Liviu Tudor
E: liviu.tudor@gmail.com
C: +1 650-2833815 (USA)
M: +44 (0)7591540313 (UK, Europe)
W: http://about.me/liviutudor
Skype: liviutudor
I'm nobody, nobody's perfect -- therefore I'm perfect!
On 30/07/2012 14:37, "Mark Thomas" <ma...@apache.org> wrote:
>On 30/07/2012 22:33, Gary Gregory wrote:
>> On Mon, Jul 30, 2012 at 5:21 PM, Liviu Tudor <li...@gmail.com>
>>wrote:
>>
>>> Mark/Gary/Seb,
>>>
>>> Just to clarify this: I haven't touched at all the 1.5 branch -- all of
>>> these refer strictly to the 1.6 branch (which uses JDK 1.5 according to
>>> the pom and also uses Junit 4.10).
>>>
>>> So in the light of these, should I:
>>>
>>> 1/ also apply these patches to the 1.5 branch?
>>>
>>
>> Up to you.
>>
>>
>>> 2/ downgrade junit to 3.8.2 on the 1.6 branch?
>>>
>>
>> I do not think so, this branch is for Java >= 1.5. Why? Does JUnit 4.10
>>not
>> run on Java 5?
>
>1.6.x is meant to be 1.5.x plus generics. The more they diverge, the
>harder they will be to keep in sync.
>
>Mark
>
>
>---------------------------------------------------------------------
>To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [pool] Question / potential improvement on KeyedObjectPool
Posted by Mark Thomas <ma...@apache.org>.
On 30/07/2012 22:33, Gary Gregory wrote:
> On Mon, Jul 30, 2012 at 5:21 PM, Liviu Tudor <li...@gmail.com> wrote:
>
>> Mark/Gary/Seb,
>>
>> Just to clarify this: I haven't touched at all the 1.5 branch -- all of
>> these refer strictly to the 1.6 branch (which uses JDK 1.5 according to
>> the pom and also uses Junit 4.10).
>>
>> So in the light of these, should I:
>>
>> 1/ also apply these patches to the 1.5 branch?
>>
>
> Up to you.
>
>
>> 2/ downgrade junit to 3.8.2 on the 1.6 branch?
>>
>
> I do not think so, this branch is for Java >= 1.5. Why? Does JUnit 4.10 not
> run on Java 5?
1.6.x is meant to be 1.5.x plus generics. The more they diverge, the
harder they will be to keep in sync.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [pool] Question / potential improvement on KeyedObjectPool
Posted by Gary Gregory <ga...@gmail.com>.
On Mon, Jul 30, 2012 at 5:21 PM, Liviu Tudor <li...@gmail.com> wrote:
> Mark/Gary/Seb,
>
> Just to clarify this: I haven't touched at all the 1.5 branch -- all of
> these refer strictly to the 1.6 branch (which uses JDK 1.5 according to
> the pom and also uses Junit 4.10).
>
> So in the light of these, should I:
>
> 1/ also apply these patches to the 1.5 branch?
>
Up to you.
> 2/ downgrade junit to 3.8.2 on the 1.6 branch?
>
I do not think so, this branch is for Java >= 1.5. Why? Does JUnit 4.10 not
run on Java 5?
Gary
>
> Regards,
>
>
> Liv
>
> Liviu Tudor
>
> E: liviu.tudor@gmail.com
> C: +1 650-2833815 (USA)
> M: +44 (0)7591540313 (UK, Europe)
> W: http://about.me/liviutudor
> Skype: liviutudor
>
> I'm nobody, nobody's perfect -- therefore I'm perfect!
>
>
>
>
>
>
>
> On 30/07/2012 14:15, "sebb" <se...@gmail.com> wrote:
>
> >>>
> >>>* Last but not the least, I see the pom references junit-4.10 but the
> >>>unit
> >>>tests are still based on the old junit-3.8 looking at it. Worth to
> >>>change
> >>>these to annotations a la junit-4?
> >>
> >>Not for Pool 1.5. The target Java version is < 1.5.
> >
> >In which case, it should not be using JUnit 4.10, which does not run
> >on Java 1.4; the most recent JUnit for Java 1.4 is 3.8.2.
> >
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
--
E-Mail: garydgregory@gmail.com | ggregory@apache.org
JUnit in Action, 2nd Ed: <http://goog_1249600977>http://bit.ly/ECvg0
Spring Batch in Action: <http://s.apache.org/HOq>http://bit.ly/bqpbCK
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory
Re: [pool] Question / potential improvement on KeyedObjectPool
Posted by Liviu Tudor <li...@gmail.com>.
Mark/Gary/Seb,
Just to clarify this: I haven't touched at all the 1.5 branch -- all of
these refer strictly to the 1.6 branch (which uses JDK 1.5 according to
the pom and also uses Junit 4.10).
So in the light of these, should I:
1/ also apply these patches to the 1.5 branch?
2/ downgrade junit to 3.8.2 on the 1.6 branch?
Regards,
Liv
Liviu Tudor
E: liviu.tudor@gmail.com
C: +1 650-2833815 (USA)
M: +44 (0)7591540313 (UK, Europe)
W: http://about.me/liviutudor
Skype: liviutudor
I'm nobody, nobody's perfect -- therefore I'm perfect!
On 30/07/2012 14:15, "sebb" <se...@gmail.com> wrote:
>>>
>>>* Last but not the least, I see the pom references junit-4.10 but the
>>>unit
>>>tests are still based on the old junit-3.8 looking at it. Worth to
>>>change
>>>these to annotations a la junit-4?
>>
>>Not for Pool 1.5. The target Java version is < 1.5.
>
>In which case, it should not be using JUnit 4.10, which does not run
>on Java 1.4; the most recent JUnit for Java 1.4 is 3.8.2.
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [pool] Question / potential improvement on KeyedObjectPool
Posted by sebb <se...@gmail.com>.
On 30 July 2012 21:16, Mark Thomas <ma...@apache.org> wrote:
> On 30/07/2012 21:07, Liviu Tudor wrote:
>> * Findbugs: indeed, some of the findbugs warnings are not entirely correct
>> -- and we should probably look at using the Findbugs annotations to
>> manually annotate the ones which we know to actually be false alarms?
>> There were also a couple related to performance suggestions which were
>> valid -- so as per your suggestion I'm going to submit a patch for those.
>> Regarding the false alerts in FindBugs -- there are 2 ways to proceed with
>> "removing" these alerts:
>> ** one is as I suggested above to enable the findbugs annotations in the
>> pom and manually annotate those 2 methods to remove the findbugs checks
>> ** second one is to manually remove the findbugs checks via a
>> findbugs-exclude.xml from the suite of findbugs tests run.
>> Not sure which one do we prefer in Commons? (or more to the point in this
>> case?)
>
> Personally, I prefer findbugs-exclude.xml since that doesn't pollute the
> build for folks who have no interest in FindBugs.
>
>> * The unit test thing though puzzles me as I keep constantly get this :
>>
>> Results :
>>
>> Failed tests:
>> testConstructors(org.apache.commons.pool.impl.TestGenericKeyedObjectPool):
>> expected:<4> but was:<2>
>>
>> testConstructors(org.apache.commons.pool.impl.TestGenericKeyedObjectPoolFac
>> tory): expected:<4> but was:<2>
>>
>> testConstructors(org.apache.commons.pool.impl.TestGenericObjectPoolFactory)
>> : expected:<4> but was:<2>
>>
>>
>> When running the TestGenericKeyedObjectPool! I'm running them on a Mac --
>> but checked them on a win machine with the same result. Looking closer, it
>> seems to be down to the fact that the maxIdle is set to be lower than
>> midIdle -- and there is logic in the GenericKeyedObjectPool to return
>> maxIdle in such cases. I'm not sure therefore if the default values in the
>> unit test are wrong or the logic in the GenericKeyedObjectPool is wrong?
>> I'm happy to change the assert's in the unit test and submit a patch --
>> but I would like this validated by someone who has been involved
>> previously in this piece of code?
>> Also, I would be curious to know whether the tests are indeed passing ?
>
> They fail for me to. Odd. I would have expected a CI nag about that.
>
> 'svn log' and 'svn blame' can be very useful when determining why code
> is the way it is. In this case they point to POOL-212 and it is indeed
> the unit tests that are wrong.
>
>> * Last but not the least, I see the pom references junit-4.10 but the unit
>> tests are still based on the old junit-3.8 looking at it. Worth to change
>> these to annotations a la junit-4?
>
> Not for Pool 1.5. The target Java version is < 1.5.
In which case, it should not be using JUnit 4.10, which does not run
on Java 1.4; the most recent JUnit for Java 1.4 is 3.8.2.
> Not sure it is worth
> it for 1.6 (and it easier to keep the 1.5.x and 1.6.x in sync if it is
> just generics that is the difference). For Pool 2.x I don't have a
> strong preference.
>
> Mark
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [pool] Question / potential improvement on KeyedObjectPool
Posted by Mark Thomas <ma...@apache.org>.
On 30/07/2012 21:07, Liviu Tudor wrote:
> * Findbugs: indeed, some of the findbugs warnings are not entirely correct
> -- and we should probably look at using the Findbugs annotations to
> manually annotate the ones which we know to actually be false alarms?
> There were also a couple related to performance suggestions which were
> valid -- so as per your suggestion I'm going to submit a patch for those.
> Regarding the false alerts in FindBugs -- there are 2 ways to proceed with
> "removing" these alerts:
> ** one is as I suggested above to enable the findbugs annotations in the
> pom and manually annotate those 2 methods to remove the findbugs checks
> ** second one is to manually remove the findbugs checks via a
> findbugs-exclude.xml from the suite of findbugs tests run.
> Not sure which one do we prefer in Commons? (or more to the point in this
> case?)
Personally, I prefer findbugs-exclude.xml since that doesn't pollute the
build for folks who have no interest in FindBugs.
> * The unit test thing though puzzles me as I keep constantly get this :
>
> Results :
>
> Failed tests:
> testConstructors(org.apache.commons.pool.impl.TestGenericKeyedObjectPool):
> expected:<4> but was:<2>
>
> testConstructors(org.apache.commons.pool.impl.TestGenericKeyedObjectPoolFac
> tory): expected:<4> but was:<2>
>
> testConstructors(org.apache.commons.pool.impl.TestGenericObjectPoolFactory)
> : expected:<4> but was:<2>
>
>
> When running the TestGenericKeyedObjectPool! I'm running them on a Mac --
> but checked them on a win machine with the same result. Looking closer, it
> seems to be down to the fact that the maxIdle is set to be lower than
> midIdle -- and there is logic in the GenericKeyedObjectPool to return
> maxIdle in such cases. I'm not sure therefore if the default values in the
> unit test are wrong or the logic in the GenericKeyedObjectPool is wrong?
> I'm happy to change the assert's in the unit test and submit a patch --
> but I would like this validated by someone who has been involved
> previously in this piece of code?
> Also, I would be curious to know whether the tests are indeed passing ?
They fail for me to. Odd. I would have expected a CI nag about that.
'svn log' and 'svn blame' can be very useful when determining why code
is the way it is. In this case they point to POOL-212 and it is indeed
the unit tests that are wrong.
> * Last but not the least, I see the pom references junit-4.10 but the unit
> tests are still based on the old junit-3.8 looking at it. Worth to change
> these to annotations a la junit-4?
Not for Pool 1.5. The target Java version is < 1.5. Not sure it is worth
it for 1.6 (and it easier to keep the 1.5.x and 1.6.x in sync if it is
just generics that is the difference). For Pool 2.x I don't have a
strong preference.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [pool] Question / potential improvement on KeyedObjectPool
Posted by Liviu Tudor <li...@gmail.com>.
Hey Mark,
* I understand about JIRA -- my question was I guess more upon the lines
of should I submit a patch in JIRA (after opening a ticket) and then carry
out the discussion for things like Set vs List there, or is this list a
better place. I understand now that the dev list is better, thank you!
* I'll start a separate discussion here about Set vs List then and try to
structure all of my thoughts / suggestions in one email.
* Re: branch
http://svn.apache.org/repos/asf/commons/proper/pool/branches/POOL_1_X -- I
can see there are commits on this branch but most of the recent ones are
to do with spelling mistakes and/or javadocs; while I am not disregarding
the importance of those, I just wondered whether this is now a branch
which will only see small commit sets not related to the actual
functionality, but rather to do with bug fixes/docco/etc, or is full
development still going on in this branch and we're still looking to add
new functionality in this branch? For whatever reason though I used the
term "abandoned" which I can see it's creating problem -- sorry about
that. I got the answer for it from your email though, thanks for the
heads-up on that.
* Checkstyle: again, wrong terminology on my side -- there seems to be a
misconfiguration in the pom which causes a lot of files to show checkstyle
warnings -- as per your email I will create a JIRA ticket and submit a
patch for this separately. The fact it's the same checkstyle warning
appearing everywhere was a concern to me and investigating it I discovered
this misconfiguration in the pom -- arguably this is a trivial error in
pom.xml, though it doesn't break the build process, so as you said, it
probably shouldn't be considered an "error" per se. Either way, I'll send
a patch for this.
* Findbugs: indeed, some of the findbugs warnings are not entirely correct
-- and we should probably look at using the Findbugs annotations to
manually annotate the ones which we know to actually be false alarms?
There were also a couple related to performance suggestions which were
valid -- so as per your suggestion I'm going to submit a patch for those.
Regarding the false alerts in FindBugs -- there are 2 ways to proceed with
"removing" these alerts:
** one is as I suggested above to enable the findbugs annotations in the
pom and manually annotate those 2 methods to remove the findbugs checks
** second one is to manually remove the findbugs checks via a
findbugs-exclude.xml from the suite of findbugs tests run.
Not sure which one do we prefer in Commons? (or more to the point in this
case?)
* The unit test thing though puzzles me as I keep constantly get this :
Results :
Failed tests:
testConstructors(org.apache.commons.pool.impl.TestGenericKeyedObjectPool):
expected:<4> but was:<2>
testConstructors(org.apache.commons.pool.impl.TestGenericKeyedObjectPoolFac
tory): expected:<4> but was:<2>
testConstructors(org.apache.commons.pool.impl.TestGenericObjectPoolFactory)
: expected:<4> but was:<2>
When running the TestGenericKeyedObjectPool! I'm running them on a Mac --
but checked them on a win machine with the same result. Looking closer, it
seems to be down to the fact that the maxIdle is set to be lower than
midIdle -- and there is logic in the GenericKeyedObjectPool to return
maxIdle in such cases. I'm not sure therefore if the default values in the
unit test are wrong or the logic in the GenericKeyedObjectPool is wrong?
I'm happy to change the assert's in the unit test and submit a patch --
but I would like this validated by someone who has been involved
previously in this piece of code?
Also, I would be curious to know whether the tests are indeed passing ?
* Last but not the least, I see the pom references junit-4.10 but the unit
tests are still based on the old junit-3.8 looking at it. Worth to change
these to annotations a la junit-4?
Thanks in advance for your help on this!
Liv
Liviu Tudor
E: liviu.tudor@gmail.com
C: +1 650-2833815 (USA)
M: +44 (0)7591540313 (UK, Europe)
W: http://about.me/liviutudor
Skype: liviutudor
I'm nobody, nobody's perfect -- therefore I'm perfect!
On 30/07/2012 06:48, "Mark Thomas" <ma...@apache.org> wrote:
>On 30/07/2012 09:36, Liviu Tudor wrote:
>> Thanks very much for pointing me in the right direction, Mark!
>> As with all development tasks, this, however, opened up new questions --
>> not entirely sure if these are best discussed on this list or whether
>> occasionally I should open up a JIRA ticket for this so more than
>> welcoming suggestions here.
>
>Jira is not the place for discussions. That is best done on the dev
>list. Once we have an agreed direction then we can open a Jira to track
>the work.
>
>> * on the [pool2] -- I haven't investigated the eviction mechanism in
>> detail there, but I understand the need for a List and that's fine. My
>> original idea was to provide a Set with all the keys for the
>> KeyedObjectPool interface, as there is no need for ordering of the keys
>> here (same as in Map.keySet) however, I can just as well have a List and
>> provide a getKeys() in the KeyedObjectPool interface -- it will be used
>> the same ultimately for my idea, which in fact needs just a collection
>> (Set was preferred due to the fact that Map provides access to the keys
>> via a Set and the fact that keys are unique obviously). So, in the light
>> of that, which would be better to have in KeyedObjectPool interface (if
>> any):
>> 1/ Set<K> keySet(); //returns a Set with all the keys in the pool
>> 2/ List<K> getKeys(); //this is already implemented part of the Mbean
>> implementation in GenericKeyedObjectPool
>> 3/ Collection<K> getKeys();//this allows us to customise it in
>> subclasses, so we can have List in GenericKeyedObjectPool but perhaps
>> other implementation can return Set if needed
>
>Since the keys are a Set, that seems like the most appropriate option.
>I'd be happy changing the existing method to use Set. We should think
>about switching the internal list as well, e.g. to a LinkedHashSet
>(would need to check behaviour was unchanged).
>
>> * Looking at [pool-1.6] -- there are a few problems with the code on
>>that
>> branch, not sure if this was abandoned or not?
>
>Absolutely not.
>
>> The layout of the project
>> is a bit non-maven, but that is fine. Checkstyle throws a bunch of
>>errors
>
>Checkstyle 'errors' are not bugs. Cleaner code is nice but not essential.
>
>> due to misconfiguration in pom.xml (which I have easily fixed). Also
>>there
>> are a bunch of FindBungs which I have fixed too.
>
>Based on previous experience they may or may not be valid.
>
>> There are a few unit
>> tests failing which I am looking at while also trying to slot in my
>>actual
>> commit (as per below) :)
>
>That sounds like your setup isn't quite right. The unit tests should
>always pass and did on my machine the last time I touched the 1.x code.
>There are also regular CI builds that check this.
>
>> So, what are the best practices here -- separate these into a bunch of
>> JIRA issues/patches :
>> 1/ checkstyle fixes
>> 2/ findbugs fixes
>> 3/ unit test fixes
>> 4/ my component
>> Or just put everything into one patch?
>
>One issue per Jira please.
>
>Mark
>
>
>---------------------------------------------------------------------
>To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [pool] Question / potential improvement on KeyedObjectPool
Posted by Mark Thomas <ma...@apache.org>.
On 30/07/2012 09:36, Liviu Tudor wrote:
> Thanks very much for pointing me in the right direction, Mark!
> As with all development tasks, this, however, opened up new questions --
> not entirely sure if these are best discussed on this list or whether
> occasionally I should open up a JIRA ticket for this so more than
> welcoming suggestions here.
Jira is not the place for discussions. That is best done on the dev
list. Once we have an agreed direction then we can open a Jira to track
the work.
> * on the [pool2] -- I haven't investigated the eviction mechanism in
> detail there, but I understand the need for a List and that's fine. My
> original idea was to provide a Set with all the keys for the
> KeyedObjectPool interface, as there is no need for ordering of the keys
> here (same as in Map.keySet) however, I can just as well have a List and
> provide a getKeys() in the KeyedObjectPool interface -- it will be used
> the same ultimately for my idea, which in fact needs just a collection
> (Set was preferred due to the fact that Map provides access to the keys
> via a Set and the fact that keys are unique obviously). So, in the light
> of that, which would be better to have in KeyedObjectPool interface (if
> any):
> 1/ Set<K> keySet(); //returns a Set with all the keys in the pool
> 2/ List<K> getKeys(); //this is already implemented part of the Mbean
> implementation in GenericKeyedObjectPool
> 3/ Collection<K> getKeys();//this allows us to customise it in
> subclasses, so we can have List in GenericKeyedObjectPool but perhaps
> other implementation can return Set if needed
Since the keys are a Set, that seems like the most appropriate option.
I'd be happy changing the existing method to use Set. We should think
about switching the internal list as well, e.g. to a LinkedHashSet
(would need to check behaviour was unchanged).
> * Looking at [pool-1.6] -- there are a few problems with the code on that
> branch, not sure if this was abandoned or not?
Absolutely not.
> The layout of the project
> is a bit non-maven, but that is fine. Checkstyle throws a bunch of errors
Checkstyle 'errors' are not bugs. Cleaner code is nice but not essential.
> due to misconfiguration in pom.xml (which I have easily fixed). Also there
> are a bunch of FindBungs which I have fixed too.
Based on previous experience they may or may not be valid.
> There are a few unit
> tests failing which I am looking at while also trying to slot in my actual
> commit (as per below) :)
That sounds like your setup isn't quite right. The unit tests should
always pass and did on my machine the last time I touched the 1.x code.
There are also regular CI builds that check this.
> So, what are the best practices here -- separate these into a bunch of
> JIRA issues/patches :
> 1/ checkstyle fixes
> 2/ findbugs fixes
> 3/ unit test fixes
> 4/ my component
> Or just put everything into one patch?
One issue per Jira please.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [pool] Question / potential improvement on KeyedObjectPool
Posted by Liviu Tudor <li...@gmail.com>.
Thanks very much for pointing me in the right direction, Mark!
As with all development tasks, this, however, opened up new questions --
not entirely sure if these are best discussed on this list or whether
occasionally I should open up a JIRA ticket for this so more than
welcoming suggestions here.
* on the [pool2] -- I haven't investigated the eviction mechanism in
detail there, but I understand the need for a List and that's fine. My
original idea was to provide a Set with all the keys for the
KeyedObjectPool interface, as there is no need for ordering of the keys
here (same as in Map.keySet) however, I can just as well have a List and
provide a getKeys() in the KeyedObjectPool interface -- it will be used
the same ultimately for my idea, which in fact needs just a collection
(Set was preferred due to the fact that Map provides access to the keys
via a Set and the fact that keys are unique obviously). So, in the light
of that, which would be better to have in KeyedObjectPool interface (if
any):
1/ Set<K> keySet(); //returns a Set with all the keys in the pool
2/ List<K> getKeys(); //this is already implemented part of the Mbean
implementation in GenericKeyedObjectPool
3/ Collection<K> getKeys();//this allows us to customise it in
subclasses, so we can have List in GenericKeyedObjectPool but perhaps
other implementation can return Set if needed
* Looking at [pool-1.6] -- there are a few problems with the code on that
branch, not sure if this was abandoned or not? The layout of the project
is a bit non-maven, but that is fine. Checkstyle throws a bunch of errors
due to misconfiguration in pom.xml (which I have easily fixed). Also there
are a bunch of FindBungs which I have fixed too. There are a few unit
tests failing which I am looking at while also trying to slot in my actual
commit (as per below) :)
So, what are the best practices here -- separate these into a bunch of
JIRA issues/patches :
1/ checkstyle fixes
2/ findbugs fixes
3/ unit test fixes
4/ my component
Or just put everything into one patch?
Thanks,
Liv
Liviu Tudor
E: liviu.tudor@gmail.com
C: +1 650-2833815 (USA)
M: +44 (0)7591540313 (UK, Europe)
W: http://about.me/liviutudor
Skype: liviutudor
I'm nobody, nobody's perfect -- therefore I'm perfect!
On 24/07/2012 09:32, "Mark Thomas" <ma...@apache.org> wrote:
>On 24/07/2012 08:42, Liviu Tudor wrote:
>> Phil & others who have been in touch with [pool],
>>
>> A follow up to this: I'm trying to log this in JIRA and submit a patch
>>but
>> I've come across the following problem:
>>
>> 1/ The mechanism I am referring to had pool-1.6.0 in mind, however, I
>> can't find in SVN the branch for 1.6? There are release branches going
>>up
>> to 1.5 but no 1.6? Can you point me in the right direction so I can
>>create
>> the patch in the right branch?
>
>Pool 2.x: http://svn.apache.org/repos/asf/commons/proper/pool/trunk/
>Pool 1.6:
>http://svn.apache.org/repos/asf/commons/proper/pool/branches/POOL_1_X/
>Pool 1.5:
>http://svn.apache.org/repos/asf/commons/proper/pool/branches/1_5_RELEASE/
>
>In theory pool 1.5.x + conversion to generics = pool 1.6.x
>
>Pool 2.x is a complete re-write of the pooling mechanism.
>
>> 2/ Since we've moved now to pool2, I would like to apply something
>>similar
>> for [pool2], however, looking at the code, I see we now have a
>>
>> List<K> getKeys();
>>
>> Due to the Mbean interface we're implementing. This looks similar to my
>> idea of returning a Set<K> for the keys -- since the order of the keys
>>of
>> the cache is not important I thought. Looking at the code it turns out
>> indeed the order is not important, so List is not justified and Set
>>could
>> have done it just fine. So am I ok to change the Mbean and the class to
>> actually use a Set rather than a List?
>
>No. Order is important for eviction. Internally it has to be a List.
>Externally, the keys could be returned as a Set or a List. I don't have
>any strong feelings. Regardless, we should more clearly (there is a
>comment already) document the requirement for ordering of keys for
>eviction somewhere in the code since it doesn't appear to be clear enough.
>
>Mark
>
>
>---------------------------------------------------------------------
>To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [pool] Question / potential improvement on KeyedObjectPool
Posted by Mark Thomas <ma...@apache.org>.
On 24/07/2012 08:42, Liviu Tudor wrote:
> Phil & others who have been in touch with [pool],
>
> A follow up to this: I'm trying to log this in JIRA and submit a patch but
> I've come across the following problem:
>
> 1/ The mechanism I am referring to had pool-1.6.0 in mind, however, I
> can't find in SVN the branch for 1.6? There are release branches going up
> to 1.5 but no 1.6? Can you point me in the right direction so I can create
> the patch in the right branch?
Pool 2.x: http://svn.apache.org/repos/asf/commons/proper/pool/trunk/
Pool 1.6:
http://svn.apache.org/repos/asf/commons/proper/pool/branches/POOL_1_X/
Pool 1.5:
http://svn.apache.org/repos/asf/commons/proper/pool/branches/1_5_RELEASE/
In theory pool 1.5.x + conversion to generics = pool 1.6.x
Pool 2.x is a complete re-write of the pooling mechanism.
> 2/ Since we've moved now to pool2, I would like to apply something similar
> for [pool2], however, looking at the code, I see we now have a
>
> List<K> getKeys();
>
> Due to the Mbean interface we're implementing. This looks similar to my
> idea of returning a Set<K> for the keys -- since the order of the keys of
> the cache is not important I thought. Looking at the code it turns out
> indeed the order is not important, so List is not justified and Set could
> have done it just fine. So am I ok to change the Mbean and the class to
> actually use a Set rather than a List?
No. Order is important for eviction. Internally it has to be a List.
Externally, the keys could be returned as a Set or a List. I don't have
any strong feelings. Regardless, we should more clearly (there is a
comment already) document the requirement for ordering of keys for
eviction somewhere in the code since it doesn't appear to be clear enough.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [pool] Question / potential improvement on KeyedObjectPool
Posted by Liviu Tudor <li...@gmail.com>.
Phil & others who have been in touch with [pool],
A follow up to this: I'm trying to log this in JIRA and submit a patch but
I've come across the following problem:
1/ The mechanism I am referring to had pool-1.6.0 in mind, however, I
can't find in SVN the branch for 1.6? There are release branches going up
to 1.5 but no 1.6? Can you point me in the right direction so I can create
the patch in the right branch?
2/ Since we've moved now to pool2, I would like to apply something similar
for [pool2], however, looking at the code, I see we now have a
List<K> getKeys();
Due to the Mbean interface we're implementing. This looks similar to my
idea of returning a Set<K> for the keys -- since the order of the keys of
the cache is not important I thought. Looking at the code it turns out
indeed the order is not important, so List is not justified and Set could
have done it just fine. So am I ok to change the Mbean and the class to
actually use a Set rather than a List?
Liv
Liviu Tudor
E: liviu.tudor@gmail.com
C: +1 650-2833815 (USA)
M: +44 (0)7591540313 (UK, Europe)
W: http://about.me/liviutudor
Skype: liviutudor
I'm nobody, nobody's perfect -- therefore I'm perfect!
On 11/07/2012 21:21, "Phil Steitz" <ph...@gmail.com> wrote:
>On 7/10/12 2:51 PM, Liviu Tudor wrote:
>> Hi guys,
>>
>> I have a question regarding [pool] -- which might turned out to be
>>either a
>> request for change or a potential improvement patch (though it could be
>>the
>> case I suppose that I've misunderstood certain things, in which case it
>>will
>> just turn out into my wasting people's time -- hopefully not though :)
>>
>> So a bit of context: I'm using [pool] in one of the projects I'm
>>working on,
>> more specifically I'm using a GenericKeyedObjectPool. This is actually
>>used
>> as a "create (instances) and cache (them) ahead" in the sense that it's
>>used
>> to create in advance N instances of certain classes and pool them. All
>>these
>> classes are "script classes" -- in the sense that we do allow in this
>> application a few hooks where we can execute user-written Java code,
>>which
>> obviously are not known ahead ‹ and because the application is a rather
>> high-throughput one, rather than creating these instances on demand
>>(when
>> execution of the program reaches the "hook" point and needs to run the
>> "script") I've decided to have a pool-based implementation which will
>>create
>> in advance N instances of these "script" classes and then when the
>>"hook"
>> point is used, simply use one of the already-created objects from the
>>pool ‹
>> thus decreasing the execution time. Without going into details, some of
>> these instances can be re-used and added back to the pool, while others
>>are
>> simply a one-off execution, after which the class needs to be destroyed
>>‹
>> however, in both cases, I would like to keep around N instances
>>pre-created
>> in the pool, for each class type.
>>
>> Now, my idea ‹ and this is where (finally!) my question/suggestion
>>comes in
>> ‹ is to have a background thread (a ScheduledExecutorService, scheduled
>>at
>> fixed intervals) which kicks in and goes through all the keys in the
>>pool
>> and for each such key it uses the KeyedObjectPoolableFactory to create
>>up to
>> N instances for each key (based of course also on the getNumIdle(Key)
>> return).
>>
>> The problem with that ‹ as I found out soon ‹ is that there is no way to
>> access the set of keys in a KeyedObjectPool! I was expecting something
>>upon
>> the lines of standard JDK collections :
>>
>> Set<K> keySet();
>>
>> or similar, however, there isn't anything like that around. So question
>> number one is : Should we consider making such a method available? (As
>>far
>> as I can see, the GenericKeyedObjectPool uses a poolMap member
>>internally so
>> we could easily wrap up the keySet() into an unmodifiable set or
>>similar if
>> that makes sense.)
>
>This seems a reasonable enhancement request. Can you please open a
>JIRA for it. That way you can also attach patches ;)
>
>The tricky bit will be how to do this with integrity but without
>performance impact.
>
>Phil
>>
>> Alternatively ‹ and this is question two, and where this email can turn
>>into
>> a proposal for a commit of a new component/class ‹ does it make sense to
>> have a specialised KeyedObjectPool which does what I described above?
>>The
>> way I'm doing the pre population is by keeping a set of all the keys
>>that
>> were requested/removed (through borrowObject/preparePool/etc) -- and the
>> background thread iterates through these keys and creates instances
>>where
>> necessary. (I am happy of course to provide this implementation to ASF
>>as a
>> patch request or similar.)
>>
>> Hopefully I managed to explain what I'm trying to achieve here ‹ but
>>happy
>> to provide more details where necessary.
>>
>> Many thanks in advance,
>>
>>
>>
>> Liv
>>
>>
>> Liviu Tudor
>>
>> E: liviu.tudor@gmail.com <ma...@gmail.com>
>> C: +1 650-2833815 (USA)
>> M: +44 (0)7591540313 (UK, Europe)
>> W: http://about.me/liviutudor <http://about.me/liviutudor>
>> Skype: liviutudor
>>
>> I'm nobody, nobody's perfect -- therefore I'm perfect!
>>
>>
>>
>>
>>
>>
>
>
>
>---------------------------------------------------------------------
>To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [pool] Question / potential improvement on KeyedObjectPool
Posted by Phil Steitz <ph...@gmail.com>.
On 7/10/12 2:51 PM, Liviu Tudor wrote:
> Hi guys,
>
> I have a question regarding [pool] -- which might turned out to be either a
> request for change or a potential improvement patch (though it could be the
> case I suppose that I've misunderstood certain things, in which case it will
> just turn out into my wasting people's time -- hopefully not though :)
>
> So a bit of context: I'm using [pool] in one of the projects I'm working on,
> more specifically I'm using a GenericKeyedObjectPool. This is actually used
> as a "create (instances) and cache (them) ahead" in the sense that it's used
> to create in advance N instances of certain classes and pool them. All these
> classes are "script classes" -- in the sense that we do allow in this
> application a few hooks where we can execute user-written Java code, which
> obviously are not known ahead ‹ and because the application is a rather
> high-throughput one, rather than creating these instances on demand (when
> execution of the program reaches the "hook" point and needs to run the
> "script") I've decided to have a pool-based implementation which will create
> in advance N instances of these "script" classes and then when the "hook"
> point is used, simply use one of the already-created objects from the pool ‹
> thus decreasing the execution time. Without going into details, some of
> these instances can be re-used and added back to the pool, while others are
> simply a one-off execution, after which the class needs to be destroyed ‹
> however, in both cases, I would like to keep around N instances pre-created
> in the pool, for each class type.
>
> Now, my idea ‹ and this is where (finally!) my question/suggestion comes in
> ‹ is to have a background thread (a ScheduledExecutorService, scheduled at
> fixed intervals) which kicks in and goes through all the keys in the pool
> and for each such key it uses the KeyedObjectPoolableFactory to create up to
> N instances for each key (based of course also on the getNumIdle(Key)
> return).
>
> The problem with that ‹ as I found out soon ‹ is that there is no way to
> access the set of keys in a KeyedObjectPool! I was expecting something upon
> the lines of standard JDK collections :
>
> Set<K> keySet();
>
> or similar, however, there isn't anything like that around. So question
> number one is : Should we consider making such a method available? (As far
> as I can see, the GenericKeyedObjectPool uses a poolMap member internally so
> we could easily wrap up the keySet() into an unmodifiable set or similar if
> that makes sense.)
This seems a reasonable enhancement request. Can you please open a
JIRA for it. That way you can also attach patches ;)
The tricky bit will be how to do this with integrity but without
performance impact.
Phil
>
> Alternatively ‹ and this is question two, and where this email can turn into
> a proposal for a commit of a new component/class ‹ does it make sense to
> have a specialised KeyedObjectPool which does what I described above? The
> way I'm doing the pre population is by keeping a set of all the keys that
> were requested/removed (through borrowObject/preparePool/etc) -- and the
> background thread iterates through these keys and creates instances where
> necessary. (I am happy of course to provide this implementation to ASF as a
> patch request or similar.)
>
> Hopefully I managed to explain what I'm trying to achieve here ‹ but happy
> to provide more details where necessary.
>
> Many thanks in advance,
>
>
>
> Liv
>
>
> Liviu Tudor
>
> E: liviu.tudor@gmail.com <ma...@gmail.com>
> C: +1 650-2833815 (USA)
> M: +44 (0)7591540313 (UK, Europe)
> W: http://about.me/liviutudor <http://about.me/liviutudor>
> Skype: liviutudor
>
> I'm nobody, nobody's perfect -- therefore I'm perfect!
>
>
>
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org