On 10/25/10 11:34 AM, Phil Steitz wrote:
On 10/25/10 11:22 AM, Steven Siebert wrote:
Phil,

I don't think we have removed the ability to access the config
options
programmatically from the config instance. You can either get a
reference
to it post-construct/build or from the getConfig() method on the
pool.
Non-config properties remain the same, accessible through the pool
API.
The JMX instance would be the only class not exposed public
(internal to
the pool instance), which would access the config properties
through the
accessors/mutators (which is why I requested the config classes be
made
thread-safe).

(come to think of it, I don't see a getConfig() on at least one
pool....add
this?)

I could very well be missing something, though. I'm working from
home now,
working on a separate project with nothing to do with pool, and
trying to
consider this at the same time =)

There is no getConfig() and the point I was making is that I don't
think we should add that. Better separation of concerns, IMO, is to
have the Config classes available only to the constructors and have
the pool expose runtime properties directly, more or less as it does
now.

I also agree with James' comment on POOL-174 that logically the
Config classes should be immutable. I say "logically" because some
use cases may require that they can have properties injected by
setters.


I notice now what I missed on initial review of Simo's patch - the pool accessors now manage the config properties via persisted Config members. I am OK with this, but it now means that the Config classes have to be mutable. What needs to be threadsafe, however, is the pool itself. Given that you can't rely on Config locks to ensure correctness for the pool (i.e. the pool-exposed mutators are still going to have to lock the pool itself), making Config accessors synchronized is just adding extra synch.

Phil
Phil

S

On Mon, Oct 25, 2010 at 11:10 AM, Phil
Steitz<phil.ste...@gmail.com> wrote:

On 10/25/10 9:06 AM, Steven Siebert wrote:

Hi Simone,

You have two +1's waiting for you in the JIRA comments =)

My comments from tracker:

"I took a look at this last night but didn't get a chance to
comment =)

I like the patch, I believe this does indeed satisfy the issue.

One question I have, since we're eliminating the primitive
configuration
properties within the pool/factory classes, we're making the Config
objects
publicly accessible, and possibly accessing through JMX is the
idea of
making the Config objects thread-safe. This would certainly
reduce the
need
to have to externally synchronize (and possibly introduce bugs)
every
time.


Sorry, I missed this before commenting on POOL-174. I don't see
it working
this way - i.e., providing public access to the config instance
used to
construct the pool at runtime for JMX or other purposes. Pool
properties
should still be exposed via threadsafe accessors / mutators,
including
runtime properties not set by the config (e.g. numActive, numIdle).

Phil


Another issue we probably need to open another ticket for is to
deprecate
the constructors we've eliminated in 1.5.

Last suggestion/question is about making inner (public static
final)
Builder
pattern classes within the concrete Config classes (and possibly
defining
an
abstract<T extends Abstract*Config> create() method in the Abstract
class).
This would further simplify the programmatic creation of the Config
classes.

Thoughts?

+1 on Simones patch...we can add any of the above after it has been
committed."


Respectfully,


Steve



On Mon, Oct 25, 2010 at 8:36 AM, Simone
Tripodi<simone.trip...@gmail.com
wrote:

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://people.apache.org/%7Esimonetripodi/>

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://people.apache.org/%7Esimonetripodi/>

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://people.apache.org/%7Esimonetripodi/>

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://people.apache.org/%7Esimonetripodi/>

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://people.apache.org/%7Esimonetripodi/>

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://people.apache.org/%7Esimonetripodi/>

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





---------------------------------------------------------------------


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

Reply via email to