BTM is not compatible with JDBC 4 API

It's a limitation of the BTM JDBC pool which I'm aware of.

You'll have to live with it for now but this will definitely be improved in the future.

Patch supporting JDBC4 (and theoretically future versions) based on a proxying approach. Check-in pending review. This patch makes BTM buildable under Java 6. The test suite must still be compiled and run under Java 1.4, if testing is desired.

Updated patch.

Added Connection version detection to JdbcPooledConnection. If JDBC4 compliant Connection is detected, the Connection.isValid() method will be used to test the connection. While the standard test query mechanism is likely efficient, it still requires the preparation of a query (one network roundtrip and DB parse/query plan preparation), execution of the query, marshaling of the resultset, and closing of the attendant resources (ResultSet and PreparedStatement). A good driver implementation of the JDBC4 isValid() method does little more than bounce a dummy packet between the client and the server.

I'm going to review the proposed patch ASAP, thanks for it.

I'm going to go ahead and check-in these changes. The proxying is pretty straight-forward, so I assuming any changes upon review will be relatively minor. Because the unit tests were affected by the change (casting Connection to JdbcConnectionHandle, etc.) I'm also checking in the test changes as a pair.

The tests now use Mockito, which requires Java 6, therefore everything should be tested under Java 6. Because of the proxying change, everything can now be built using Java 6.

Here are some basic numbers as a result of the proxying change:

JdbcPreparedStatementHandle:
Before: 456 lines
After: 179 lines

LrcConnectionHandle:
Before: 193 lines
After: 71 lines

JdbcConnectionHandle:
Before: 390 lines
After: 324 lines

Brett,

I'm very happy with your patch so if you have reasonably (ie: in somewhat real condition) tested a build containing this change you can mark this issue resolved.

Thanks!

Marking resolved/fixed.

Ludovic,

I have found a problem with the JDBC4 isValid()/testQuery logic.

Currently, we have code something like this:

private void testConnection(Connection connection) throws SQLException {
   ...
   if ("isValid".equals(query) && jdbcVersionDetected >= 4) {
      try {
	 isValid = (Boolean) isValidMethod.invoke(connection, new Object[] {...});
      } catch (Exception e) {
         log.warn("dysfunctional JDBC4 Connection.isValid() method...  Falling back to test query.");
         jdbcVersionDetected = 3;
      }
}

If the driver is dysfunctional, or the acquisition timeout is negative, we fallback to using the test query. The problem in this case is that we are requiring the test query to be the String "isValid". Because this isn't valid SQL, when (if) we fallback to the test query it will always throw an exception, and thus it would be impossible to obtain a connection.

Related, but minor is in the detectJdbcVersion() method where we have this logic:

private synchronized void detectJdbcVersion(Connection connection) {
   try {
      ...
      jdbcVersionDetected = 4;
      if (poolingDataSource.getTestQuery() != null) {
         log.info("test query is configured but DataSource is JDBC4 or newer and supports isValid(), ignoring test query");
      }
   } catch (Exception e) {
      jdbcVersionDetected = 3;
   }
}

Because we are requiring the test query to be configured as "isValid", it will never be null here and we will always log this informational message.

I don't really like the idea of semantic overloading – using the test query as both a flag or a test query. And I think that testing via JDBC4 Connection.isValid(), but being able to fallback to a SQL test query, is a valid use case.

I add a new property, enableJdbc4ConnectionTest. This will allow both a test query and/or JDBC4 connection testing to be configured. enableJdbc4ConnectionTest is disabled by default.

• If a driver that supports JDBC4 isValid() is detected, but enableJdbc4ConnectionTest is not set, we log an informational.
• If enableJdbc4ConnectionTest is true and the driver supports isValid(), the connection will be tested first by isValid()
• If the isValid() test fails, but there is a test query configured, the test query will be used to validate the connection.
• If only a test query is configured, the behavior is the same as 1.3.3

Checked in code adding new enableJdbc4ConnectionTest flag and semantics.

Brett,

I've reviewed your latest changes and removed the serial version UID from the PoolingDataSource and changed the 'isValid disabled' log to debug.

While reviewing this minor thing there is another more important one I noticed which I missed at first: in JdbcPooledConnection.getConnectionHandle() you added code to force auto-commit to true when in global TX mode. I removed that as the JDBC drivers and DB behaviors in that case is completely random so not only this change could have caused havoc but it also wasn't very consistent as only connection acquired in a global TX context would have run into this.

I've read your comment about XAER_OUTSIDE but if an end user decides to mix both local and global transactions he'd better know what he's doing and the connection pool shouldn't decide on the user's behalf. if the JDBC driver decides to throw a XAException(XAER_OUTSIDE) then this should be reported and not avoided.

I'm cool with that.

Just one question, why remove the serial version UID? My IDE flags the class with a warning because it doesn't have it. Any class that implements Serializable, even through multiple layers of interfaces, should generally have a serial version UID.

Brett,

(I think you forgot to commit your latest change.)

Regarding the serial version UID I prefer to leave it out for now, as stupid this sounds. I'm not sure PoolingDataSource and PoolingConnectionFactory should be serializable at all. I think I added that for full support of JNDI but that seems unnecessary after all.

I also had bad experiences with fixed serial version UIDs where classes weren't compatible between versions but the UID was not updated and I don't want to go through that hassle.