Uploaded image for project: 'OFBiz'
  1. OFBiz
  2. OFBIZ-8179

Enhance DBCPConnectionFactory to implement a TransactionFactory and remove the geronimo component

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: Trunk
    • Fix Version/s: 16.11.01
    • Component/s: framework
    • Labels:
      None

      Description

      Made the DBCPConnectionFactory an implementation of TransactionFactory by moving the code from the GeronimoTransactionFactory.

      With this change the geronimo component is no more required.

      Without any functional change, this design simplifies greatly the calls and component dependencies because it removes a cyclic dependency between entity and geronimo components (see diagram attached that explain the architecture before and after this change).

      1. OFBIZ-8179.patch
        8 kB
        Jacopo Cappellato
      2. TransactionConnectionFactory-before-after.png
        25 kB
        Jacopo Cappellato

        Activity

        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Thanks Scott and Jacopo, this is the better compromise indeed.

        Show
        jacques.le.roux Jacques Le Roux added a comment - Thanks Scott and Jacopo, this is the better compromise indeed.
        Hide
        jacopoc Jacopo Cappellato added a comment -

        Thanks for all your feedback and reviews.
        I ended up committing a different version of my patch, in order to address Scott's comment: see rev. 1760376
        With this fix I have removed the cyclic dependency between the two components; I didn't simplify the chain of calls required to get a connection from GeronimoTransactionFactory+DBCPConnectionFactory in order to maintain the flexibility to replace one of the two factories. We could revisit this discussion if/when we will have a measure that tells that direct calls improve the performance.

        Show
        jacopoc Jacopo Cappellato added a comment - Thanks for all your feedback and reviews. I ended up committing a different version of my patch, in order to address Scott's comment: see rev. 1760376 With this fix I have removed the cyclic dependency between the two components; I didn't simplify the chain of calls required to get a connection from GeronimoTransactionFactory+DBCPConnectionFactory in order to maintain the flexibility to replace one of the two factories. We could revisit this discussion if/when we will have a measure that tells that direct calls improve the performance.
        Hide
        lektran Scott Gray added a comment -

        Hi Jacopo,

        Doesn't this change force users to use geronimo as their transaction manager when using dbcp2? Alternatively, if I want to use a different connection pooling implementation, I would now need to reimplement the TransactionFactory as well?

        Could we not just move GeronimoTransactionFactory.java into the entity component and achieve roughly the same result while still maintaining separation between transactions and connections?

        Show
        lektran Scott Gray added a comment - Hi Jacopo, Doesn't this change force users to use geronimo as their transaction manager when using dbcp2? Alternatively, if I want to use a different connection pooling implementation, I would now need to reimplement the TransactionFactory as well? Could we not just move GeronimoTransactionFactory.java into the entity component and achieve roughly the same result while still maintaining separation between transactions and connections?
        Hide
        jacopoc Jacopo Cappellato added a comment -

        Taher Alkhateeb I think you are referring to:

             <!-- the connection factory class to use, one is needed for obtaining connections/pools for defined resources -->
        +    <!--
             <connection-factory class="org.apache.ofbiz.entity.connection.DBCPConnectionFactory"/>
        +    -->
        

        I have commented it out because it is not required setting and it is not specifically required by the new transaction-factory.
        I agree with you that it is better to remove it rather than leaving it commented out.
        Thanks

        Show
        jacopoc Jacopo Cappellato added a comment - Taher Alkhateeb I think you are referring to: <!-- the connection factory class to use, one is needed for obtaining connections/pools for defined resources --> + <!-- <connection-factory class= "org.apache.ofbiz.entity.connection.DBCPConnectionFactory" /> + --> I have commented it out because it is not required setting and it is not specifically required by the new transaction-factory. I agree with you that it is better to remove it rather than leaving it commented out. Thanks
        Hide
        taher Taher Alkhateeb added a comment -

        Hi Jacopo, Great work! Why comment out the transaction-factory and not delete instead? Or do you want to do that in one shot while deleting the geronimo component and libs from build.gradle as well (which would be great I might add)

        Show
        taher Taher Alkhateeb added a comment - Hi Jacopo, Great work! Why comment out the transaction-factory and not delete instead? Or do you want to do that in one shot while deleting the geronimo component and libs from build.gradle as well (which would be great I might add)
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        +1 after review (voted)

        Show
        jacques.le.roux Jacques Le Roux added a comment - +1 after review (voted)

          People

          • Assignee:
            jacopoc Jacopo Cappellato
            Reporter:
            jacopoc Jacopo Cappellato
          • Votes:
            1 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development