2 Replies Latest reply on Feb 4, 2011 10:24 PM by Jeff Yu

    Review of RDBMS session store

    Gary Brown Master

      Hi Jeff

       

      Great work - and very quick

       

      Only a few comments:

       

      1) DummySession could probably be moved to the src/test/java?

       

      2) Transaction Manager - although providing an option to pass one in is fine, I think it would also be good if the 'initialize' method could use a configuration property (if defined) as a JNDI lookup name - as when deployed to an app server, this is probably the most common way to obtain the transaction manager.

       

      3) Might also need a property to determine if an XA context is required? Would this cause a problem if (for example) an XA context was used against a non-XA datasource?

       

      4) Minor point - could you change "savara-session" to be "savara-monitor-session", just in case there are other 'session' concepts in Savara for non-monitoring?

       

      Thanks!

       

      Regards

      Gary

        • 1. Review of RDBMS session store
          Gary Brown Master

          Actually I think (1) can be ignored, as I guess once the create API call has been updated to pass in the serializable, then it shouldn't be required, even for testing - so I assume it will just be deleted?

          • 2. Review of RDBMS session store
            Jeff Yu Master

            Hi Gary,

            Gary Brown wrote:

             

            1) DummySession could probably be moved to the src/test/java?

             

            Now, this class has been moved to src/test/java. The reason that I am still using this class is because I didn't use the DefaultSession class as a Session Impl for the testing.

             

             

            2) Transaction Manager - although providing an option to pass one in is fine, I think it would also be good if the 'initialize' method could use a configuration property (if defined) as a JNDI lookup name - as when deployed to an app server, this is probably the most common way to obtain the transaction manager.

             

            Good point, Done, I've added a property for this, the property name is: transaction.manager.jndi.name

            Gary Brown wrote:

             

            3) Might also need a property to determine if an XA context is required? Would this cause a problem if (for example) an XA context was used against a non-XA datasource?

            I am not sure about this, will need to check.

             

            Gary Brown wrote:

             

            4) Minor point - could you change "savara-session" to be "savara-monitor-session", just in case there are other 'session' concepts in Savara for non-monitoring?

            Good point, renaming is Done.

             

            Regards

            Jeff