9 Replies Latest reply on Feb 11, 2005 8:49 AM by Andrei Tarkhov

    Impossible to subclass IdleRemover and InternalManagedConnec

    Andrei Tarkhov Newbie

      IdleRemover and InternalManagedConnectionPool tightly coupled and impossible to subclass.

      During last period we are faced with the necessity to extend JBoss JCA
      and add several parameters to provide better flexibility and
      manageability (for example "aged timeout").

      Due to private constructor and coupling with InternalManagedConnectionPool is impossible to subclass both classes and overwrite only needed methods.

      Simple refactoring may solve this problem.

        • 1. Re: Impossible to subclass IdleRemover and InternalManagedCo
          Adrian Brock Master

          IdleRemover is a static instance that runs a single remover thread for all pools.

          To override the implementation would require an IdleRemoveFactory
          and a global configuration somewhere in jbossjca-service.xml

          Why do you need to change it? All it does it callback to the registered pool
          on a timeout defined by the pool.

          • 2. Re: Impossible to subclass IdleRemover and InternalManagedCo
            Adrian Brock Master

            I would be prepared to accept a modification that allows the internal pool
            classname to be specified on the ManagedConnectionPool.

            In fact there are three configurations that are hardwired and need exposing
            in -ds.xml for more advanced use cases:

            <ManagedConnectionPool/>Defines the ManagedConnectionPool implementation (default JBossManagedConnectionPool)
            <InternalConnectionPool/>Defines the internal pool implementation (default InternalManagedConnectionPool)
            <ConnectionManager/>Allows the connection manager implementation to be overridden
            


            In fact, allowing the ManagedConnectionPool to be overriden is probably too
            broad brush when what is really required is the ability to specify the PoolingStrategy
            class rather than using the short hand ByContainer, ByApplication, etc.

            Care to prototype these modifications? Some of them involve changes
            to the stylesheet rather than the code.

            • 3. Re: Impossible to subclass IdleRemover and InternalManagedCo
              Adrian Brock Master

              On your immediate problem, it seems to that what is really required is the ability
              define a plugin for the remover logic in InternalManagedConnectionPool.

              Something like:

              <remover-policy>com.acme.RemoverPolicy</remover-policy>
              
              - if (cl.isTimedOut(timeout))
              + if (removerPolicy.isTimedOut(cl, poolParams))
              


              This will require a more generic notion of PoolParams if you want to be
              able to add new parameters for your logic to use. e.g. in -ds.xml

              <pool-param name="myparam" type="java.lang.Integer">22</pool-param>

              • 4. Re: Impossible to subclass IdleRemover and InternalManagedCo
                Adrian Brock Master

                In anycase. What feature are you adding?
                If it is generically useful, this could be added to the base implementation
                rather than needing all this factory/plugins complication.

                • 5. Re: Impossible to subclass IdleRemover and InternalManagedCo
                  Andrei Tarkhov Newbie

                   

                  "adrian@jboss.org" wrote:
                  In anycase. What feature are you adding?
                  If it is generically useful, this could be added to the base implementation
                  rather than needing all this factory/plugins complication.


                  I belive that it is generically useful and would be appreciated by community.
                  Basicly, added the next parameters to handle connection pool:
                  1. Aged timeout - the total amount of time which connection can be alive.
                  After this period connections are removed from the pool. We are checking this period in two cases: when connection is returned to pool and by some external thread (IdleRemover). This timeout is different then IdleTimeout.
                  2. The conn-usage-limit - the limit of connection usage
                  (total number of connection life circles "allocate-release").
                  3. The reap-timeout - the time interval in seconds when connection must be checked by some external service.

                  "adrian@jboss.org" wrote:
                  IdleRemover is a static instance that runs a single remover thread for all pools.

                  To override the implementation would require an IdleRemoveFactory
                  and a global configuration somewhere in jbossjca-service.xml

                  Why do you need to change it? All it does it callback to the registered pool
                  on a timeout defined by the pool.


                  There are several reasons:
                  1. IdleRemover defines waiting period for the thread. If we process additional timeouts by the same thread (like "aged timeout") currrent definition for the waiting period like t=min(IdleTimeout/2) is not suitable (for example aged timeout may be less than IdleTimeout). In this case is better to use additional independent parameter - "reap timeout" (how it's done for WebSphere). In this case waiting period calculated like min(reapTimeout).
                  2. Additional parameters can be encapsulated in PoolParams (or subclass of PP), but in case of subclassing of PoolParams we have to make correspondent casting in IdleRemover or overload a constructor.

                  "adrian@jboss.org" wrote:
                  I would be prepared to accept a modification that allows the internal pool
                  classname to be specified on the ManagedConnectionPool.

                  In fact there are three configurations that are hardwired and need exposing
                  in -ds.xml for more advanced use cases:

                  Code:

                  <ManagedConnectionPool/>Defines the ManagedConnectionPool implementation (default JBossManagedConnectionPool)
                  <InternalConnectionPool/>Defines the internal pool implementation (default InternalManagedConnectionPool)
                  <ConnectionManager/>Allows the connection manager implementation to be overridden

                  In fact, allowing the ManagedConnectionPool to be overriden is probably too
                  broad brush when what is really required is the ability to specify the PoolingStrategy
                  class rather than using the short hand ByContainer, ByApplication, etc.

                  Care to prototype these modifications? Some of them involve changes
                  to the stylesheet rather than the code.


                  It would be perfect.
                  We were very close to idea to define InternalManagedConnection by name.

                  "adrian@jboss.org" wrote:
                  On your immediate problem, it seems to that what is really required is the ability
                  define a plugin for the remover logic in InternalManagedConnectionPool.


                  You are right. It would be additional room for extension.
                  Only one complication is that "aged timeout" processing shoud be done in two cases:
                  1. By external thread for connections in the pool (IdleRemover). Required
                  callback method modification.
                  2. While returning connections to the pool (returnConnection(ConnectionListener cl, boolean kill))





                  • 6. Re: Impossible to subclass IdleRemover and InternalManagedCo
                    Adrian Brock Master

                    I still don't see why you need to change the IdleRemover.

                    If the aged timeout is less than the idle timeout, you pass that as the interval.
                    It just means it is going to make more frequent calls to removedTimeOut.

                    There is no loss here since it just checks one connection (the oldest unused)
                    to see whether it timed out, or you have more complicated processing.
                    It may check more with your policy.

                    I'd suggest you identify a TimeoutPolicy that can be plugged into the InternalManagedConnectionPool.PoolParams
                    (with a default policy for when it is not specified - that is the current default).

                    This would encapsulate:
                    1) The interval used when registerting with the idle remover
                    2) What happens at return connection (i.e. should it kill it)
                    3) What happens on the callback from the idle remover
                    4) What data (javabean object) needs storing in the ConnectionListener to satisfy the policy
                    Any others?

                    Something like:

                    public interface ConnectionTimeoutPolicy
                    {
                     void setPoolParams(PoolParams pp); // invoked at pool startup after construction
                     long getRemoverInterval(); // invoked at pool startup before registering with idle remover
                     boolean isTimedOut(ConnectionListener cl); // invoked on each return
                     ArrayList /* connections to destroy */ removeTimedOut(Arraylist connectionListeners); // invoked from the idle remover callback
                     Object initTimeOutParams(); // inserted into each ConnectionListener
                    }
                    


                    The only issue is how you make it easily configurable through the stylesheet
                    and maintain backwards compatibily for older configurations.
                    i.e. we need somebody using the current configuration to be unaware
                    of this change.
                    How is your xslt :-)

                    This gives you what you want and it is possible for somebody else to override
                    the behaviour in the future.

                    • 7. Re: Impossible to subclass IdleRemover and InternalManagedCo
                      Andrei Tarkhov Newbie

                       

                      "adrian@jboss.org" wrote:
                      I still don't see why you need to change the IdleRemover.

                      If the aged timeout is less than the idle timeout, you pass that as the interval.
                      It just means it is going to make more frequent calls to removedTimeOut.


                      Right. But IdleRemover makes additional calculation:
                      private void internalRegisterPool(InternalManagedConnectionPool mcp, long interval)
                      {
                      synchronized (pools)
                      {
                      pools.add(mcp);
                      if (interval > 1 && interval/2 < this.interval)
                      {
                      this.interval = interval/2;
                      }
                      ...
                      }

                      I would prefer to change it a little by:
                      private void internalRegisterPool(InternalManagedConnectionPool mcp, long newInterval)
                      {
                      synchronized (pools)
                      {
                      pools.add(mcp);
                      if (interval > 1 && newInterval < this.interval)
                      {
                      this.interval = newInterval;
                      }
                      ...
                      }

                      Meaning of "interval" would be more straightforward here.


                      "adrian@jboss.org" wrote:

                      There is no loss here since it just checks one connection (the oldest unused)
                      to see whether it timed out, or you have more complicated processing.
                      It may check more with your policy.

                      I'd suggest you identify a TimeoutPolicy that can be plugged into the InternalManagedConnectionPool.PoolParams
                      (with a default policy for when it is not specified - that is the current default).

                      Ok. For Idle connections enough to check elements from FIFO by ".get(0)", but for "aged connections" it is more complicated but may be encapsulated in TimeoutPolicy.

                      "adrian@jboss.org" wrote:

                      This would encapsulate:
                      1) The interval used when registerting with the idle remover
                      2) What happens at return connection (i.e. should it kill it)
                      3) What happens on the callback from the idle remover
                      4) What data (javabean object) needs storing in the ConnectionListener to satisfy the policy
                      Any others?

                      Something like:
                      public interface ConnectionTimeoutPolicy
                      {
                       void setPoolParams(PoolParams pp); // invoked at pool startup after construction
                       long getRemoverInterval(); // invoked at pool startup before registering with idle remover
                       boolean isTimedOut(ConnectionListener cl); // invoked on each return
                       ArrayList /* connections to destroy */ removeTimedOut(Arraylist connectionListeners); // invoked from the idle remover callback
                       Object initTimeOutParams(); // inserted into each ConnectionListener
                      }
                      




                      Some additions:
                      1. I would remove setPoolParams(PoolParams pp) from interface and add factory method getConnectionPolicy(String classname, PoolParams pp);
                      in this case default implementation would be like:
                      public class DefaultConnectionTimeoutPolicy implements ConnectionTimeoutPolicy
                      {
                      protected DefaultConnectionTimeoutPolicy(){}
                      public ConnectionTimeoutPolicy getConnectionPolicy(String classname, PoolParams){
                      ConnectionTimeoutPolicy ctp = null;
                      if(classname != null){
                      try{
                      ctp = class.byName(classname);
                      }
                      catch(){}
                      }
                      else if(ctp == null){
                      ctp = new Default...();
                      }
                      ctp.setPoolParams();
                      return ctp;
                      }

                      protected void setPoolParams(PoolParams pp){}
                      }

                      2. I would add long getInterval(); method to interface.
                      For default implementation it would be like:
                      public getInterval(){
                      return PoolParams.idleTimeout/2;
                      }


                      "adrian@jboss.org" wrote:

                      The only issue is how you make it easily configurable through the stylesheet
                      and maintain backwards compatibily for older configurations.
                      i.e. we need somebody using the current configuration to be unaware
                      of this change.
                      How is your xslt :-)

                      This gives you what you want and it is possible for somebody else to override
                      the behaviour in the future.


                      XSLT not a problem, but several additional things has been done:
                      1. InternalManagedConnectionPool has to be refactored (constructor, returnConnection(), removeTimeout() and PoolParams inner class).
                      I would added protected accessors to some private fields to simlify future
                      subclassing.
                      2. I would separate JBossManagedConnectionPool class and inner BasePool tree.
                      3. Some new initialization parameters may be addded to subclass of JBossManagedConnectionPool if we add accessor to "poolParams" field.
                      4. BaseConnectionManager.BaseConnectionEventListener
                      should contain reference to Object which encapsulate timeout parameters like:
                      {
                      private long lastUse;
                      private long firstUse; // Added to support aged timeout
                      private long timesUsed = 0; // Added to support usageLimit
                      }
                      4. ConnectionFactoryTemplate.xsl changes in this case would be trivial like:
                      <!-- template to generate the pool mbean -->
                      <xsl:template name="pool">
                      <xsl:param name="mcf-template">generic-mcf</xsl:param>
                      <depends optional-attribute-name="ManagedConnectionPool">

                      <!--embedded mbean-->
                      <mbean code="org.jboss.resource.connectionmanager.ext.NewManagedConnectionPool" name="jboss.jca:service=ManagedConnectionPool,name={jndi-name}" display-name="Connection Pool for DataSource {jndi-name}">
                      ....
                      <!-- new parameters -->
                      <xsl:choose>
                      <xsl:when test="timeout-policy-class-name">
                      <xsl:value-of select="timeout-policy-class-name"/>
                      </xsl:when>
                      <xsl:otherwise>
                      null
                      </xsl:otherwise>
                      </xsl:choose>
                      <xsl:choose>
                      <xsl:when test="aged-timeout">
                      <xsl:value-of select="aged-timeout"/>
                      </xsl:when>
                      <xsl:otherwise>
                      0
                      </xsl:otherwise>
                      </xsl:choose>
                      ....
                      Correspondent changes has to be added to .dtd

                      If user defines this properties in ds.xml -> will be used some new ConnectionTimeoutPolicy,
                      if not - DefaultConnectionTimeoutPolicy (functionality will be transparent and exactly the same as before.)

                      Agree? How we can proceed with this solution?
                      I am ready to prepare prototype for 1-2 days.



                      • 8. Re: Impossible to subclass IdleRemover and InternalManagedCo
                        Adrian Brock Master

                        Do you have a sourceforge id?

                        If you do I will get you cvs access where you can prototype your ideas in jboss-head
                        (the development branch which will become JBoss5).

                        Make sure you read the developers guide on coding standards (they are not the
                        same as Sun's guidelines), cvs branches, etc.

                        Also note, that the way the jca container works is going to change in JBoss5:
                        http://jira.jboss.com/jira/browse/JBAS-1435

                        But your work will be useful if it can be backported to jboss4 without breaking
                        current users configurations.

                        • 9. Re: Impossible to subclass IdleRemover and InternalManagedCo
                          Andrei Tarkhov Newbie

                           

                          "adrian@jboss.org" wrote:
                          Do you have a sourceforge id?

                          If you do I will get you cvs access where you can prototype your ideas in jboss-head
                          (the development branch which will become JBoss5).


                          My id is 'andreitar'.

                          "adrian@jboss.org" wrote:
                          Make sure you read the developers guide on coding standards (they are not the
                          same as Sun's guidelines), cvs branches, etc.


                          Ok.