You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2020/07/05 20:25:19 UTC
[GitHub] [commons-dbcp] robertPas opened a new pull request #42: DBCP-566: clearStatementPoolOnReturn
robertPas opened a new pull request #42:
URL: https://github.com/apache/commons-dbcp/pull/42
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-dbcp] psteitz commented on a change in pull request #42: DBCP-566: clearStatementPoolOnReturn
Posted by GitBox <gi...@apache.org>.
psteitz commented on a change in pull request #42:
URL: https://github.com/apache/commons-dbcp/pull/42#discussion_r449927856
##########
File path: src/main/java/org/apache/commons/dbcp2/PoolableConnection.java
##########
@@ -124,6 +124,9 @@ public PoolableConnection(final Connection conn, final ObjectPool<PoolableConnec
protected void passivate() throws SQLException {
super.passivate();
setClosedInternal(true);
+ if (getDelegateInternal() instanceof PoolingConnection) {
+ ((PoolingConnection) getDelegateInternal()).connectionReturnedToPool();
+ }
}
Review comment:
This is a little awkward. I see now how my initial idea does not work. Implementing passivate in PoolingConnection would do nothing as it would never be called. I was tempted to recommend that we change PoolableConnection passivate to cascade to its delegate, but the problem is the delegate could just be a Connection (physical). So no way around this that I can see.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-dbcp] psteitz commented on a change in pull request #42: DBCP-566: clearStatementPoolOnReturn
Posted by GitBox <gi...@apache.org>.
psteitz commented on a change in pull request #42:
URL: https://github.com/apache/commons-dbcp/pull/42#discussion_r449926294
##########
File path: src/main/java/org/apache/commons/dbcp2/BasicDataSource.java
##########
@@ -221,6 +221,8 @@ protected static void validateConnectionFactory(final PoolableConnectionFactory
*/
private boolean poolPreparedStatements = false;
+ private boolean clearStatementPoolOnReturn = false;
Review comment:
I get it now, but disagree. The code is more readable IMO when the default is specified.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-dbcp] robertPas commented on a change in pull request #42: DBCP-566: clearStatementPoolOnReturn
Posted by GitBox <gi...@apache.org>.
robertPas commented on a change in pull request #42:
URL: https://github.com/apache/commons-dbcp/pull/42#discussion_r463733462
##########
File path: src/main/java/org/apache/commons/dbcp2/BasicDataSource.java
##########
@@ -2218,6 +2232,17 @@ public synchronized void setPoolPreparedStatements(final boolean poolingStatemen
this.poolPreparedStatements = poolingStatements;
}
+ /**
+ * Sets wheter the pool of statements (which was enabled with {@link #setPoolPreparedStatements(boolean)}) should
Review comment:
All fixed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-dbcp] psteitz commented on a change in pull request #42: DBCP-566: clearStatementPoolOnReturn
Posted by GitBox <gi...@apache.org>.
psteitz commented on a change in pull request #42:
URL: https://github.com/apache/commons-dbcp/pull/42#discussion_r449928885
##########
File path: src/test/java/org/apache/commons/dbcp2/TestPStmtPoolingBasicDataSource.java
##########
@@ -233,6 +233,52 @@ public void testPStmtPoolingAcrossClose() throws Exception {
assertSame(inner1, inner2);
}
+ @Test
+ public void testPStmtPoolingAcrossCloseWithClearOnReturn() throws Exception {
+ ds.setMaxTotal(1); // only one connection in pool needed
+ ds.setMaxIdle(1);
+ ds.setClearStatementPoolOnReturn(true);
+ ds.setAccessToUnderlyingConnectionAllowed(true);
+ final Connection conn1 = getConnection();
+ assertNotNull(conn1);
+ assertEquals(1, ds.getNumActive());
+ assertEquals(0, ds.getNumIdle());
+
+ final PreparedStatement stmt1 = conn1.prepareStatement("select 'a' from dual");
+ assertNotNull(stmt1);
+ final Statement inner1 = ((DelegatingPreparedStatement) stmt1).getInnermostDelegate();
+ assertNotNull(inner1);
+ stmt1.close();
+
+ final PreparedStatement stmt2 = conn1.prepareStatement("select 'a' from dual");
+ assertNotNull(stmt2);
+ final Statement inner2 = ((DelegatingPreparedStatement) stmt2).getInnermostDelegate();
+ assertNotNull(inner2);
+ assertSame(inner1, inner2); // from the same connection the statement must be pooled
+ stmt2.close();
+
+ conn1.close();
+ assertTrue(inner1.isClosed());
+ assertTrue(inner2.isClosed());
+
+ assertEquals(0, ds.getNumActive());
+ assertEquals(1, ds.getNumIdle());
+
+ final Connection conn2 = getConnection();
+ assertNotNull(conn2);
+ assertEquals(1, ds.getNumActive());
+ assertEquals(0, ds.getNumIdle());
+
+ final PreparedStatement stmt3 = conn2.prepareStatement("select 'a' from dual");
+ assertNotNull(stmt3);
+ final Statement inner3 = ((DelegatingPreparedStatement) stmt3).getInnermostDelegate();
+ assertNotNull(inner3);
+
+ assertNotSame(inner1, inner3); // when aquiring the connection the next time, statement must be new
+
Review comment:
Nice job finding a way to test if the pool is cleared. I would be +1 to exposing a property on PoolingConnection that returns the current size of the statement pool or even a getter for the pool itself (like BDS does). Client code (and tests) would have to cast to PoolingConnection to get it, but I could see how it might be useful. With that added, we could also beef up statement pooling tests like this without reflection hacks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-dbcp] psteitz commented on a change in pull request #42: DBCP-566: clearStatementPoolOnReturn
Posted by GitBox <gi...@apache.org>.
psteitz commented on a change in pull request #42:
URL: https://github.com/apache/commons-dbcp/pull/42#discussion_r449926157
##########
File path: src/main/java/org/apache/commons/dbcp2/BasicDataSource.java
##########
@@ -221,6 +221,8 @@ protected static void validateConnectionFactory(final PoolableConnectionFactory
*/
private boolean poolPreparedStatements = false;
+ private boolean clearStatementPoolOnReturn = false;
Review comment:
Not sure what you mean here, Gary. We definitely want this property to default to false.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-dbcp] garydgregory commented on a change in pull request #42: DBCP-566: clearStatementPoolOnReturn
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #42:
URL: https://github.com/apache/commons-dbcp/pull/42#discussion_r463658170
##########
File path: src/main/java/org/apache/commons/dbcp2/PoolableConnectionFactory.java
##########
@@ -597,6 +600,17 @@ public void setPoolStatements(final boolean poolStatements) {
this.poolStatements = poolStatements;
}
+ /**
+ * Sets wheter the pool of statements (which was enabled with {@link #setPoolStatements(boolean)}) should
Review comment:
Fix spelling.
##########
File path: src/main/java/org/apache/commons/dbcp2/BasicDataSource.java
##########
@@ -2218,6 +2232,17 @@ public synchronized void setPoolPreparedStatements(final boolean poolingStatemen
this.poolPreparedStatements = poolingStatements;
}
+ /**
+ * Sets wheter the pool of statements (which was enabled with {@link #setPoolPreparedStatements(boolean)}) should
Review comment:
Spelling `wheter` -> `whether`.
##########
File path: src/main/java/org/apache/commons/dbcp2/PoolingConnection.java
##########
@@ -599,4 +622,20 @@ public synchronized String toString() {
public boolean validateObject(final PStmtKey key, final PooledObject<DelegatingPreparedStatement> pooledObject) {
return true;
}
+
+ /**
+ * Notification from {@link PoolableConnection} that we returned to the pool.
+ *
+ * @throws SQLException
Review comment:
Specify _when_ this exception is thrown.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-dbcp] robertPas commented on pull request #42: DBCP-566: clearStatementPoolOnReturn
Posted by GitBox <gi...@apache.org>.
robertPas commented on pull request #42:
URL: https://github.com/apache/commons-dbcp/pull/42#issuecomment-657106886
Can you please make one more review?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-dbcp] robertPas commented on a change in pull request #42: DBCP-566: clearStatementPoolOnReturn
Posted by GitBox <gi...@apache.org>.
robertPas commented on a change in pull request #42:
URL: https://github.com/apache/commons-dbcp/pull/42#discussion_r450060557
##########
File path: src/main/java/org/apache/commons/dbcp2/BasicDataSourceMXBean.java
##########
@@ -131,6 +131,13 @@ default String getDefaultSchema() {
*/
boolean isPoolPreparedStatements();
+ /**
+ * See {@link BasicDataSource#isClearStatementPoolOnReturn()}
+ *
+ * @return {@link BasicDataSource#isClearStatementPoolOnReturn()}
+ */
+ boolean isClearStatementPoolOnReturn();
Review comment:
The next version (for `since`) will be 2.8?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-dbcp] garydgregory commented on a change in pull request #42: DBCP-566: clearStatementPoolOnReturn
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #42:
URL: https://github.com/apache/commons-dbcp/pull/42#discussion_r449915921
##########
File path: src/main/java/org/apache/commons/dbcp2/BasicDataSource.java
##########
@@ -221,6 +221,8 @@ protected static void validateConnectionFactory(final PoolableConnectionFactory
*/
private boolean poolPreparedStatements = false;
+ private boolean clearStatementPoolOnReturn = false;
Review comment:
Let's not continue the bad habit of initializing instance variables to their default values.
##########
File path: src/main/java/org/apache/commons/dbcp2/PoolingConnection.java
##########
@@ -65,6 +65,8 @@
/** Pool of {@link PreparedStatement}s. and {@link CallableStatement}s */
private KeyedObjectPool<PStmtKey, DelegatingPreparedStatement> pstmtPool;
+ private boolean clearStatementPoolOnReturn = false;
+
Review comment:
No need to init to default value.
##########
File path: src/main/java/org/apache/commons/dbcp2/BasicDataSourceMXBean.java
##########
@@ -131,6 +131,13 @@ default String getDefaultSchema() {
*/
boolean isPoolPreparedStatements();
+ /**
+ * See {@link BasicDataSource#isClearStatementPoolOnReturn()}
+ *
+ * @return {@link BasicDataSource#isClearStatementPoolOnReturn()}
+ */
+ boolean isClearStatementPoolOnReturn();
Review comment:
You must use a default method here or this will break binary compatibility.
Please add a Javadoc `since` tag for any new public or protected methods.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-dbcp] garydgregory merged pull request #42: DBCP-566: clearStatementPoolOnReturn
Posted by GitBox <gi...@apache.org>.
garydgregory merged pull request #42:
URL: https://github.com/apache/commons-dbcp/pull/42
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-dbcp] psteitz commented on a change in pull request #42: DBCP-566: clearStatementPoolOnReturn
Posted by GitBox <gi...@apache.org>.
psteitz commented on a change in pull request #42:
URL: https://github.com/apache/commons-dbcp/pull/42#discussion_r449929548
##########
File path: src/main/java/org/apache/commons/dbcp2/BasicDataSource.java
##########
@@ -221,6 +221,8 @@ protected static void validateConnectionFactory(final PoolableConnectionFactory
*/
private boolean poolPreparedStatements = false;
+ private boolean clearStatementPoolOnReturn = false;
Review comment:
I agree with Gary's other comments and added a couple of my own. Nice work, Robert! Nice thorough job updating everything.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-dbcp] psteitz commented on a change in pull request #42: DBCP-566: clearStatementPoolOnReturn
Posted by GitBox <gi...@apache.org>.
psteitz commented on a change in pull request #42:
URL: https://github.com/apache/commons-dbcp/pull/42#discussion_r449926157
##########
File path: src/main/java/org/apache/commons/dbcp2/BasicDataSource.java
##########
@@ -221,6 +221,8 @@ protected static void validateConnectionFactory(final PoolableConnectionFactory
*/
private boolean poolPreparedStatements = false;
+ private boolean clearStatementPoolOnReturn = false;
Review comment:
Not sure what you mean here, Gary. We definitely want this property to default to false.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-dbcp] garydgregory commented on a change in pull request #42: DBCP-566: clearStatementPoolOnReturn
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #42:
URL: https://github.com/apache/commons-dbcp/pull/42#discussion_r450180528
##########
File path: src/main/java/org/apache/commons/dbcp2/BasicDataSourceMXBean.java
##########
@@ -131,6 +131,13 @@ default String getDefaultSchema() {
*/
boolean isPoolPreparedStatements();
+ /**
+ * See {@link BasicDataSource#isClearStatementPoolOnReturn()}
+ *
+ * @return {@link BasicDataSource#isClearStatementPoolOnReturn()}
+ */
+ boolean isClearStatementPoolOnReturn();
Review comment:
Not quite, the POM shows `2.8.0-SNAPSHOT` so the next version will be `2.8.0`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org