Review of RDBMS session store

[objectiser]

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

[objectiser]

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?

[jeff.yuchang]

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