Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update connection pool library #558

Closed
julie-sullivan opened this issue Mar 5, 2014 · 14 comments
Closed

Update connection pool library #558

julie-sullivan opened this issue Mar 5, 2014 · 14 comments
Assignees
Milestone

Comments

@julie-sullivan
Copy link
Member

Per @araport

The problem that remains is InterMine not recovering from dropped connection(s). We can simulate this problem on our internal servers by restarting the postgres database. At that point there are constant internal errors (red box) until the tomcat server is restarted. As Jason outlined in the JIRA case, we think that a more robust connection pool (like Commons-DBCP) will solve the problem.

@julie-sullivan
Copy link
Member Author

Tell @eriksf

@rnsmith
Copy link
Contributor

rnsmith commented Jun 5, 2014

I've experimented with replacing the default Postgres connection pool with https://github.com/brettwooldridge/HikariCP (there are lots of other options). This works much better and tolerates a restart of Postgres while the webapp is running. There are a lot more options for setting timeouts on connections.

The properties to configure other connection pools are similar but different. We have a 'generic' system for reading database properties that works by reflection but doesn't easily extend to other drivers.

If we want to provide options for the connection pool we'll need to think about the properties some more. We also may need to massage the old format properties so that mines don't have to make changes.

@rnsmith
Copy link
Contributor

rnsmith commented Jun 9, 2014

I would recommend using HikariCP, it seems to be fast compared to many other connection pools and has all the options we need.

If nobody objects I'll add this as an option. HikariCP is configured slightly differently to the existing postgres driver but I think we should massage the old format properties so only a single change is needed to the default.intermine.properties in each Mine.

  • the old driver and configuration will remain as a fall back if the properties aren't updated
  • change the default properties created in the skeleton Mines
  • log a warning if the old Postgres connection pool is configured
  • change all Mine properties in the main codebase

@julie-sullivan
Copy link
Member Author

Looks good! Let's develop this on a branch of beta.

@rnsmith
Copy link
Contributor

rnsmith commented Jun 9, 2014

This is implemented but causing problems during the MalariaMine build, the build dies on the go source with the error that the database connection has been closed by user request. This is the connection being used by the CopyManager to write binary data to the database.

The go source runs on it's own with an empty or populated database, the build succeeds if the go source is removed.

Caused by: org.postgresql.util.PSQLException: ERROR: canceling statement due to user request
  Where: COPY ontologytermparents, line 141000
    at org.postgresql.core.v3.QueryExecutorImpl.receiveErrorResponse(QueryExecutorImpl.java:2161)
    at org.postgresql.core.v3.QueryExecutorImpl.processCopyResults(QueryExecutorImpl.java:966)
    at org.postgresql.core.v3.QueryExecutorImpl.endCopy(QueryExecutorImpl.java:828)
    at org.postgresql.core.v3.CopyInImpl.endCopy(CopyInImpl.java:59)
    at org.postgresql.copy.CopyManager.copyIn(CopyManager.java:181)
    at org.postgresql.copy.CopyManager.copyIn(CopyManager.java:161)
    at org.intermine.sql.writebatch.FlushJobPostgresCopyImpl.flush(FlushJobPostgresCopyImpl.java:51)

@rnsmith
Copy link
Contributor

rnsmith commented Jun 12, 2014

Having spent all day trying to debug why the new connection pool is closing connections unexpectedly I reverted to the old PGPoolingDataSource and got the same problem. So I think it may be an issue with the Postgres JDBC driver that I changed a couple of weeks ago. Have any builds been run successfully since the driver was updated?

I'll try building my malariamine with a clean checkout of the beta branch next week.

@rnsmith
Copy link
Contributor

rnsmith commented Jun 12, 2014

The beta branch works. Must be something else I've done.

@eriksf
Copy link

eriksf commented Jun 17, 2014

It's great to see progress on this issue. How close are we to getting some code to try out? We (Araport) have been talking to another group who has also had connection pool issues with their mine.

@rnsmith
Copy link
Contributor

rnsmith commented Jun 18, 2014

The build issues are now fixed. The test for connection validity in ObjectStoreWriteInterMineImpl needed a longer timeout to wait for flushing data to the database.

I've updated the properties for Mines that are in the intermine repo, other Mines will need to make a change to minename/default.intermine.integrate.properties and minename/default.intermine.webapp.properties.

For all databases configured, replace:
db.xxx.datasource.class=org.postgresql.ds.PGPoolingDataSource

with:
db.xxx.datasource.class=com.zaxxer.hikari.HikariDataSource

and add:
db.xxx.datasource.dataSourceClassName=org.postgresql.ds.PGSimpleDataSource

@rnsmith
Copy link
Contributor

rnsmith commented Jun 27, 2014

Need to set the minimum idle connections to be a small number (less than max connections) and provide option to override in properties

@rnsmith rnsmith assigned rnsmith and unassigned sergiocontrino Jun 27, 2014
@eriksf
Copy link

eriksf commented Jun 27, 2014

It's not critical, but it would be nice to be able to override any of the properties: https://github.com/brettwooldridge/HikariCP#configuration-knobs-baby.

@rnsmith
Copy link
Contributor

rnsmith commented Jun 27, 2014

@sergiocontrino it should be possible to set any of the properties linked above, do you want to try it out? The code in Database.java alters some property names so the old configuration works but new properties should work fine.

@julie-sullivan
Copy link
Member Author

@sergiocontrino Can we close this ticket? Fixed by #699

@sergiocontrino
Copy link
Member

@julie-sullivan : yes, i have only tested one property (minimumIdle), but in general adding a property should be just a matter of adding a line to the default properties file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants