JBAS-1241 Background Pool Validation

[weston.price]

For http://jira.jboss.com/jira/browse/JBAS-1241:

This issue popped up the priority stack recently. In looking at this, I propose the following:

Modify BaseWrapperManagedConnectionFactory to implement the ValdatingConnectionFactory interface from the 1.5 spec. The method exposed is:

Set getInvalidConnections(Set managedConnecdtions);

Add validateConnections() method to InternalManagedConnectionPool that is called by an 'external' validating background thread (more on this in a bit).

The BaseWrapperMCF should validate the set of MC's calling checkValid() much in the same manner as is done in the match method returning the invalid MC's that are then destroyed.

The real issue I see with this is efficiency. The interface takes a set of MC's, returning the invalid set. Taken at face value this requires 2 passes through the list of connection listeners. One to get the MC's and the next to find the listeners to destroy. Since this has to be synchornized, this could get to be a real bottle neck, background thread or not.

As an optimization, a JBossValidatingCF interface could be added that took a let of ConnectionListeners and returned the ones that needed to be removed directly. The invalid connections could then be destroyed immiedately.

We could either add an element to the XML to tell the pool to look for this, set a flag in the constructor, do it dynamically etc, etc.

As far as a background thread, I don't like using the IdleRemover to do this. I think it should be a seperated thread BackgroundValidator or something like that. Right now the IdleRemove waits on the idle timeout interval. At the very least this would have to be modified, or add another remover to do the validation. If we are going to this, let's just clarify it as another class.

[adrian.brock]

The way I see this change is:

1) Add new parameters to the connection pool for the background
checking frequency
2) Warn if frequency configured and it doesn't implement the JCA 1.5
optional interface
3) Change InternalManagedConnectionPool to have a validateConnections
like you said
4) In validateConnections() - pseudo code (be careful with error handling)

if (permits.attempt()) // Get a single permit, don't worry about it if all the permits are in use
{
 // Whether we destroyed a connection
 boolean destroyed = false;
 try
 {
 while (true)
 {
 ConnectionListener cl = null;
 // Temporarily make the first connection that needs checking
 // unavailable to applications
 synchronized (cls)
 {
 cl = removeFirstClNotCheckedWithinFrequency();
 }
 // We've checked them all
 if (cl == null)
 break;
 // Check the single connection
 try
 {
 Set set = Collections.singleton(cl.getManagedConnection());
 set = ((ValidatingConnectionFactory) mcf).getInvalidConnections(set);

 // This connection failed
 if (set != null && set.size() > 0)
 {
 // If the checking already invoked connectionErrorOccured
 // don't bother warning
 if (cl.state != DESTROYED)
 log.warn("....");

 // Destroy the failed connection
 destroy(cl);
 destroyed == true;
 }
 }
 finally
 {
 // Its important that the MRU list is maintained
 // for the idle removal alogorithm to work properly
 synchronized (cls)
 {
 putItBackAccordingMRU(cl);
 }
 }
 }
 }
 finally
 {
 // Release our permit
 permits.release();

 // We destroyed something, check the minimum.
 if (destroyed && shutdown.get() == false && poolParams.minSize > 0)
 PoolFiller.fillPool(this);
 }
}

[adrian.brock]

The JDBC resource adapter should change to implement the JCA1.5 api.
It needs an extra parameter to say don't do the validation check
for every matchManagedConnections().

This can be automatically set to false when the frequency is specified,
if the user doesn't specify it explicitly.

I don't understand why you need a JBoss specific api?

[adrian.brock]

As far as a background thread, I don't like using the IdleRemover to do this. I think it should be a seperated thread BackgroundValidator or something like that. Right now the IdleRemove waits on the idle timeout interval. At the very least this would have to be modified, or add another remover to do the validation. If we are going to this, let's just clarify it as another class.

This really needs tidying up. There are already two threads
1) IdleRemover
2) PoolFiller

There should be a single "scheduler" for all these background tasks.
When running inside JBossAS it should use whatever it defines as the
background task scheduler (currently this doesn't exist but IIRC there
is a feature request somewhere).
But this can be left to a future task (as long as it gets done :-)

[adrian.brock]

The reason I would implement it the way I sketched out, is because it doesn't
lock out the entire pool from the application threads while you are
checking a single connection.

Another possible improvement would be to remove the automatic
valid connection checking from the resource adapter's
matchManagedConnections() and instead invoke the optional
interface from the pool based on connection manager configuration.
IMHO this is a much cleaner way to do it. ;-)

[weston.price]

Weird...I think I am actually beginning to think like you...

The JBoss specific API was largely due to offering an optimized version of the 'checking' the new explanation removes this need.

[adrian.brock]

Weird...I think I am actually beginning to think like you...

Oh dear! :-)

[weston.price]

Question on branch/version:

The priority of this popped up the stack due to some support stuff. Should we put this in as a patch for a specific customer (current version 4.0.2) or should it go into a more current release and then we backport etc, etc.

The only reason I ask is because I am getting asked about it.

[adrian.brock]

You do it in the main branches first then backport to a patch branch.
1) Otherwise, it will get lost.
2) You need others to be using/testing this feature which means
it should be on the main branch.

[weston.price]

The pseudo code was about what I had, however on the removeFirstCheckedNotWithinFrequency() etc, etc. I am assuming we have to maintain some sort of book keeping on which CL's have been checked and then reset the state after the validation completes.

Correct?

[adrian.brock]

I'd just add a "last validated time" to the ConnectionListener class.

[weston.price]

Weird...I just did that...oh man, this is creepy....

[weston.price]

On the thread/scheduling implementation, here is what I think should happen; again, this can be retrofitted later, but this does, as you pointed out need cleanup.

Currently each thread Idle,Filler merely invokes a method on the Internal Pool. This is fine, but should probably be refactored into a single JCARunnable type process that can be independently scheduled. For the Idle and Validation threads we would just need to specify the interval, and the actual method to call (new interface to get this method), PoolFiller could be fit into this model as well.

[adrian.brock]

This should really be on a separate thread and discussed
as part of generic feature for all projects.
Probably using a java.util.concurrent api?

[weston.price]

This is where it would have been real, real handy to actually have a WorkManagement spec that wasn't bogged down with legality.