Hi all mates, I updated the jira issue uploading my patch; it contains the configuration extraction and some code modification. IMHO we shouldn't replicate the same data in both configuration AND factory/pool, when creating the factory/pool it is enough storing the configuration reference, just use it. I intentionally missed the interfaces layer, since they can be added directly in the JMX support in the required form. Please take a look at the patch and provide feedbacks, if you agree I could start committing the modifications and proceed on JMX support. Have a nice day, Simo
http://people.apache.org/~simonetripodi/ http://www.99soft.org/ On Fri, Oct 22, 2010 at 5:23 AM, Gary Gregory <ggreg...@seagullsoftware.com> wrote: >> -----Original Message----- >> From: Steven Siebert [mailto:smsi...@gmail.com] >> Sent: Thursday, October 21, 2010 18:08 >> To: Commons Developers List >> Subject: Re: [pool] Reusing Config >> >> Gary, >> >> Great work so far. I'm checking out the diffs now, I'm gonna hack out some >> simple UML "diffs", if only to wrap my head around it all. I'll upload the >> file to the issue once complete. >> >> BTW, I hope I didn't offend with the 'academic' comment, I >> most certainly did not intend to infer that there weren't functional >> importances to this issue. I was mostly trying to delineate the two issues >> in my mind, and putting it to "paper" was a good way to do that =) >> >> Cheers, >> >> S > > Hi Steven, > > No offense even considered from this end :) > > I'm glad we are going through this exercise. This will improve the software I > am sure. > > Gary > >> >> >> On Thu, Oct 21, 2010 at 3:35 PM, Gary Gregory >> <ggreg...@seagullsoftware.com>wrote: >> >> > > -----Original Message----- >> > > From: Phil Steitz [mailto:phil.ste...@gmail.com] >> > > Sent: Thursday, October 21, 2010 06:29 >> > > To: Commons Developers List >> > > Subject: Re: [pool] Reusing Config >> > > >> > > On 10/21/10, Simone Tripodi <simone.trip...@gmail.com> wrote: >> > > > it seems you've been doing a very good work, the only thing I *suggest* >> > is >> > > > >> > > > * simplifying the mutable/immutable interfaces, one interface for >> > > > already known common (im)mutable fields should be enough; >> > > > * adding/renaming the interfaces with the <PoolName>`MBean` postfix to >> > > > be ready for JMX support; >> > > > >> > > > btw it seems you're now much more deep inside the topic than me ;) >> > > > >> > > > WDYT? >> > > > Simo >> > > > >> > > >> > > Sorry I have been a little slow on this. I will have a careful look >> > > this eve. Based on a very quick review, I am +1 on the idea and >> > > approach to separate mutable / immutable. Also +1 for JMX support. >> > > Two quick things to keep top of mind: >> > > >> > > 1. Please make sure not to lose documentation. Whatever is >> > > documented today in protected field / internal getters / setters docs >> > > needs to be carried forward. >> > >> > Check. I did not check as I refactored that Javadocs were in the right >> > places. That would be a requirement for a real patch. I only meant this as >> > an experiment that went a lot further than I thought. >> > >> > > >> > > 2. Somewhat related - I am fine just plowing ahead for now using >> > > existing API concepts, but some of those concepts are anachronistic or >> > > broken, IMO, so we may later decide to revamp much of the "accounting" >> > > aspects of the API. That we should and will discuss on other >> > > threads. One thing that might be good to think about at this point, >> > > however, is getting rid of primitive properties (we started that with >> > > whenExhaustedAction). I think there is a DBCP issue on this raised by >> > > Dain a couple of years ago. >> > >> > It would be nice to track this someplace, I am not sure if Javadoc is the >> > right place. >> > >> > Gary >> > >> > > >> > > Thanks all for moving this along! >> > > >> > > Phil >> > > > http://people.apache.org/~simonetripodi/ >> > > > http://www.99soft.org/ >> > > > >> > > > >> > > > >> > > > On Thu, Oct 21, 2010 at 8:23 AM, Gary Gregory >> > > > <ggreg...@seagullsoftware.com> wrote: >> > > >>> -----Original Message----- >> > > >>> From: Simone Tripodi [mailto:simone.trip...@gmail.com] >> > > >>> Sent: Wednesday, October 20, 2010 22:41 >> > > >>> To: Commons Developers List >> > > >>> Subject: Re: [pool] Reusing Config >> > > >>> >> > > >>> Hi Gary! >> > > >>> unfortunately the link replied with 404 code, can you give me please >> > > >>> the issue ID? >> > > >> >> > > >> It's https://issues.apache.org/jira/browse/POOL-173 >> > > >> >> > > >> I've updated the diff file a couple of times since my initial msg. >> > > >> >> > > >> Gary >> > > >> >> > > >>> Many thanks in advance, have a nice day!!! >> > > >>> Simo >> > > >>> >> > > >>> http://people.apache.org/~simonetripodi/ >> > > >>> http://www.99soft.org/ >> > > >>> >> > > >>> >> > > >>> >> > > >>> On Thu, Oct 21, 2010 at 12:16 AM, Gary Gregory >> > > >>> <ggreg...@seagullsoftware.com> wrote: >> > > >>> > Hi Simone, >> > > >>> > >> > > >>> > Please see my experiment in progress here >> > > >>> >> > https://issues.apache.org/jira/secure/attachment/12457710/pool2config.diff >> > > >>> > >> > > >>> > Gary Gregory >> > > >>> > Senior Software Engineer >> > > >>> > Rocket Software >> > > >>> > 3340 Peachtree Road, Suite 820 * Atlanta, GA 30326 * USA >> > > >>> > Tel: +1.404.760.1560 >> > > >>> > Email: ggreg...@seagullsoftware.com >> > > >>> > Web: seagull.rocketsoftware.com >> > > >>> > >> > > >>> > >> > > >>> > >> > > >>> >> -----Original Message----- >> > > >>> >> From: Simone Tripodi [mailto:simone.trip...@gmail.com] >> > > >>> >> Sent: Wednesday, October 20, 2010 14:53 >> > > >>> >> To: Commons Developers List >> > > >>> >> Subject: Re: [pool] Reusing Config >> > > >>> >> >> > > >>> >> Hi, >> > > >>> >> sorry for not having been clear, but in my previous email my >> > intent >> > > >>> >> was saying that depending on how we manage the Config class, it >> > could >> > > >>> >> influence de JMX support design, nothing more, and since I'm not >> > > >>> >> expert on JMX I was waiting for feedbacks from who knows more than >> > me >> > > >>> >> >> > > >>> >> About Gary's question, I had the following thought >> > > >>> >> >> > > >>> >> AbstractGenericObjectPoolConfig >> > > >>> >> - int maxIdle >> > > >>> >> - int minIdle >> > > >>> >> - int maxActive >> > > >>> >> - long maxWait >> > > >>> >> - WhenExhaustedAction whenExhaustedAction >> > > >>> >> - boolean testOnBorrow >> > > >>> >> - boolean testOnReturn >> > > >>> >> - boolean testWhileIdle >> > > >>> >> - long timeBetweenEvictionRunsMillis >> > > >>> >> - int numTestsPerEvictionRun >> > > >>> >> - long minEvictableIdleTimeMillis >> > > >>> >> - boolean lifo >> > > >>> >> >> > > >>> >> GenericObjectPoolConfig extends AbstractGenericObjectPoolConfig >> > > >>> >> - long softMinEvictableIdleTimeMillis >> > > >>> >> >> > > >>> >> GenericKeyedObjectPoolConfig extends GenericObjectPoolConfig >> > > >>> >> - int maxTotal >> > > >>> >> >> > > >>> >> About the pools: >> > > >>> >> >> > > >>> >> class GenericObjectPool { >> > > >>> >> + GenericObjectPool(GenericObjectPoolFactory factory) { >> > > >>> >> this(factory, new GenericObjectPoolConfig()); >> > > >>> >> } >> > > >>> >> >> > > >>> >> + GenericObjectPool(GenericObjectPoolFactory factory, >> > > >>> >> GenericObjectPoolConfig config) {...} >> > > >>> >> >> > > >>> >> + GenericObjectPoolConfig getConfig() {...} >> > > >>> >> } >> > > >>> >> >> > > >>> >> same thing for the Keyed version. >> > > >>> >> >> > > >>> >> Too simple and stupid? Maybe. But reduces the redundancies to 0. >> > > >>> >> Moreover I'm not sure it is just an academical way to approach the >> > > >>> >> issue, I'm sure it is more than pragmatic, simplifying the >> > > >>> >> maintainability and makes easier keep in synch the Pool and >> > related >> > > >>> >> Factory configuration. >> > > >>> >> Just my 2 cents, now off to bed due my local timezone :P >> > > >>> >> Simo >> > > >>> >> >> > > >>> >> http://people.apache.org/~simonetripodi/ >> > > >>> >> http://www.99soft.org/ >> > > >>> >> >> > > >>> >> >> > > >>> >> >> > > >>> >> On Wed, Oct 20, 2010 at 10:40 PM, Gary Gregory >> > > >>> >> <ggreg...@seagullsoftware.com> wrote: >> > > >>> >> > So I am doing an experimental refactoring to see what the code >> > would >> > > >>> >> > look >> > > >>> >> like with a Config class extracted and I ran into the following. >> > > >>> >> > >> > > >>> >> > The class GenericObjectPool has an >> > _softMinEvictableIdleTimeMillis >> > > >>> >> > ivar >> > > >>> but >> > > >>> >> the equivalent GenericKeyedObjectPool does not. >> > > >>> >> > >> > > >>> >> > Is that a little hole in implementation that could have been >> > avoided >> > > >>> >> > with >> > > >>> a >> > > >>> >> common classes used for config? Even if GenericKeyedObjectPool >> > would >> > > >>> >> throw >> > > >>> a >> > > >>> >> "not implemented" exception. >> > > >>> >> > >> > > >>> >> > Thoughts? >> > > >>> >> > >> > > >>> >> > Gary Gregory >> > > >>> >> > Senior Software Engineer >> > > >>> >> > Rocket Software >> > > >>> >> > 3340 Peachtree Road, Suite 820 * Atlanta, GA 30326 * USA >> > > >>> >> > Tel: +1.404.760.1560 >> > > >>> >> > Email: ggreg...@seagullsoftware.com >> > > >>> >> > Web: seagull.rocketsoftware.com >> > > >>> >> > >> > > >>> >> > >> > > >>> >> > >> > > >>> >> >> -----Original Message----- >> > > >>> >> >> From: Simone Tripodi [mailto:simone.trip...@gmail.com] >> > > >>> >> >> Sent: Wednesday, October 20, 2010 12:22 >> > > >>> >> >> To: Commons Developers List >> > > >>> >> >> Subject: Re: [pool] Reusing Config >> > > >>> >> >> >> > > >>> >> >> sure, I always wait for feedbacks before coding :P Cool >> > expression >> > > >>> >> >> "Rambo through the code", that was the first time I read it and >> > > >>> >> >> made >> > > >>> >> >> me laugh :D >> > > >>> >> >> All the best, >> > > >>> >> >> Simo >> > > >>> >> >> >> > > >>> >> >> http://people.apache.org/~simonetripodi/ >> > > >>> >> >> http://www.99soft.org/ >> > > >>> >> >> >> > > >>> >> >> >> > > >>> >> >> >> > > >>> >> >> On Wed, Oct 20, 2010 at 9:17 PM, Gary Gregory >> > > >>> >> >> <ggreg...@seagullsoftware.com> wrote: >> > > >>> >> >> > It seems to me there is a reason the code is the way it is so >> > I'd >> > > >>> really >> > > >>> >> >> like to hear thoughts from some of the original authors before >> > we >> > > >>> >> >> go and >> > > >>> >> Rambo >> > > >>> >> >> through the code ;) >> > > >>> >> >> > >> > > >>> >> >> > Gary >> > > >>> >> >> > >> > > >>> >> >> > On Oct 20, 2010, at 12:13, "Simone Tripodi" >> > > >>> >> >> > <simone.trip...@gmail.com> >> > > >>> >> >> wrote: >> > > >>> >> >> > >> > > >>> >> >> >> Hi Gary, >> > > >>> >> >> >> yes that's me that raised the question[1] and discussed a >> > little >> > > >>> >> >> >> with >> > > >>> >> >> >> Seb. What blocked me was the JMX support proposal since I'm >> > not >> > > >>> >> >> >> familiar with that technology, so I was consulting >> > documentation >> > > >>> >> >> >> to >> > > >>> >> >> >> study. >> > > >>> >> >> >> >> > > >>> >> >> >> My very big +1 for that, with the wish of work directly on >> > that >> > > >>> stuff. >> > > >>> >> >> >> Anyone else has a different thought, before proceeding? >> > > >>> >> >> >> Thanks in advance, >> > > >>> >> >> >> Simo >> > > >>> >> >> >> >> > > >>> >> >> >> [1] http://markmail.org/message/q4y7ghux57s7hk6v >> > > >>> >> >> >> >> > > >>> >> >> >> http://people.apache.org/~simonetripodi/ >> > > >>> >> >> >> http://www.99soft.org/ >> > > >>> >> >> >> >> > > >>> >> >> >> >> > > >>> >> >> >> >> > > >>> >> >> >> On Wed, Oct 20, 2010 at 7:43 PM, Gary Gregory >> > > >>> >> >> >> <ggreg...@seagullsoftware.com> wrote: >> > > >>> >> >> >>> In the same department, I see the following ivars: >> > > >>> >> >> >>> >> > > >>> >> >> >>> lifo : boolean >> > > >>> >> >> >>> maxActive : int >> > > >>> >> >> >>> maxIdle : int >> > > >>> >> >> >>> maxTotal : int >> > > >>> >> >> >>> maxWait : long >> > > >>> >> >> >>> minEvictableIdleTimeMillis : long >> > > >>> >> >> >>> minIdle : int >> > > >>> >> >> >>> numTestsPerEvictionRun : int >> > > >>> >> >> >>> testOnBorrow : boolean >> > > >>> >> >> >>> testOnReturn : boolean >> > > >>> >> >> >>> testWhileIdle : boolean >> > > >>> >> >> >>> timeBetweenEvictionRunsMillis : long >> > > >>> >> >> >>> whenExhaustedAction : WhenExhaustedAction >> > > >>> >> >> >>> >> > > >>> >> >> >>> defined in four classes: >> > > >>> >> >> >>> >> > > >>> >> >> >>> GenericKeyedObjectPool >> > > >>> >> >> >>> GenericKeyedObjectPoolFactory >> > > >>> >> >> >>> GenericObjectPool >> > > >>> >> >> >>> GenericObjectPoolFactory >> > > >>> >> >> >>> >> > > >>> >> >> >>> Which feels to me like a missed opportunity to avoid >> > > >>> >> >> >>> duplication. >> > > >>> >> >> >>> >> > > >>> >> >> >>> Is making one ivar private or final or volatile be applied >> > to >> > > >>> >> >> >>> all >> > > >>> four >> > > >>> >> >> classes? >> > > >>> >> >> >>> >> > > >>> >> >> >>> We could: >> > > >>> >> >> >>> >> > > >>> >> >> >>> Use a config object instead of the 13 ivars. >> > > >>> >> >> >>> Or a common superclass then we can consider if it should >> > hold >> > > >>> >> >> >>> the >> > > >>> ivar >> > > >>> >> >> list or a Config object. >> > > >>> >> >> >>> >> > > >>> >> >> >>> Would it be too weird to have a common super class for >> > > >>> BaseObjectPool >> > > >>> >> and >> > > >>> >> >> BasePoolableObjectFactory for example? >> > > >>> >> >> >>> >> > > >>> >> >> >>> Gary Gregory >> > > >>> >> >> >>> Senior Software Engineer >> > > >>> >> >> >>> Rocket Software >> > > >>> >> >> >>> 3340 Peachtree Road, Suite 820 . Atlanta, GA 30326 . USA >> > > >>> >> >> >>> Tel: +1.404.760.1560 >> > > >>> >> >> >>> Email: ggreg...@seagullsoftware.com >> > > >>> >> >> >>> Web: seagull.rocketsoftware.com >> > > >>> >> >> >>> >> > > >>> >> >> >>> >> > > >>> >> >> >>> >> > > >>> >> >> >>>> -----Original Message----- >> > > >>> >> >> >>>> From: Gary Gregory [mailto:ggreg...@seagullsoftware.com] >> > > >>> >> >> >>>> Sent: Wednesday, October 20, 2010 10:29 >> > > >>> >> >> >>>> To: Commons Developers List >> > > >>> >> >> >>>> Subject: [pool] Reusing Config >> > > >>> >> >> >>>> >> > > >>> >> >> >>>> Hi All: >> > > >>> >> >> >>>> >> > > >>> >> >> >>>> I think this came up recently. Any thoughts or plans on >> > > >>> >> >> >>>> extracting >> > > >>> the >> > > >>> >> >> Config >> > > >>> >> >> >>>> class out of GenericKeyedObjectPool and GenericObjectPool >> > so >> > > >>> >> >> >>>> it can >> > > >>> be >> > > >>> >> >> reused. >> > > >>> >> >> >>>> The constants for default values could then also be moved >> > to >> > > >>> Config. >> > > >>> >> >> >>>> Gary Gregory >> > > >>> >> >> >>>> Senior Software Engineer >> > > >>> >> >> >>>> Rocket Software >> > > >>> >> >> >>>> 3340 Peachtree Road, Suite 820 * Atlanta, GA 30326 * USA >> > > >>> >> >> >>>> Tel: +1.404.760.1560 >> > > >>> >> >> >>>> Email: >> > > >>> >> ggreg...@seagullsoftware.com<mailto:ggreg...@seagullsoftware.com> >> > > >>> >> >> >>>> Web: >> > > >>> >> seagull.rocketsoftware.com<http://www.seagull.rocketsoftware.com/ >> > > >> > > >>> >> >> >>>> >> > > >>> >> >> >>> >> > > >>> >> >> >>> >> > > >>> >> >> >>> >> > ---------------------------------------------------------------- >> > > ---- >> > > >>> - >> > > >>> >> >> >>> 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 >> > > >>> >> >> > >> > > >>> >> >> > >> > > >>> >> >> >> > > >>> >> >> >> > -------------------------------------------------------------------- >> > > - >> > > >>> >> >> 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 >> > > >> >> > > >> >> > > > >> > > > --------------------------------------------------------------------- >> > > > 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 >> > >> > > > --------------------------------------------------------------------- > 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
