Added Support For beginRequest and endRequest (new version) #2126
Conversation
| @@ -56,6 +56,7 @@ final class PoolEntry implements IConcurrentBagEntry | |||
|
|
|||
| private final boolean isReadOnly; | |||
| private final boolean isAutoCommit; | |||
| private boolean isJDBC43OrLater; | |||
There was a problem hiding this comment.
The choice of variable is inconsistent with the codebase
| @@ -70,6 +71,17 @@ final class PoolEntry implements IConcurrentBagEntry | |||
| this.isAutoCommit = isAutoCommit; | |||
| this.lastAccessed = currentTime(); | |||
| this.openStatements = new FastList<>(Statement.class, 16); | |||
| boolean isJDBC43OrLater = false; | |||
There was a problem hiding this comment.
any specific reason to use isJDBC43OrLater inside the static initializer block? Could this be moved?
There was a problem hiding this comment.
I imagine there is a desired to make the member variable final by having only a single line actually set the value. But this code still has a non-final member.
| boolean isJDBC43OrLater = false; | ||
| try { | ||
| DatabaseMetaData dm = connection.getMetaData(); | ||
| isJDBC43OrLater = ((dm != null) && ( |
There was a problem hiding this comment.
If the connection instance has the methods "beginRequest" and "endRequest", wouldn't that be sufficient to know we can call them, rather than doing a version check?
Maybe using reflection on the connection instance?
I'm wondering if adding the call to getMetaData has any performance implications.
It also seems like it would better to set this pool-wide, rather than have the value calculated for every new connection in the pool. I can't imagine that you would have a pool that would have different JDBC versions for each connection.
There was a problem hiding this comment.
Thank you for the feedback. I chose to do it for each connection because I thought that one could change the version of the driver that is in the application classpath and the pool would start using a new driver, but if that is not possible I propose the following approach:
I will move the check to PoolBase.checkDriverSupport(Connection) and store the value(s) in a private member. This/these value(s) will be added as a parameter on the constructor of the PoolEntry class, and passed in the PoolBase.newEntry() method.
Concerning using relection instead of database metadata, I could argue that if Hikari starts using new JDBC functionality in the future, the code could end up with several boolean variables (one for each method present in from specific version of the JDBC standard) instead of to int values jdbcMajorVersion and jdbcMinorVersion. I am new to Hikari, so please let me know which choice you prefer and I will comply.
| @@ -70,6 +71,17 @@ final class PoolEntry implements IConcurrentBagEntry | |||
| this.isAutoCommit = isAutoCommit; | |||
| this.lastAccessed = currentTime(); | |||
| this.openStatements = new FastList<>(Statement.class, 16); | |||
| boolean isJDBC43OrLater = false; | |||
There was a problem hiding this comment.
I imagine there is a desired to make the member variable final by having only a single line actually set the value. But this code still has a non-final member.
|
Given that this is a Java 11+ version of HikariCP and that request boundaries (Connection.beginRequest() and Connection.endRequest()) are present since Java 9, I have removed the check for JDBC version. |
|
@lfbayer Can you please review my latest changes to this pull request ? |
|
@lfbayer could you please review this request? Thank you! |
|
Hi @lfbayer any update on the merge? Thanks! |
|
Manually resolved conflicts. Merged. |
Continued work from pull request #2079. Addressed review comments #2079 (comment) and added tests.