> We can always change the superclass.
Well, the superclass methods could pass a context object around that the subclass could use to store connection.
> Not if it's a member of an extra object that's instantiated per each top level method call.
Yes, but then the superclass needs to pass this object as method argument. Which is probably okay.
> For now I'm mostly worried about the patch being more complex than it needs to be. We can add config entries once the basic functionality is in place.
The problem here is that right now every component (persistence manager, fs, journal, ...) in jackrabbit is responsible for creating the database connection thus the connection properties are specified as configuration option for the component. But connection pooling essentially takes this responsibility from component. So how should the database properties be handled? Where should the connection pool be configured?
My previous patch solved this in way that allowed propagation of connection properties from components to connection pool and didn't require any changes to configuration files but that came at cost of readability and complexity. Current way of configuring database connections in jackrabbit assumes the connection is created by the component but this is really not compatible with connection pools.
The cleanest solution would be to add section in configuration to define data sources and then way to assign a data source to each component that needs database connection. This obviously requires changing the configuration scheme though.
If you have an idea how to configure connection pools and configure the components to use those pools while preserving current configuration scheme I'd love to know about it. The approach I tried worked but it resulted in code apparently too complicated to be committed.
There is one way I can imagine connection pooling working without change to configuration syntax. While parsing the configuration jackrabbit would create datasource (with default connection pool i.e. dbcp) for each distinct connection properties and then pass this datasource to the component to which the connection properties belong.
This would make the configuration parsing code bit more complicated but wouldn't require any change to configuration files.
> So we don't need to implement one. Note that the solution should work with existing Jackrabbit configurations that do not specify a connection pool. There's no need for DBCP when a JNDI DataSource is configured, but it makes things a lot easier for non-JNDI configurations.
I agree that jackrabbit should have connection pooling configured by default but that should be overridable in configuration file.
> What's SimplePoolingConnectionProvider for then?
That's just a simple connection pool i used for development. It was meant to be kind of "Default" connection provider and the connection pooling logic should be replaced with DBCP should people have agried with the approach i taked with the patch.