Allow auto-configured HikariDataSource to participate in checkpoint-restore #36422
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
There was a problem hiding this comment.
Thanks for the PR, @christophstrobl. I'm still not sure about the reflection, but ignoring that for the time being I think there are a few refinements that could be made. WDYT?
| if (hikariDataSource.getHikariPoolMXBean() instanceof HikariPool pool) { | ||
|
|
||
| Field closeConnectionExecutor = ReflectionUtils.findField(HikariPool.class, "closeConnectionExecutor"); | ||
| if (closeConnectionExecutor != null) { |
There was a problem hiding this comment.
If the field's not found something's gone really wrong. I think it would be better to fail hard.
There was a problem hiding this comment.
The idea was to be very defensive here, and fall back to checking total connections if for whatever reason HikariPool does not meet the expectations.
There was a problem hiding this comment.
If Hikari doesn't meet expectations then I think we should just give up as we have no way of knowing that our approach is valid. We'll know, through tests, that it works with the version of Hikari in dependency management. If someone overrides that version and Hikari has changed incompatibility I think we'd be better to fail fast. That'll make the problem much easier to diagnose rather than dealing with a checkpoint failure and trying to figure out why it occurred.
|
|
||
| ReflectionUtils.makeAccessible(closeConnectionExecutor); | ||
| Object field = ReflectionUtils.getField(closeConnectionExecutor, pool); | ||
| if (field instanceof ThreadPoolExecutor executor) { |
There was a problem hiding this comment.
Won't this always be a ThreadPoolExecutor?
There was a problem hiding this comment.
Same defensive strategy as with the field above.
I looked up the history of HikaryPool and it seems that part did not change too much in the past. Will take care of this.
| } | ||
| } | ||
| } | ||
| if (this.hasOpenConnections == null) { |
There was a problem hiding this comment.
Wondering about thread safety here. Ideally hasOpenConnections would be final.
wilkinsona
added
the
status: waiting-for-internal-feedback
This commit allows to suspend and gracefully shut down the connection pool of a HikariDataSource within the container lifecycle phases.
This commit makes sure to fail hard when assumptions related to the HikariDataSource are not met to perform lifecycle mgmt. Since the pool may not have been initialized when creating the lifecycle some checks are delayed an will be performed when accessing a managed lifecycle phase.
christophstrobl
force-pushed
the
hikariCP/hikari-lifecycle
branch
from
750217a to
201ac53
Compare
|
@wilkinsona rebased and now failing hard when conditions are not met. I had to delay the reflective lookup to pass all tests. So initially there'll only be a field and type check, but the |
|
Thanks for the updates, @christophstrobl. We discussed this today and decided that we're happy with the reflection as it'll only come into effect when checkpoint-restore is being used. Would you like to take the PR out of draft status? |
wilkinsona
changed the title
wilkinsona
added
theme: checkpoint-restore
wilkinsona
changed the title
|
Thanks very much, @christophstrobl. |
wilkinsona
changed the title
wilkinsona
changed the title
This PR allows to suspend and gracefully shut down the connection pool of a
HikariDataSourcewithin the container lifecycle phases.