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

Database connection pooling
---------------------------

                 Key: JCR-1456
                 URL: https://issues.apache.org/jira/browse/JCR-1456
             Project: Jackrabbit
          Issue Type: Improvement
          Components: jackrabbit-core
            Reporter: Jukka Zitting
             Fix For: 1.5


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.

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

Esteban Franqueiro commented on JCR-1456:
-----------------------------------------

Is it possible to have, in those areas, the same problem reported in JCR-1388?
The idea is to use a pool package or to build our own?

> Database connection pooling
> ---------------------------
>
>                 Key: JCR-1456
>                 URL: https://issues.apache.org/jira/browse/JCR-1456
>             Project: Jackrabbit
>          Issue Type: Improvement
>          Components: jackrabbit-core
>            Reporter: Jukka Zitting
>             Fix For: 1.5
>
>
> 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.

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

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

> Is it possible to have, in those areas, the same problem reported in JCR-1388?

Connection pools would nicely solve most of our concurrent access issues, as we wouldn't be constrained to a single connection by default and wouldn't need workarounds like the one in JCR-1388.

> The idea is to use a pool package or to build our own?

I'd leverage a pooling DataSource whenever available (JNDI configuration), and use commons-dbcp to pool explicitly configured connections (JDBC Driver configuration).

> Database connection pooling
> ---------------------------
>
>                 Key: JCR-1456
>                 URL: https://issues.apache.org/jira/browse/JCR-1456
>             Project: Jackrabbit
>          Issue Type: Improvement
>          Components: jackrabbit-core
>            Reporter: Jukka Zitting
>             Fix For: 1.5
>
>
> 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.

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

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

Any update on this? Is there any estimation or is there a patch expected? :)

> Database connection pooling
> ---------------------------
>
>                 Key: JCR-1456
>                 URL: https://issues.apache.org/jira/browse/JCR-1456
>             Project: Jackrabbit
>          Issue Type: Improvement
>          Components: jackrabbit-core
>            Reporter: Jukka Zitting
>             Fix For: 1.5
>
>
> 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.

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

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

    Attachment: patch-1456-1.txt

Proof of concept patch.

* Abstracts database connection creation to allow pluggable pooling implementation.
* Not thoroughly tested and also not checked against checkstyle.
* So far it only covers BundleDbPersistenceManager and it's subclasses. All other components (db journal, db filesystem still use ConnectionRecoveryManager).

> Database connection pooling
> ---------------------------
>
>                 Key: JCR-1456
>                 URL: https://issues.apache.org/jira/browse/JCR-1456
>             Project: Jackrabbit
>          Issue Type: Improvement
>          Components: jackrabbit-core
>            Reporter: Jukka Zitting
>             Fix For: 1.5
>
>         Attachments: patch-1456-1.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.

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

Stefan Guggisberg commented on JCR-1456:
----------------------------------------

thanks for the patch, matej. that's very much appreciated.

i quickly browsed through the diff and noticed the following issue:

it seems like a connection is retrieved from the pool in every
PersistenceManager method. that's probably fine for
reading methods but that's not gonna work for writing methods
since they need to use the same connection (i.e. transaction).
all method calls within the store(ChangeLog) scope need
to use the same connection (with autoCommit set to false),
otherwise you'll end up with inconsistent/brokem repositories.

i am also a bit concerned about the impact of the proposed change
since it touches a lot of current code. the patch would have to be
thoroughly tested with all currently supported backends...

> Database connection pooling
> ---------------------------
>
>                 Key: JCR-1456
>                 URL: https://issues.apache.org/jira/browse/JCR-1456
>             Project: Jackrabbit
>          Issue Type: Improvement
>          Components: jackrabbit-core
>            Reporter: Jukka Zitting
>             Fix For: 1.5
>
>         Attachments: patch-1456-1.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.

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

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

Hi,

Thanks a lot for the comment. You're right, there might be a problem with different connections obtained. This could be handled by attaching active connection to current thread, so that the nested calls would always obtain the active connection. Anyway, I will look into it and post a new patch.

I agree that this is a substantial change and will require lot of testing. But i think at some point it will be necessary to bite the bullet and implement connection pooling, whether it will be based on my patch or not. The current situation is rather problematic, keeping opened connection per workspace doesn't scale well at all.

> Database connection pooling
> ---------------------------
>
>                 Key: JCR-1456
>                 URL: https://issues.apache.org/jira/browse/JCR-1456
>             Project: Jackrabbit
>          Issue Type: Improvement
>          Components: jackrabbit-core
>            Reporter: Jukka Zitting
>             Fix For: 1.5
>
>         Attachments: patch-1456-1.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.

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

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

    Attachment: patch-1456-2.txt

Added ConnectionPooling for DatabaseFileSystem and DbDataStore.
Database connections are thread bound if necessary.

> Database connection pooling
> ---------------------------
>
>                 Key: JCR-1456
>                 URL: https://issues.apache.org/jira/browse/JCR-1456
>             Project: Jackrabbit
>          Issue Type: Improvement
>          Components: jackrabbit-core
>            Reporter: Jukka Zitting
>             Fix For: 1.5
>
>         Attachments: patch-1456-1.txt, patch-1456-2.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.

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

knopp edited comment on JCR-1456 at 8/10/08 9:58 AM:
-----------------------------------------------------------

Added ConnectionPooling for DatabaseFileSystem and DbDataStore.
Database connections are thread bound if necessary.
Checked again checkstyle.
Not thoroughly tested.

      was (Author: knopp):
    Added ConnectionPooling for DatabaseFileSystem and DbDataStore.
