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/05/18 06:01:11 UTC
[GitHub] [commons-dbcp] kyohei-nakamura opened a new pull request #40: Add Evictor threads test code.
kyohei-nakamura opened a new pull request #40:
URL: https://github.com/apache/commons-dbcp/pull/40
When set poolPreparedStatements to true, Evictor thread is disappeared.
Related reports are as follows.
- http://mail-archives.apache.org/mod_mbox/commons-user/202004.mbox/%3CCAD1sBYus4-j4i2Fs6dvzv_p4z0kaiu_8Yz8qUwXxTGAKp1ef1g%40mail.gmail.com%3E
----------------------------------------------------------------
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 #40: Add Evictor threads test code.
Posted by GitBox <gi...@apache.org>.
garydgregory merged pull request #40:
URL: https://github.com/apache/commons-dbcp/pull/40
----------------------------------------------------------------
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 edited a comment on pull request #40: Add Evictor threads test code.
Posted by GitBox <gi...@apache.org>.
garydgregory edited a comment on pull request #40:
URL: https://github.com/apache/commons-dbcp/pull/40#issuecomment-630794885
Hi @kyohei-nakamura
- This test passes on Travis-CI it seems (never mind the openjdk EA failure, that's a build plugin issue)
- Does this mean that using a current SNAPSHOT build works for you? See https://repository.apache.org/content/repositories/snapshots/org/apache/commons/commons-dbcp2/2.8.0-SNAPSHOT/
- Do not use console output (println) in tests please.
----------------------------------------------------------------
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 #40: Add Evictor threads test code.
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #40:
URL: https://github.com/apache/commons-dbcp/pull/40#discussion_r428679539
##########
File path: src/test/java/org/apache/commons/dbcp2/TestBasicDataSource.java
##########
@@ -916,6 +917,34 @@ public void testCreateConnectionFactory() throws Exception {
conn.close();
ds.close();
}
+
+ @Test
+ public void testEvict() throws Exception {
+ ds.setInitialSize(10);
+ ds.setMaxIdle(10);
+ ds.setMaxTotal(10);
+ ds.setMinIdle(5);
+ ds.setNumTestsPerEvictionRun(3);
+ ds.setMinEvictableIdleTimeMillis(100);
+ ds.setTimeBetweenEvictionRunsMillis(1000);
+ ds.setPoolPreparedStatements(true);
+
+ Connection conn = ds.getConnection();
+ conn.close();
+
+ ThreadMXBean threadBean = ManagementFactory.getThreadMXBean();
+ while (Arrays.stream(threadBean.getThreadInfo(threadBean.getAllThreadIds()))
+ .anyMatch(t -> t.getThreadName().equals("commons-pool-evictor-thread"))) {
+ if (ds.getNumIdle() <= ds.getMinIdle()) {
+ break;
+ }
+ Thread.sleep(1000);
Review comment:
If this `1000` matches the one above in the call to `setTimeBetweenEvictionRunsMillis`, it would make sense to refactor the value in a local variable to make obvious that these must indeed match.
----------------------------------------------------------------
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] kyohei-nakamura commented on a change in pull request #40: Add Evictor threads test code.
Posted by GitBox <gi...@apache.org>.
kyohei-nakamura commented on a change in pull request #40:
URL: https://github.com/apache/commons-dbcp/pull/40#discussion_r429753652
##########
File path: src/test/java/org/apache/commons/dbcp2/TestBasicDataSource.java
##########
@@ -916,6 +917,34 @@ public void testCreateConnectionFactory() throws Exception {
conn.close();
ds.close();
}
+
+ @Test
+ public void testEvict() throws Exception {
+ ds.setInitialSize(10);
+ ds.setMaxIdle(10);
+ ds.setMaxTotal(10);
+ ds.setMinIdle(5);
+ ds.setNumTestsPerEvictionRun(3);
+ ds.setMinEvictableIdleTimeMillis(100);
+ ds.setTimeBetweenEvictionRunsMillis(1000);
+ ds.setPoolPreparedStatements(true);
+
+ Connection conn = ds.getConnection();
+ conn.close();
+
+ ThreadMXBean threadBean = ManagementFactory.getThreadMXBean();
+ while (Arrays.stream(threadBean.getThreadInfo(threadBean.getAllThreadIds()))
+ .anyMatch(t -> t.getThreadName().equals("commons-pool-evictor-thread"))) {
+ if (ds.getNumIdle() <= ds.getMinIdle()) {
+ break;
+ }
+ Thread.sleep(1000);
Review comment:
Thanks for your comment.
The Evictor thread runs with a delay of the value of timeBetweenEvictionRunsMillis.
I think that the sleep time of the main thread should refer to that value.
I have fixed it.
----------------------------------------------------------------
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] kyohei-nakamura edited a comment on pull request #40: Add Evictor threads test code.
Posted by GitBox <gi...@apache.org>.
kyohei-nakamura edited a comment on pull request #40:
URL: https://github.com/apache/commons-dbcp/pull/40#issuecomment-631846564
Thank you for your review.
> * Does this mean that using a current SNAPSHOT build works for you? See https://repository.apache.org/content/repositories/snapshots/org/apache/commons/commons-dbcp2/2.8.0-SNAPSHOT/
Yes, this test works fine for current 2.8.0-SNAPSHOT and master branch.
The result of the JUnit test is "Failure", so it is correct.
> * Do not use console output (println) in tests please.
I am sorry, I deleted the console output.
----------------------------------------------------------------
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] kyohei-nakamura commented on pull request #40: Add Evictor threads test code.
Posted by GitBox <gi...@apache.org>.
kyohei-nakamura commented on pull request #40:
URL: https://github.com/apache/commons-dbcp/pull/40#issuecomment-631846564
Thank you for your review.
> * Does this mean that using a current SNAPSHOT build works for you? See https://repository.apache.org/content/repositories/snapshots/org/apache/commons/commons-dbcp2/2.8.0-SNAPSHOT/
Yes, this test works fine for current 2.8.0-SNAPSHOT and master branch.
The result of the JUnit test is "Failure", so it is correct.
> * Do not use console output (println) in tests please.
I am sorry, I deleted the console output.
----------------------------------------------------------------
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 pull request #40: Add Evictor threads test code.
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #40:
URL: https://github.com/apache/commons-dbcp/pull/40#issuecomment-630794885
Hi @kyohei-nakamura
- This test passes on Travis-CI it seems (never mind the openjdk EA failure, that's a build plugin issue)
- Does this mean that using a current SNAPSHOT build works for you?
- Do not use console output (println) in tests please.
----------------------------------------------------------------
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