[
https://issues.apache.org/jira/browse/JCR-1456?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12709547#action_12709547
]
Matej Knopp commented on JCR-1456:
----------------------------------
>Using ThreadLocal for the connection seems unnecessarily complex (from the
>perspective of someone new trying to understand the code), it's better to pass
>the connection around as a method argument or encapsulate it as a member
>variable of an object that performs the database operations
The problem here is that BundleDbPersistenceManager superclass is connection
agnostic so passing the database connection as argument doesn't seem to be an
option. Storing that as member variable is, but it requires additional locking.
To reduce unnecessary locking I decided to use thread locals. Since it's
perceived to be too complicated there won't be any in next patch.
> Could you implement this without introducing new configuration entries? We
> may consider adding that later, but it would be clearer if we first
> implemented connection pooling with the access configuration that we
> currently have.
As far as I can remember all introduced configuration entities were completely
optional with sane default making the change pretty much transparent for user.
> We should leverage something like Commons DBCP instead of implementing our
> own connection pooling logic. Commons DBCP is much better than anything that
> we could come up with.
What's the point of hardwiring concrete connection pool? In my previous patch I
didn't attempt to do any connection pooling. I just had a connection source
which could be easily implemented to get the connection from *any* connection
pool or JNDI. If you insist though on using commons dbcp I can live with that
though.
> Related to the above, we should use the standard DataSource interface
> interface instead of a custom ConnectionManager class. This would nicely
> abstract away all the pooling logic and make the code much more familiar to
> people who already know JDBC. All top-level methods would look like something
> like this
The connection manager in patch didn't create database connections. It
delegated that to a connection provider. The purpose of ConnectionManager were
some convenience methods and prepared statements caching. The connection
provider was simple DataSource like interface. The reason why I chose not to
use DataSource is because DataSource has no means to pass connection properties
(url, driver, etc). That normally isn't a problem, in jackrabbit however it is,
because the database properties are specified on components level (persistence
manager, file system, journal, etc).
If PersistenceManager is supposed to have connection properties (which you seem
to insist on in order not to change configuration) then it needs a way to pass
these information to code that manages database connections. And DataSource
doesn't provide any means for it.
The custom sandbox could work for me.
Thanks for your input, it's very appreciated.
> Database connection pooling
> ---------------------------
>
> Key: JCR-1456
> URL: https://issues.apache.org/jira/browse/JCR-1456
> Project: Jackrabbit Content Repository
> Issue Type: Improvement
> Components: jackrabbit-core
> Reporter: Jukka Zitting
> Attachments: patch-1456-1.txt, patch-1456-2.txt, patch-1456-3.txt
>
>
> Jackrabbit should use database connection pools instead of a single
> connection per persistence manager, cluster journal, or database data store.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
