FrankChen021 commented on code in PR #20403:
URL: https://github.com/apache/druid/pull/20403#discussion_r4082349260
##########
processing/src/main/java/org/apache/druid/java/util/http/client/pool/ResourcePool.java:
##########
@@ -62,8 +64,17 @@ public class ResourcePool<K, V> implements Closeable
public ResourcePool(final ResourceFactory<K, V> factory, final
ResourcePoolConfig config,
final boolean eagerInitialization)
{
- this.pool = CacheBuilder.newBuilder().build(
- new CacheLoader<>()
+ this.pool = CacheBuilder.newBuilder()
+
.expireAfterAccess(config.getUnusedConnectionTimeoutMillis(),
TimeUnit.MILLISECONDS)
+ .removalListener(
+ (RemovalNotification<K,
ResourceHolderPerKey<K, V>> notification) -> {
+ if (notification.wasEvicted()) {
+ notification.getValue().close();
Review Comment:
[P2] Ensure expired holders are closed during pool shutdown
**Finding:** Adding expiration makes an entry eligible to be hidden from
Cache.asMap().entrySet() before Guava removes it. ResourcePool.close() iterates
that view but never calls cleanUp(), so an expired holder can be skipped and
its removal notification never delivered; its idle resources then remain open
when the HTTP client is stopped. This is a lifecycle leak on the new expiration
path.
**Suggestion:** Run cache maintenance and deliver pending removal
notifications before or as part of close(), or explicitly invalidate and close
every holder; add a test that lets a key expire and then closes the pool
without another cache access.
##########
processing/src/main/java/org/apache/druid/java/util/http/client/pool/ResourcePool.java:
##########
@@ -62,8 +64,17 @@ public class ResourcePool<K, V> implements Closeable
public ResourcePool(final ResourceFactory<K, V> factory, final
ResourcePoolConfig config,
final boolean eagerInitialization)
{
- this.pool = CacheBuilder.newBuilder().build(
- new CacheLoader<>()
+ this.pool = CacheBuilder.newBuilder()
+
.expireAfterAccess(config.getUnusedConnectionTimeoutMillis(),
TimeUnit.MILLISECONDS)
Review Comment:
[P1] Do not expire holders while resources are borrowed
**Finding:** The cache entry's access time is updated when take() starts,
not when a resource is returned. If an HTTP request keeps all resources for
longer than unusedConnectionTimeout (the documented defaults are 15 minutes for
readTimeout and 4 minutes for unusedConnectionTimeout), the next access expires
the holder even though it still has lent resources. close() leaves those in-use
resources open until their containers return, while the load immediately
creates a fresh holder with maxPerKey new resources, so the configured per-key
connection limit is violated and repeated long-lived requests can accumulate
extra channels and SSL buffers.
**Suggestion:** Tie key eviction to holder idleness, such as deferring
eviction while resources are lent and closing the holder only after the last
borrowed container returns; add a test that holds a resource past the timeout
before taking another resource for the same key.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
