[jira] Created: (JCR-1456) Database connection pooling

60 messages Options
Embed this post
Permalink
1 2 3
JIRA jira@apache.org

[jira] Commented: (JCR-1456) Database connection pooling

Reply Threaded More More options
Print post
Permalink
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/JCR-1456?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12709516#action_12709516 ]

Jukka Zitting commented on JCR-1456:
------------------------------------

I think the main concerns raised above are:

* The entire store(ChangeLog) operation needs to happen atomically

* 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

I also have some extra concerns:

* 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.

* 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.

* 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:

    public void doSomething() throws SQLException {
        Connection connection = dataSource.getConnection();
        try {
            // do something with the connection
        } finally {
            connection.close();
        }
    }

The above are of course just individual opinions. Feel free to argue otherwise if you have a better solution.

PS. The Jackrabbit sandbox is nowadays open to all Apache committers, so if you may want to create a development branch of Jackrabbit trunk (or the 1.x branch) in the sandbox for this work. The changes here are so extensive that it may be easier for us to work incrementally through svn.

PPS. Alternatively, if you know Git, you may want to clone git://git.apache.org/jackrabbit.git and publish your changes for example on Github.



> 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.

JIRA jira@apache.org

[jira] Commented: (JCR-1456) Database connection pooling

Reply Threaded More More options
Print post
Permalink
In reply to this post by JIRA jira@apache.org

    [ 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.

JIRA jira@apache.org

[jira] Commented: (JCR-1456) Database connection pooling

Reply Threaded More More options
Print post
Permalink
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/JCR-1456?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12709572#action_12709572 ]

Jukka Zitting commented on JCR-1456:
------------------------------------

> The problem here is that BundleDbPersistenceManager superclass is connection
> agnostic so passing the database connection as argument doesn't seem to be an option.

We can always change the superclass.

> Storing that as member variable is, but it requires additional locking.

Not if it's a member of an extra object that's instantiated per each top level method call.

> As far as I can remember all introduced configuration entities were completely optional
> with sane default making the change pretty much transparent for user.

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.

> What's the point of hardwiring concrete connection pool?

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.

> In my previous patch I didn't attempt to do any connection pooling.

What's SimplePoolingConnectionProvider for then?

> The reason why I chose not to use DataSource is because DataSource has no means to
> pass connection properties (url, driver, etc).

That's what we'd use Commons DBCP for (in cases when a JNDI DataSource has not already been configured).

> The custom sandbox could work for me.

OK, good. I created such a development branch in https://svn.apache.org/repos/asf/jackrabbit/sandbox/JCR-1456.



> 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.

JIRA jira@apache.org

[jira] Commented: (JCR-1456) Database connection pooling

Reply Threaded More More options
Print post
Permalink
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/JCR-1456?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12709596#action_12709596 ]

Matej Knopp commented on JCR-1456:
----------------------------------

> 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.



> 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.

JIRA jira@apache.org

[jira] Updated: (JCR-1456) Database connection pooling

Reply Threaded More More options
Print post
Permalink
In reply to this post by JIRA jira@apache.org

     [ https://issues.apache.org/jira/browse/JCR-1456?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Jukka Zitting updated JCR-1456:
-------------------------------

    Attachment: dbcp.patch

> 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?

Right where we currently configure the single database connection per component.

> 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.

See the attached patch (dbcp.patch) for a simple change that makes all the connections we currently create to come from connection pools. This change obviously doesn't solve the main issue, but should illustrate how I envision us to handle existing database configurations.

> 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.

Exactly!


> 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: dbcp.patch, 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.

JIRA jira@apache.org

[jira] Updated: (JCR-1456) Database connection pooling

Reply Threaded More More options
Print post
Permalink
In reply to this post by JIRA jira@apache.org

     [ https://issues.apache.org/jira/browse/JCR-1456?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Jukka Zitting updated JCR-1456:
-------------------------------

    Attachment: dbcp.patch

Minor update (protect against a null driver class) to dbcp.patch.

> 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: dbcp.patch, dbcp.patch, 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.

JIRA jira@apache.org

[jira] Commented: (JCR-1456) Database connection pooling

Reply Threaded More More options
Print post
Permalink
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/JCR-1456?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12712054#action_12712054 ]

Jukka Zitting commented on JCR-1456:
------------------------------------

What do you think about my dbcp.patch? Should I commit it to the feature branch as a starting point?

> 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: dbcp.patch, dbcp.patch, 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.

JIRA jira@apache.org

[jira] Commented: (JCR-1456) Database connection pooling

Reply Threaded More More options
Print post
Permalink
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/JCR-1456?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12712071#action_12712071 ]

Matej Knopp commented on JCR-1456:
----------------------------------

I like it. I think it's a good way to start.

> 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: dbcp.patch, dbcp.patch, 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.

JIRA jira@apache.org

[jira] Commented: (JCR-1456) Database connection pooling

Reply Threaded More More options
Print post
Permalink
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/JCR-1456?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12713051#action_12713051 ]

Jukka Zitting commented on JCR-1456:
------------------------------------

OK, thanks. I applied the patch in revision 778741. As a next step I think we should change the ConnectionFactory to return the configured DataSource instead of a Connection object. This way we can push the DataSource reference all the way up to the PersistenceManager implementations and use it to get Connections only on demand.

> 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: dbcp.patch, dbcp.patch, 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.

JIRA jira@apache.org

[jira] Commented: (JCR-1456) Database connection pooling

Reply Threaded More More options
Print post
Permalink
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/JCR-1456?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12713099#action_12713099 ]

Matej Knopp commented on JCR-1456:
----------------------------------

There is one problem with your patch that I overlooked. You create new BasicDataSource every time getDriverDataSource() is called. I think there should only be one datasource instance per driverclass/url combo. Otherwise it just keeps creating pools.

I can fix this easily but it will take some time. Right now I'm in process of getting BundleDbPersistenceManager and subclasses used to borrowing connections instead of relying on one shared always being available.

> 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: dbcp.patch, dbcp.patch, 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.

JIRA jira@apache.org

[jira] Commented: (JCR-1456) Database connection pooling

Reply Threaded More More options
Print post
Permalink
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/JCR-1456?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12713171#action_12713171 ]

Jukka Zitting commented on JCR-1456:
------------------------------------

> You create new BasicDataSource every time getDriverDataSource() is called.

That's by design. As noted in my previous comment, I think we should replace ConnectionFactory.getConnection() with ConnectionFactory.getDataSource() and store a reference to the returned DataSource in the persistence manager.

> I think there should only be one datasource instance per driverclass/url combo.

Eventually yes. There the extra configuration parts that you proposed earlier will come in handy. However I think it's more straightforward if we start with one DataSource per persistence manager for now. Doing it this way we can keep the changes nicely localized within a single persistence manager. We can change the configuration mechanism or introduce some repository-local DataSource registry later on once the basic pooling functionality is there.

> 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: dbcp.patch, dbcp.patch, 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.

JIRA jira@apache.org

[jira] Updated: (JCR-1456) Database connection pooling

Reply Threaded More More options
Print post
Permalink
In reply to this post by JIRA jira@apache.org

     [ https://issues.apache.org/jira/browse/JCR-1456?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Matej Knopp updated JCR-1456:
-----------------------------

    Attachment: 777490.patch

Initial version of new patch. Probably lacks lot of polish. Would be nice if someone reviewed the patch and provided feedback.

> 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: 777490.patch, dbcp.patch, dbcp.patch, 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.

JIRA jira@apache.org

[jira] Updated: (JCR-1456) Database connection pooling

Reply Threaded More More options
Print post
Permalink
In reply to this post by JIRA jira@apache.org

     [ https://issues.apache.org/jira/browse/JCR-1456?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Matej Knopp updated JCR-1456:
-----------------------------

    Attachment:     (was: 777490.patch)

> 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: 777490.patch, dbcp.patch, dbcp.patch, 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.

JIRA jira@apache.org

[jira] Updated: (JCR-1456) Database connection pooling

Reply Threaded More More options
Print post
Permalink
In reply to this post by JIRA jira@apache.org

     [ https://issues.apache.org/jira/browse/JCR-1456?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Matej Knopp updated JCR-1456:
-----------------------------

    Attachment: 777490.patch

> 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: 777490.patch, dbcp.patch, dbcp.patch, 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.

JIRA jira@apache.org

[jira] Commented: (JCR-1456) Database connection pooling

Reply Threaded More More options
Print post
Permalink
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/JCR-1456?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12714429#action_12714429 ]

Jukka Zitting commented on JCR-1456:
------------------------------------

Reviewing a 2000+ line patch isn't too easy. Could you split it to smaller pieces? Also, feel free to commit the incremental changes directly to the branch in the sandbox. That way we can better label, comment and discuss each step separately instead of syncing up only on aggregate patches.

On the proposed changes: Good stuff, thanks! We're definitely seeing good progress here.

The main concern I have is about the Context concept you're introducing. I see where you're coming, but I think there's a better way to do this. The "context" of a method call is the object on which the method is called. How about, instead of passing the Context objects around, we actually moved the recipient methods *into* the Context class?

The Context class would then become something like a generic DatabaseOperation base class that encapsulates the database Connection being used for that operation. Subclasses like LoadBundleOperation, SaveChangesOperation or CheckSchemaOperation could extend this base class with specific functionality that we currently have inside the PersistenceManager (and other) classes. Database-specific extensions can be handled as yet another subclasses like OracleCheckSchemaOperation and the PersistenceManager classes would simply act as factories of these Operation instances instead of actually implementing the database functionality.

WDYT?

> 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: 777490.patch, dbcp.patch, dbcp.patch, 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.

JIRA jira@apache.org

[jira] Commented: (JCR-1456) Database connection pooling

Reply Threaded More More options
Print post
Permalink
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/JCR-1456?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12714472#action_12714472 ]

Matej Knopp commented on JCR-1456:
----------------------------------

Thanks for the feedback.

Unfortunately most of the patch is one big change - it modifies AbstractBundlePersistenceManager so it requires all it's subclasses to be adapted.

The context class was IMHO probably the easiest way to introduce connection pooling without requiring complete refactor/rewrite of persistence managers. Yet the patch is quite big.

I like the idea of operations. This would however be far bigger change that what I did. I thought the idea was to introduce connection pooling with minimal fuzz. Looks like I was wrong. I will look into this. I agree that if done properly this would be much cleaner solution than passing context object around.

> 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: 777490.patch, dbcp.patch, dbcp.patch, 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.

JIRA jira@apache.org

[jira] Commented: (JCR-1456) Database connection pooling

Reply Threaded More More options
Print post
Permalink
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/JCR-1456?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12714680#action_12714680 ]

Jukka Zitting commented on JCR-1456:
------------------------------------

My concerns about too many changes were mostly about having the modifications go too much beyond the o.a.j.core.persistence package (and other database-related packages). What we do inside those packages is open for discussion, and I'd personally prefer to reach a clean design that's built with connection pooling in mind than to patch the current design to work with pooled connections.

Anyway, I think the Context class is a good starting point, and we can continue by refactoring until the design is better.

Fair point about the one big change. If you like you can commit the full patch as-is and we can work from there in svn.

> 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: 777490.patch, dbcp.patch, dbcp.patch, 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.

JIRA jira@apache.org

[jira] Commented: (JCR-1456) Database connection pooling

Reply Threaded More More options
Print post
Permalink
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/JCR-1456?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12714682#action_12714682 ]

Thomas Mueller commented on JCR-1456:
-------------------------------------

To reduce the risk of problems, what about creating new classes instead of patching the existing classes? Like that you could concentrate on one database type first. It's just an idea...

> 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: 777490.patch, dbcp.patch, dbcp.patch, 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.

JIRA jira@apache.org

[jira] Commented: (JCR-1456) Database connection pooling

Reply Threaded More More options
Print post
Permalink
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/JCR-1456?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12714683#action_12714683 ]

Jukka Zitting commented on JCR-1456:
------------------------------------

We're already working on a branch, so I'm not that worried about changing things. Let's see what we come up with and then consider whether the result should be merged back as-is or perhaps copied into a new package in trunk.  We should probably also set up some extra integration tests that exercise all the database types that we can easily set up.

> 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: 777490.patch, dbcp.patch, dbcp.patch, 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.

JIRA jira@apache.org

[jira] Commented: (JCR-1456) Database connection pooling

Reply Threaded More More options
Print post
Permalink
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/JCR-1456?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12728725#action_12728725 ]

Jukka Zitting commented on JCR-1456:
------------------------------------

I updated the sandbox branch to match the trunk and applied the latest patch.

BTW, the branch now only compiles on Java 5 as the DataSource wrapper class doesn't work with Java 6.

> 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: 777490.patch, dbcp.patch, dbcp.patch, 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.

1 2 3