Refactoring

[weston.price]

Currently, on the 4.0.5 code base I have done the following:

Moved all pool related source into a newly created package

org.jboss.resource.pool

This removes the inner classes from the JBossManagedConnectionPoolMBean implementation and is overall, a cleaner way to organize things (at least in IMO).

I would like to do something similar with the jdbc supporting stuff (ie ValidConnectionChecker, ExceptionSorter etc, etc) and get that stuff out of the adapter structure.

I was thinking something along the lines of

org.jboss.resource.jdbc.support

and refactoring from there.

Thoughts?

[weston.price]

New packages:

org.jboss.resource.jdbc.support

org.jboss.resource.jdbc.support.vendor

The former includes the

Cached*
BaseClasses/Interfaces


The latter

Vendor specific stuff.

[adrian.brock]

You cannot change any interface or user level class.
You break somebody else's compilation.

That is unless you leave the interface in place and then
do:

public class ValidConnectionChecker extends org.jboss.new.location.ValidConnectionChecker

I'm against refactoring just for the sake of it.
i.e. no good reason like just changing names or packages.

There are some people who have access to the codebase that
I would like to go their IDE and put a big sticker over the refactoring
button saying "leave it alone and do some real work!". :-)
or
"leave it alone until you learn to be more careful about breaking the build!".

5.0.x (major version number change)
is the place where you can change things incompatiblity.
And then only if there is a good reason for it.

[weston.price]

It's not refactoring for the sake of refactoring. In terms of the JDBC support, this should have never been in the "adapter" package to begin with.

As far as the 'real work' comment, not sure about that one :-) And as far as I remember I haven't broken the build...well, that one time due to the 1.4/1.5 stuff. Making this easier to read, understand etc, etc. may actually alleviate some of the 'People just don't grok JCA' mantra.

In terms of the pool classes, The JBossManagedConnectionPool/MBean remains in the same place. The only thing that has been moved is the BasePool and all it's children along with the management threads (PoolFiller, IdleRemover, ConnectionValidator) which should never be used directly anyway.

[weston.price]

And, the build works fine (with the changes), testsuite runs fine etc. etc.

I do actually do these things before checking stuff in you know :-)

[adrian.brock]

It wasn't aimed at you, more at the webservices project. :-)

My aversion to refactoring still stands.
If it doesn't serve a good purpose.

On the jdbc support classes, I don't see how these
aren't a part of the resource adapter anyway?

1) They are specific to jdbc
2) The rar wouldn't work without them
3) The rar shouldn't depend upon classes from the connection manager

There is a support jar
jboss-common-jdbc-wrapper.jar
that should really be in the rar as well.

The reason it is not, is because we have multiple versions
of the jdbc rar.

* This is legacy of the JCA1.0 packaging where you could have
only one outbound MCF per rar
* The HA rars only exist separately because they were initially
experimental

If you do move them somewhere else, the integration apis
should be in a ".spi" package so it is clear which interfaces
are supported for user integration. But that isn't something
we can do in mid 4.0.x branch.

I prefer the package name ".plugin" for implementations of the spi.

[weston.price]

Right, 'refactoring' was probably a bad word :-) And I am in complete agreement with the frustration of doing an update and suddenly things are either completely removed, completely different etc, etc. Nothing worse than wasting an hour trying to figure exactly what happened.

I guess the comment was more directed towards improvements down the road. How we want to do things differently, how much of the JCA/MC stuff we can share across implementations. That sort of thing.

[weston.price]

Our JDBC RAR's should really be collapsed into one RAR as you point out. This would clean things up tremendously I think. We could also remove the DummyResourceAdapter stuff at the same time.