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.

Sign up for GitHub

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

Jump to bottom

Added Support For beginRequest and endRequest (new version) #2126

Open
wants to merge 19 commits into
base: dev
Choose a base branch
from
Open

Added Support For beginRequest and endRequest (new version) #2126

Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
aa9ee1e
Done begin and endRequest.
Deeptanshu-Chowdhuri May 29, 2023
9c3f087
BeginRequest Changed a bit.
Deeptanshu-Chowdhuri May 30, 2023
0650456
Pool Entry changed only
Deeptanshu-Chowdhuri May 31, 2023
5d5b6e6
Removed Useless Comments
Deeptanshu-Chowdhuri May 31, 2023
eed24d2
Added Try catch to avoid changing function signature and stop at begi…
Deeptanshu-Chowdhuri May 31, 2023
a8e905e
changed Log warning for begin and endRequest
Deeptanshu-Chowdhuri May 31, 2023
248bc3b
changed Formatting and commented changes.
Deeptanshu-Chowdhuri May 31, 2023
726876f
corrected DriverDataSource.java
Deeptanshu-Chowdhuri May 31, 2023
b21683c
Removed comments to comply with the repository.
Deeptanshu-Chowdhuri Jul 7, 2023
df4bfce
Moved get JDBCVersion
fmeheust Oct 24, 2023
116e3ba
PoolEntry begin end request test
fmeheust Oct 24, 2023
0b35a72
typo
fmeheust Oct 24, 2023
2b9a35c
Moved request boundaries to HikariPool + property
fmeheust Oct 30, 2023
5761a5f
Corrected test
fmeheust Oct 30, 2023
5b845f2
Renamed property and tests
fmeheust Oct 30, 2023
a72b575
Removed changed to PoolEntry
fmeheust Oct 30, 2023
a70ccb0
Removed changes to PoolEntry
fmeheust Oct 30, 2023
56ae757
Removed changes to PoolEntry
fmeheust Oct 30, 2023
71e238a
Removed changes to PoolEntry
fmeheust Oct 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 16 additions & 12 deletions src/main/java/com/zaxxer/hikari/pool/PoolEntry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ final class PoolEntry implements IConcurrentBagEntry

private final boolean isReadOnly;
private final boolean isAutoCommit;
private boolean isJDBC43OrLater;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The choice of variable is inconsistent with the codebase


static
{
Expand All @@ -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;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any specific reason to use isJDBC43OrLater inside the static initializer block? Could this be moved?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

try {
DatabaseMetaData dm = connection.getMetaData();
isJDBC43OrLater = ((dm != null) && (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

(dm.getJDBCMajorVersion() > 4) ||
(dm.getJDBCMajorVersion() == 4 && dm.getJDBCMinorVersion() >= 3)
));
} catch (SQLException sqlEx) {
LOGGER.warn("getMetaData Failed for: {},({})", connection, sqlEx.getMessage());
}
this.isJDBC43OrLater = isJDBC43OrLater;
}

/**
Expand All @@ -80,12 +92,8 @@ void recycle()
if (connection != null) {
this.lastAccessed = currentTime();
try {
DatabaseMetaData dm = connection.getMetaData();
if (dm!=null){
if (dm.getJDBCMajorVersion() > 4 ||
(dm.getJDBCMajorVersion() == 4 && dm.getJDBCMinorVersion() >= 3)) {
connection.endRequest();
}
if (this.isJDBC43OrLater){
connection.endRequest();
}
} catch (SQLException e) {
LOGGER.warn("endRequest Failed for: {},({})",connection,e.getMessage());
Expand All @@ -112,12 +120,8 @@ Connection createProxyConnection(final ProxyLeakTask leakTask)
{
Connection newproxyconn= ProxyFactory.getProxyConnection(this, connection, openStatements, leakTask, isReadOnly, isAutoCommit);
try {
DatabaseMetaData dm=connection.getMetaData();
if (dm!=null){
if (dm.getJDBCMajorVersion() > 4 ||
(dm.getJDBCMajorVersion() == 4 && dm.getJDBCMinorVersion() >= 3)) {
connection.beginRequest();
}
if (this.isJDBC43OrLater){
connection.beginRequest();
}
} catch (SQLException e) {
LOGGER.warn("beginRequest Failed for: {}, ({})",connection,e.getMessage());
Expand Down