zabetak commented on code in PR #5661:
URL: https://github.com/apache/hive/pull/5661#discussion_r1987058308
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/datasource/HikariCPDataSourceProvider.java:
##########
@@ -90,7 +94,42 @@ public DataSource create(Configuration hdpConfig, int
maxPoolSize) throws SQLExc
config.addDataSourceProperty(kv.getKey(), kv.getValue());
}
- return new HikariDataSource(initMetrics(config));
+ return new HikariDataSource(initMetrics(config)) {
+ @Override
+ public Connection getConnection() throws SQLException {
Review Comment:
The idea of using a connection proxy between Hikari and Hive for handling
cleanup is resourceful but I feel that it's outside Hive's responsibility.
Adding an override at this point makes the solution Hikari specific so it
will not address the problem if another connection pool is used. The override
changes the behavior of the connection pool so basically by adding a connection
proxy we are essentially "implementing" our own JDBC connection pool with
custom error handling behavior.
Since this is a Hikari extension it would fit better as a contribution to
the Hikari project. However, it is pretty clear from existing discussions:
* https://github.com/brettwooldridge/HikariCP/issues/823
* https://github.com/brettwooldridge/HikariCP/issues/843
that such changes wouldn't be accepted since they are against the philosophy
of the project.
Some other JDBC pool implementations
([commons-dbcp](https://commons.apache.org/proper/commons-dbcp/configuration.html),
[jdbc-pool](https://tomcat.apache.org/tomcat-9.0-doc/jdbc-pool.html)) allow
applications to recover when connections are not closed properly (aka.
abandoned) and in this cases it probably does not make much sense to have this
kind of proxying logic.
Moreover, the cleanup logic at the pool level shouldn't be necessary if
everything was working correctly. There seems to be a bug somewhere and my
feeling is that this is in datanucleus.
Another more subtle point with this proxy approach is performance. Every
single connection will now have an extra overhead on every method invocation.
One of the main selling points of Hikaricp implementation is performance so I
am not sure if this extra overhead is acceptable.
Last but not least, we don't really know what side effects may appear if we
introduce this rollback and close logic since test coverage right now is
probably zero.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
