commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Liviu Tudor <liviu.tu...@gmail.com>
Subject Re: [pool] Question / potential improvement on KeyedObjectPool
Date Mon, 30 Jul 2012 20:07:25 GMT
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" <markt@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


Mime
View raw message