Database connections are thread bound if necessary.
 

> Database connection pooling
> ---------------------------
>
>                 Key: JCR-1456
>                 URL: https://issues.apache.org/jira/browse/JCR-1456
>             Project: Jackrabbit
>          Issue Type: Improvement
>          Components: jackrabbit-core
>            Reporter: Jukka Zitting
>             Fix For: 1.5
>
>         Attachments: patch-1456-1.txt, patch-1456-2.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.

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

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

What about doing that for DbDataStore first? The patch would be much smaller.

> attaching active connection to current thread, so that the nested calls would always obtain the active connection

That sounds too complicated, too tricky, and too slow for me. For store(ChangeLog), why not simply pass the connection object to the nested calls?

> require lot of testing

Just to make sure: You mean automated tests, right? Manual tests is a maintenance problem.

> Database connection pooling
> ---------------------------
>
>                 Key: JCR-1456
>                 URL: https://issues.apache.org/jira/browse/JCR-1456
>             Project: Jackrabbit
>          Issue Type: Improvement
>          Components: jackrabbit-core
>            Reporter: Jukka Zitting
>             Fix For: 1.5
>
>         Attachments: patch-1456-1.txt, patch-1456-2.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.

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

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

> What about doing that for DbDataStore first? The patch would be much smaller.

Well, the file system is also instantiated per workspace so I believe connection pooling makes sense there. But it can be excluded from the patch, that shouldn't be a big issue.
Same goes for DataStore. But while you can live without Db file system, DbDateStore is more or less necessary for clustered environments and the lack of connection pooling can be a serious issue there.

> That sounds too complicated, too tricky, and too slow for me. For store(ChangeLog), why not simply pass the connection object to the nested calls?

Because the nested calls are invoked from BundleDbPersistenceManager which doesn't know about database connection.
 
It's really not complicated at all. There is one class (ThreadLocalConnectionProviderAdapter) that makes sure that getConnection() returns same connection for "nested" calls.
Also, could you please be more specific about what exactly sounds too slow about this?

> Just to make sure: You mean automated tests, right? Manual tests is a maintenance problem.
Well, all unit tests that work with "vanilla" jackrabbit also work after the patch is applied. However the patch hasn't been heavily tested in "real world" environment or with different database backends.

> Database connection pooling
> ---------------------------
>
>                 Key: JCR-1456
>                 URL: https://issues.apache.org/jira/browse/JCR-1456
>             Project: Jackrabbit
>          Issue Type: Improvement
>          Components: jackrabbit-core
>            Reporter: Jukka Zitting
>             Fix For: 1.5
>
>         Attachments: patch-1456-1.txt, patch-1456-2.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.

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

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

    Attachment: patch-1456-3.txt

Patch for current trunk (rev 672286)

> Database connection pooling
> ---------------------------
>
>                 Key: JCR-1456
>                 URL: https://issues.apache.org/jira/browse/JCR-1456
>             Project: Jackrabbit
>          Issue Type: Improvement
>          Components: jackrabbit-core
>            Reporter: Jukka Zitting
>             Fix For: 1.5
>
>         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.

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

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

Thanks for the patch!

> It's really not complicated at all. There is one class (ThreadLocalConnectionProviderAdapter...

This class does look complicated to me. To avoid ThreadLocal, what about:

BundleDbPersistenceManager {
  Connection currentConnection
  synchronized store(..) {
    try {
       currentConnection = ...
       super.store(..)
    } finally {
       currentConnection = null
    }
  }

> the patch hasn't been heavily tested in "real world"

I have already said, manual tests and real world tests are a maintenance problem. If there is no automated test, each change is big risk. Maintaining and improving the code is very hard in this case.

> Database connection pooling
> ---------------------------
>
>                 Key: JCR-1456
>                 URL: https://issues.apache.org/jira/browse/JCR-1456
>             Project: Jackrabbit
>          Issue Type: Improvement
>          Components: jackrabbit-core
>            Reporter: Jukka Zitting
>             Fix For: 1.5
>
>         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.

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

Dave Brosius commented on JCR-1456:
-----------------------------------

I like the idea of this patch, but i think the ConnectionProperties is too specific. It should just be


private String url;
private String driver;
private Properties connectionProperties.

for instance, i would like to add

properties.put("oracle.net.ssl_cipher_suites", "(SSL_DH_anon_WITH_3DES_EDE_CBC_SHA, SSL_DH_anon_WITH_RC4_128_MD5, SSL_DH_anon_WITH_DES_CBC_SHA)";

so that i can connect to the jackrabbit database over SSL.

And of course a similar implication for the repository.xml, to include arbitrary connection properties.

> Database connection pooling
> ---------------------------
>
>                 Key: JCR-1456
>                 URL: https://issues.apache.org/jira/browse/JCR-1456
>             Project: Jackrabbit
>          Issue Type: Improvement
>          Components: jackrabbit-core
>            Reporter: Jukka Zitting
>             Fix For: 1.5
>
>         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.

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

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

Hi Dave,

The connection properties you described are unrelated to "Database connection pooling", right?
If yes then I suggest to open another issue.

Regards,
Thomas

> Database connection pooling
> ---------------------------
>
>                 Key: JCR-1456
>                 URL: https://issues.apache.org/jira/browse/JCR-1456
>             Project: Jackrabbit
>          Issue Type: Improvement
>          Components: jackrabbit-core
>            Reporter: Jukka Zitting
>             Fix For: 1.5
>
>         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.