TRUNK-6136 - Add static method to retrieve the configured OpenMRS Dat…

[mseaton]

…aSource from Hibernate

Note: This PR is against 2.5.x branch

[coveralls]

Coverage increased (+0.1%) to 63.642% when pulling 2a65360 on TRUNK-6136 into ccaa39b on 2.5.x.

[dkayiwa]

@mseaton how do you plan to access this from your module? Is it via Context.getConnection()?

[mseaton]

@dkayiwa this is a static method, so I just plan to access it directly

[dkayiwa]

I do not see any problem resulting from this simple change. Though it looks ugly to provide direct public access to the data access layer via HibernateContextDAO.getConnection() 😊

[mseaton]

I do not see any problem resulting from this simple change. Though it looks ugly to provide direct public access to the data access layer via HibernateContextDAO.getConnection() blush

I agree @dkayiwa . We could try doing something like exposing the underlying datasource via JNDI, which log4j seems to be able to handle as an alternative to a static method for retrieving a connection, but that likely has it's own drawbacks and is not something I've done much before, so am less comfortable making that change. The main reason I felt this was OK is because a static public utility method already exists for getting a connection to the same database (DatabaseUpdater.getConnection()), it just creates a fresh connection every time instead of leveraging a connection pool, so this isn't really introducing any new access to the data layer that doesn't already exist.

Would it be better to move this static method off of HibernateContextDAO and into a utility class instead, to isolate the business of getting a connection from the implementation of using the HibernateContextDAO to do so?

I could certainly add the static DataSource variable and static "getConnection()" method to someplace like the existing "DatabaseUtil" class... Thoughts @dkayiwa / @ibacher ?

[ibacher]

Would it be better to move this static method off of HibernateContextDAO and into a utility class instead, to isolate the business of getting a connection from the implementation of using the HibernateContextDAO to do so?

Or just to move the user-accessible version of it into Context directly... I think there's an idea to minimize the number of OpenMRS classes module code is directly relying on.

We could try doing something like exposing the underlying datasource via JNDI

I don't know that JNDI adds a whole ton of value here... It's nice for the case where you want to configure a data source in the container (e.g. Tomcat) and use it in the application or share a connection pool across applications, but ultimately, since we pull the connection string and credentials from the runtime properties, something like log4J would need to be able to update itself between the connection not being available (because OpenMRS is starting up, but the logger is already working) and the connection being available...

[mseaton]

Or just to move the user-accessible version of it into Context directly... I think there's an idea to minimize the number of OpenMRS classes module code is directly relying on

Yes, I could do this. It would involve adding a new public instance method to HibernateContextDao and ContextDao to get either the Connection or the underlying DataSource, and then adding a public static method on Context to use the contextDao to retrieve this. Would this be better?

something like log4J would need to be able to update itself between the connection not being available (because OpenMRS is starting up, but the logger is already working) and the connection being available...

This will be an issue regardless. Having a static getConnection method that relies on the Context means that the Context needs to be started for this to be accessible. The alternative would be to roll a new, totally separate connection pool from the one used by Hibernate. I also am curious if this is more correct for other reasons (eg. will logging with the same connection pool as used for domain persistence be disruptive).

Thoughts?

[dkayiwa]

@mseaton do you happen to know why this change works only in core but failed when you put it in your module?

Second question:
The JDBC Appender...must be backed by a connection pool. Otherwise, logging performance will suffer greatly.
Do you happen to know if this applies for all cases including yours? In other wards, would the performance difference be such a significant one to disrupt your use case?

[mseaton]

do you happen to know why this change works only in core but failed when you put it in your module?

@dkayiwa I haven't fully debugged, but best I can gather is that the first time a class tries to get an instance of a Logger, log4j will initialize, and module classes are not loaded or available to the classloader yet at this time. Perhaps a deeper dive of the logging configuration is needed to ensure it is working as expected.

[mseaton]

The JDBC Appender...must be backed by a connection pool. Otherwise, logging performance will suffer greatly.
Do you happen to know if this applies for all cases including yours? In other wards, would the performance difference be such a significant one to disrupt your use case?

I don't know

[mseaton]

Or just to move the user-accessible version of it into Context directly... I think there's an idea to minimize the number of OpenMRS classes module code is directly relying on

@ibacher / @dkayiwa - I've moved the static method onto Context as suggested. Let me know what you think of this approach. Notably, I also removed the cached reference to the DataSource, so this will retrieve the DataSource from the sessionFactory using SessionFactoryUtils each time "getConnection()" is invoked. I hope this will be cheap, but will see with testing.

My JDBC appender, configured like this, is working for me in a dev SDK instance:
<ConnectionFactory class="org.openmrs.api.context.Context" method="getDatabaseConnection" />

[dkayiwa]

No big deal though, i would just do SessionFactoryUtils.getDataSource(sessionFactory).getConnection()

[mseaton]

Done

[dkayiwa]

What do you think of mentioning that this is a pooled connection?

[mseaton]

Done