blob: harden HTTP connection pools across clouds to cut tail latency under bursty traffic

[hkhiri]

Summary

High-throughput services take latency and reliability hits from connection-pool behavior that, until now, could only be tuned on AWS. This extends both levers to GCP and Ali so the win is uniform no matter which cloud a service runs on:
- withDisableConnectionReaper(true): keeps pooled connections warm through traffic troughs instead of reaping them right before the next spike. Kills the fresh TCP/TLS handshakes that inflate p99 on spiky workloads (e.g. search fan-out). Fewer handshakes = lower cost per request under load.
- withTcpKeepAlive(true): detects and evicts dead peers behind load balancers/NAT before they get handed to a request, removing a class of sporadic, hard-to-reproduce failures on long-lived pools.

Both default to unset, so existing callers see zero change (SDK defaults kept).

Coverage across providers:
- GCP: both levers wired into the Apache pool; reaper suppresses the idle evictor, keep-alive sets the socket option on the connection manager.
- Ali: reaper wired into both the sync and async Apache5 clients. TCP keep-alive is a safe, documented no-op ; Alibaba's OSS SDK exposes no keep-alive setting.

Tested with unit tests on GCP and Ali (sync + async). Blob suites green (blob-client 283, blob-ali 211, blob-gcp 331), AWS regression 148, checkstyle clean.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 91.83673% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.43%. Comparing base (19c33e6) to head (7d60f06).

Files with missing lines	Patch %	Lines
.../multicloudj/blob/ali/async/AliAsyncBlobStore.java	80.00%	0 Missing and 2 partials ⚠️
.../salesforce/multicloudj/blob/aws/AwsBlobStore.java	88.88%	0 Missing and 1 partial ⚠️
.../multicloudj/blob/aws/async/AwsAsyncBlobStore.java	88.88%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #533      +/-   ##
============================================
+ Coverage     82.39%   82.43%   +0.03%     
  Complexity      662      662              
============================================
  Files           210      210              
  Lines         14334    14369      +35     
  Branches       1932     1952      +20     
============================================
+ Hits          11811    11845      +34     
  Misses         1696     1696              
- Partials        827      828       +1     

Flag	Coverage Δ
unittests	82.43% <91.83%> (+0.03%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[LihaoLiuXs]

withDisableConnectionReaper(true) is inert on this provider on its own, and silently discards withIdleConnectionTimeout(...) when both are set.

evictIdleConnections is the only idle-eviction mechanism in this client, and it was already conditional on getIdleConnectionTimeout() != null. So a caller who sets only the new flag gets a byte-identical client — there was no reaping here to suppress. A caller who had tuned withIdleConnectionTimeout(Duration.ofSeconds(30)) (a common way to stay under a load balancer's idle timeout) and then adopts this flag for the tail-latency win silently loses idle eviction entirely: pooled connections are now retained indefinitely, and withIdleConnectionTimeout — documented in BlobStoreBuilder as "the maximum amount of time that a connection should be allowed to remain open while idle" — stops being honored, with no error and no mention in the new javadoc.

That makes the knob provider-dependent: where the underlying SDK reaps idle connections by default (50–60s windows), the flag is a real change standalone; here it is a no-op standalone and destructive in combination.

Suggested fix: fall back to a documented default idle window when idleConnectionTimeout is unset so the flag has a standalone effect, and document the disableConnectionReaper × idleConnectionTimeout interaction on BlobStoreBuilder#withDisableConnectionReaper.

[LihaoLiuXs]

This SAM is being reshaped concurrently by #532, and the two changes are not composable as written.

This PR adds a 5th positional parameter here and widens the gate below (|| getDisableConnectionReaper() != null) so the flag forces an explicitly constructed transport. #532 widens that same gate for metricsPublisher and adds AliInstrumentedHttpClientFactory.instrument(...), whose javadoc states it deliberately wraps "the one the SDK builder already produced, so all of the SDK's default transport behavior (TLS trust config, idle-connection reaping, proxy, timeouts, pool sizing) is preserved untouched" — the assumption this PR invalidates.

Whichever lands second has to reconcile by hand inside the AliBlobStore/AliAsyncBlobStore lambdas: useReaper(...) must be applied to the builder and the concrete built client passed through instrument(...). The instrument overloads are typed on Apache5HttpClient/Apache5AsyncHttpClient rather than HttpClient, so the composition order is load-bearing and it is easy to silently drop the reaper flag or the metrics wrapper while resolving the conflict.

Suggested fix: replace the growing positional list with a small options/config object (proxyHost, readWriteTimeout, maxConnections, idleConnectionTimeout, disableConnectionReaper, …) so both changes extend one type instead of the same signature, and agree on a merge order.