Add metrics support for idle jdbc connections #17504
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
snicoll
added
the
status: waiting-for-feedback
|
@snicoll, sorry, you are right, I just forgot to update it, I will fix it |
spring-projects-issues
added
status: feedback-provided
| @@ -185,6 +193,27 @@ private static HikariDataSource createHikariDataSource(String poolName) { | |||
| return hikariDataSource; | |||
| } | |||
|
|
|||
| @Configuration | |||
There was a problem hiding this comment.
I don't understand why initializing the pool here is necessary. Can you please clarify why this is done as part of this PR?
| @@ -51,6 +57,27 @@ void dataSourceIsInstrumented() { | |||
| }); | |||
| } | |||
|
|
|||
| @Configuration | |||
| static class HikariPool { | |||
| /** | ||
| * Return the number of established but idle connections. Can also return {@code null} | ||
| * if that information is not available. | ||
| * @return the number of established but idle connections or {@code null} |
There was a problem hiding this comment.
This method should have a @since 2.2.0 on it
| return pool.getIdleConnections(); | ||
| } | ||
|
|
||
| return super.getIdle(); |
There was a problem hiding this comment.
I don't think calling super is what I'd do here
| * if that information is not available. | ||
| * @return the number of established but idle connections or {@code null} | ||
| */ | ||
| default Integer getIdle() { |
There was a problem hiding this comment.
Rather than adding this method at the end I'd group it logically with a similar concept. Right below getActive sounds good. As a result, methods in implementations and tests should be reordered accordingly.
| @@ -69,4 +72,14 @@ public Boolean getDefaultAutoCommit() { | |||
| return getDataSource().isAutoCommit(); | |||
| } | |||
|
|
|||
| @Override | |||
| public Integer getIdle() { | |||
| HikariPool pool = getHikariPool(); | |||
There was a problem hiding this comment.
I'd do the same as what we do in getActive()
snicoll
added
status: waiting-for-feedback
|
@snicoll, thank you for suggestions, I pushed changes to follow by this suggestion. Regarding the tests, it was just my mistake and I have fixed it now. Could you please check that I don't forget something else? |
spring-projects-issues
added
status: feedback-provided
| // Make sure the pool is initialized | ||
| JdbcTemplate jdbcTemplate = new JdbcTemplate(getDataSourceMetadata().getDataSource()); | ||
| jdbcTemplate.execute((ConnectionCallback<Void>) (connection) -> null); | ||
| assertThat(getDataSourceMetadata().getIdle()).isEqualTo(Integer.valueOf(1)); |
There was a problem hiding this comment.
This assumes that an Hikari connection pool with min=0; max=2 has 1 initial connection.
The other connection pools seem to default to 0 - or is that because the other connection pools are not initiliazed ?
There was a problem hiding this comment.
yes, it's because the other connection pools are not initialized
There was a problem hiding this comment.
I guess I need to add the same tests with initialization pool for other connection pools, what do you think about it?
There was a problem hiding this comment.
We should configure the pool explicitly so that the idle connection stays in the pool indeed.
There was a problem hiding this comment.
sorry, I'm not sure that I catch you, Is this way of initializing pool good for the tests?
There was a problem hiding this comment.
We could use a dedicated HikariDataSource dataSource = createDataSource(0, 2); in the test itself. If there is a setting that we could set to increase the time an idle connection stays in the pool, that would be nice too as we ensure that our test is asserting the conditions as we expect them.
There was a problem hiding this comment.
ok, I have got it.
snicoll
removed
status: feedback-provided
snicoll
changed the title
|
Thanks again for the PR @ayudovin. That's not exactly what I had in mind for the test, I've updated your proposal with what I had in mind. |
|
@snicoll, ok, thank you! |
this enhancement