feat(http): introduce RavenHttpClientFactory with shared connection p…#21
Open
richard483 wants to merge 4 commits into
Open
feat(http): introduce RavenHttpClientFactory with shared connection p…#21richard483 wants to merge 4 commits into
richard483 wants to merge 4 commits into
Conversation
…ools Each @RavenApiClient previously instantiated its own reactor-netty HttpClient (and hence its own ConnectionProvider + LoopResources). For apps with many declared clients targeting one or two backends this multiplied event loops, selector threads, and connection pools without benefit. Introduce a public RavenHttpClientFactory extension point with a default implementation that shares an HttpClient (with its underlying ConnectionProvider and LoopResources) across all clients sharing a pool key — by default the scheme+host:port of the configured URL. Per-client opt-out is available via: nephren.raven.apiclient.configs.<name>.isolate-pool: true which routes that client through a private pool keyed by "isolated:<client-name>". Use this when a client has materially different timeout/TLS/proxy needs from siblings, or its load profile would starve them. Shared-pool tunables are exposed under: nephren.raven.apiclient.shared-pool.max-connections (default 500) nephren.raven.apiclient.shared-pool.pending-acquire-max-count (default -1) The factory is wired in RavenApiConfiguration with @ConditionalOnMissingBean so users can replace it with a metric-instrumented or TLS-customized implementation. DefaultRavenHttpClientFactory implements DisposableBean so cached ConnectionProviders are released cleanly on context shutdown. Per-call timeouts are layered on top of the cached base HttpClient via .option/.doOnConnected; reactor-netty's HttpClient is copy-on-configure so this does not allocate new pool resources per call. The interceptor's prepareWebClient now resolves the pool key + timeouts and delegates to the factory; the local getHttpClient helper is removed. Adds DefaultRavenHttpClientFactoryTest covering: scheme+host:port pool keys, default port resolution, isolate-pool key shape, base-client caching (same key → one base, different keys → distinct), isolated clients bypassing the owned-provider cache, and DisposableBean cleanup. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new RavenHttpClientFactory extension point so Raven API clients can reuse Reactor Netty HTTP resources instead of each client creating its own HttpClient. In the broader codebase, this shifts client transport setup out of RavenApiClientMethodInterceptor and into a configurable factory intended to centralize pooling behavior.
Changes:
- Add a public
RavenHttpClientFactoryAPI plus a default implementation that derives pool keys from client URLs and caches baseHttpClientinstances. - Add new configuration knobs for shared pools and per-client pool isolation, and wire them into property merging and auto-configuration.
- Update the interceptor to obtain its
HttpClientfrom the factory, and add unit tests around pool-key generation and factory cache behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/test/java/com/nephren/raven/apiclient/unit/DefaultRavenHttpClientFactoryTest.java |
Adds unit coverage for pool-key derivation, cache sizing, and destroy behavior. |
src/main/java/com/nephren/raven/apiclient/properties/RavenSharedPoolProperties.java |
Introduces bindable shared-pool configuration properties. |
src/main/java/com/nephren/raven/apiclient/properties/RavenApiClientProperties.java |
Adds per-client isolatePool configuration. |
src/main/java/com/nephren/raven/apiclient/properties/PropertiesHelper.java |
Extends config merge logic to copy the new isolation flag. |
src/main/java/com/nephren/raven/apiclient/http/RavenHttpClientFactory.java |
Defines the new factory extension point and pool-key helper API. |
src/main/java/com/nephren/raven/apiclient/http/PoolKeys.java |
Adds URL-to-pool-key normalization helpers. |
src/main/java/com/nephren/raven/apiclient/http/DefaultRavenHttpClientFactory.java |
Implements shared/isolated client creation, caching, and disposal. |
src/main/java/com/nephren/raven/apiclient/configuration/RavenApiConfiguration.java |
Registers shared-pool properties and the default factory bean. |
src/main/java/com/nephren/raven/apiclient/aop/RavenApiClientMethodInterceptor.java |
Replaces local HttpClient construction with factory-based creation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+23
to
+36
| /** | ||
| * Resolve an {@link HttpClient} for the given client config and pool key. The factory is | ||
| * free to share an underlying {@code ConnectionProvider}/{@code LoopResources} between | ||
| * calls with the same {@code poolKey}; per-call configuration like timeouts is applied on | ||
| * top of the cached base client and does not affect the cache. | ||
| * | ||
| * @param poolKey identifies the pool. The default factory uses the scheme+host:port of | ||
| * the configured URL, or {@code "isolated:" + clientName} when the client | ||
| * is configured with {@code isolate-pool: true}. | ||
| * @param timeouts per-call connect/read/write timeouts to apply to the returned client. | ||
| * @return an {@link HttpClient} configured with the given timeouts; the underlying | ||
| * connection pool may be shared across calls with the same {@code poolKey}. | ||
| */ | ||
| HttpClient httpClient(String poolKey, ConfigTimeouts timeouts); |
Comment on lines
+47
to
+53
| return base | ||
| .option(ChannelOption.CONNECT_TIMEOUT_MILLIS, (int) timeouts.connect().toMillis()) | ||
| .doOnConnected(connection -> connection | ||
| .addHandlerLast(new ReadTimeoutHandler( | ||
| timeouts.read().toMillis(), TimeUnit.MILLISECONDS)) | ||
| .addHandlerLast(new WriteTimeoutHandler( | ||
| timeouts.write().toMillis(), TimeUnit.MILLISECONDS))); |
Comment on lines
+8
to
+20
| /** | ||
| * Tunables for the Reactor Netty {@code ConnectionProvider} backing every shared | ||
| * {@code HttpClient} pool. Most apps will never need to touch these — they are exposed | ||
| * primarily for environments with high client fan-out or unusually slow downstreams. | ||
| * | ||
| * <p>These values apply to every shared pool the factory creates (one per scheme+host:port | ||
| * key). Per-client opt-out via {@code isolate-pool} is configured on each entry in | ||
| * {@code nephren.raven.apiclient.configs.<name>}. | ||
| */ | ||
| @Data | ||
| @AllArgsConstructor | ||
| @NoArgsConstructor | ||
| @ConfigurationProperties("nephren.raven.apiclient.shared-pool") |
Comment on lines
+36
to
+38
| // boolean primitive — always copy. Default field value (false) makes this safe even | ||
| // when the source did not set it explicitly. | ||
| target.setIsolatePool(source.isIsolatePool()); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/test/java/com/nephren/raven/apiclient/unit/DefaultRavenHttpClientFactoryTest.java (1)
85-87: 💤 Low valueReflection-based field extraction is brittle; prefer package-visible accessors or a dedicated state-inspection method.
extracting("baseClients", ...)andextracting("ownedProviders", ...)couple the tests to private field names. A rename of either field silently breaks all assertions here with a runtimeIntrospectionExceptionrather than a compile error.Consider either exposing package-private
Map<String, HttpClient> baseClients()/Map<String, ConnectionProvider> ownedProviders()accessors (test-scope visibility acceptable for the same package), or adding aint cachedClientCount()/int ownedProviderCount()method to the factory for testability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/nephren/raven/apiclient/unit/DefaultRavenHttpClientFactoryTest.java` around lines 85 - 87, The test is brittle because it uses reflection on private fields ("baseClients" and "ownedProviders"); add package-visible inspection methods on DefaultRavenHttpClientFactory and update the test to call them instead of extracting field names. Either add Map getters e.g. Map<String, HttpClient> baseClients() and Map<String, ConnectionProvider> ownedProviders() with package-private visibility, or add lightweight counters int cachedClientCount() and int ownedProviderCount() if you only need sizes; then change the assertions in DefaultRavenHttpClientFactoryTest to call the new methods (replace extracting("baseClients", ...) and extracting("ownedProviders", ...) with direct calls and assert sizes).src/main/java/com/nephren/raven/apiclient/http/RavenHttpClientFactory.java (1)
43-49: 💤 Low value
poolKeyForas astaticinterface method prevents custom implementations from controlling key derivation.Custom
RavenHttpClientFactoryimplementations can controlhttpClient()resolution but cannot reroute key computation —RavenApiClientMethodInterceptoralways calls the staticRavenHttpClientFactory.poolKeyFor(...)regardless of which implementation is active. Users wanting non-standard key semantics (e.g., grouping several hosts into one pool, or a tenant-aware key) would also need to fork the interceptor.A forward-compatible option is to add a default instance method alongside the static helper:
♻️ Optional refactor — add an overridable instance method
// In RavenHttpClientFactory default String poolKeyFor(String clientName, RavenApiClientProperties.ApiClientConfigProperties config) { if (config.isIsolatePool()) { return "isolated:" + clientName; } return PoolKeys.fromUrl(config.getUrl()); }Then in
RavenApiClientMethodInterceptor.prepareWebClient():-String poolKey = RavenHttpClientFactory.poolKeyFor(name, metadata.getProperties()); +String poolKey = factory.poolKeyFor(name, metadata.getProperties());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/nephren/raven/apiclient/http/RavenHttpClientFactory.java` around lines 43 - 49, The static poolKeyFor(...) on RavenHttpClientFactory prevents implementations from overriding key derivation; add a default instance method with the same signature (default String poolKeyFor(String clientName, RavenApiClientProperties.ApiClientConfigProperties config)) that encapsulates the current logic (or delegates to the existing static helper for backward compatibility), and update RavenApiClientMethodInterceptor.prepareWebClient() to call the factory instance's poolKeyFor(...) instead of RavenHttpClientFactory.poolKeyFor(...), so custom RavenHttpClientFactory implementations can override pool key semantics (keep the static helper only if you want to preserve direct usage but delegate from the default).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/com/nephren/raven/apiclient/http/DefaultRavenHttpClientFactory.java`:
- Around line 85-91: DefaultRavenHttpClientFactory currently calls
provider.disposeLater().block() inside the ownedProviders.forEach which can hang
indefinitely; change this to use a bounded wait (e.g.,
provider.disposeLater().block(Duration.ofSeconds(...)) or
provider.disposeLater().timeout(Duration.ofSeconds(...)).onErrorResume(...).block())
so that disposal of a ConnectionProvider will time out; catch and handle the
timeout/exception (log a warning including the provider identity) instead of
blocking forever and ensure the loop continues for other providers.
- Around line 57-60: The isolated-client branch currently returns
HttpClient.create() which uses Reactor Netty's global shared ConnectionProvider;
change it to create a dedicated ConnectionProvider per isolated poolKey (e.g.,
ConnectionProvider.create(poolKey, ...same max/maxIdle/etc. settings used by the
non-isolated path) and pass that provider into HttpClient.create(provider) so
each "isolated:..." key gets its own private pool; update
DefaultRavenHttpClientFactory to allocate and use ConnectionProvider instances
for the isolated branch (reference symbols: poolKey,
DefaultRavenHttpClientFactory, HttpClient.create(),
ConnectionProvider.create()).
In
`@src/test/java/com/nephren/raven/apiclient/unit/DefaultRavenHttpClientFactoryTest.java`:
- Around line 100-110: The current httpClient logic in
DefaultRavenHttpClientFactory returns HttpClient.create() for
poolKey.startsWith("isolated:"), which uses Reactor Netty's global
ConnectionProvider and does not give per-key isolation; change httpClient to
create and register a dedicated ConnectionProvider for isolated keys (similar to
the non-isolated branch that populates ownedProviders) — e.g. create a
ConnectionProvider with a unique name derived from the poolKey, put it into the
ownedProviders map, build the HttpClient with
HttpClient.create(connectionProvider) and ensure destroy() disposes those owned
ConnectionProviders; update any tests that assume isolated keys are not tracked
in ownedProviders to expect ownedProviders contains the provider for
isolated:<name>.
---
Nitpick comments:
In `@src/main/java/com/nephren/raven/apiclient/http/RavenHttpClientFactory.java`:
- Around line 43-49: The static poolKeyFor(...) on RavenHttpClientFactory
prevents implementations from overriding key derivation; add a default instance
method with the same signature (default String poolKeyFor(String clientName,
RavenApiClientProperties.ApiClientConfigProperties config)) that encapsulates
the current logic (or delegates to the existing static helper for backward
compatibility), and update RavenApiClientMethodInterceptor.prepareWebClient() to
call the factory instance's poolKeyFor(...) instead of
RavenHttpClientFactory.poolKeyFor(...), so custom RavenHttpClientFactory
implementations can override pool key semantics (keep the static helper only if
you want to preserve direct usage but delegate from the default).
In
`@src/test/java/com/nephren/raven/apiclient/unit/DefaultRavenHttpClientFactoryTest.java`:
- Around line 85-87: The test is brittle because it uses reflection on private
fields ("baseClients" and "ownedProviders"); add package-visible inspection
methods on DefaultRavenHttpClientFactory and update the test to call them
instead of extracting field names. Either add Map getters e.g. Map<String,
HttpClient> baseClients() and Map<String, ConnectionProvider> ownedProviders()
with package-private visibility, or add lightweight counters int
cachedClientCount() and int ownedProviderCount() if you only need sizes; then
change the assertions in DefaultRavenHttpClientFactoryTest to call the new
methods (replace extracting("baseClients", ...) and extracting("ownedProviders",
...) with direct calls and assert sizes).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b47a4ddd-8c3b-4a4d-98b6-e75ab585eeb5
📒 Files selected for processing (9)
src/main/java/com/nephren/raven/apiclient/aop/RavenApiClientMethodInterceptor.javasrc/main/java/com/nephren/raven/apiclient/configuration/RavenApiConfiguration.javasrc/main/java/com/nephren/raven/apiclient/http/DefaultRavenHttpClientFactory.javasrc/main/java/com/nephren/raven/apiclient/http/PoolKeys.javasrc/main/java/com/nephren/raven/apiclient/http/RavenHttpClientFactory.javasrc/main/java/com/nephren/raven/apiclient/properties/PropertiesHelper.javasrc/main/java/com/nephren/raven/apiclient/properties/RavenApiClientProperties.javasrc/main/java/com/nephren/raven/apiclient/properties/RavenSharedPoolProperties.javasrc/test/java/com/nephren/raven/apiclient/unit/DefaultRavenHttpClientFactoryTest.java
…fety, inheritance Addresses seven review comments on PR #21: #5/#7 (CodeRabbit, Copilot) — Isolated keys previously called HttpClient.create() with no provider, which uses Reactor Netty's process-global ConnectionProvider. Two clients with isolate-pool: true would still contend on the same global 500-connection pool, contradicting the documented "private set of resources" guarantee. Fix: every key — isolated or shared — now owns a dedicated ConnectionProvider tracked in ownedProviders and disposed on shutdown. #2 (Copilot) — ReadTimeoutHandler / WriteTimeoutHandler attach to the Netty channel; a connection created under client A and later reused by client B would carry A's timeout handlers. The default pool key now includes the connect/read/write timeout fingerprint (scheme://host:port|c=...,r=...,w=...), so two clients only share a pooled channel when they have identical timeouts. Documented the trade-off in DefaultRavenHttpClientFactory's javadoc — custom factories that apply timeouts at the request level (e.g. responseTimeout) may use a narrower key. #4 (Copilot) — isolatePool was a primitive boolean with default false, which the merge helper unconditionally copied. A value set on configs.default.isolate-pool would therefore be silently overwritten back to false by any named config that omitted the key. Switched to Boolean (nullable) and updated PropertiesHelper to copy only when the source has it set, matching the inheritance pattern used by every other optional field. #1 (Copilot) — The factory contract narrowed everything down to (poolKey, ConfigTimeouts) before the impl saw it, which prevented custom factories from making per-client decisions based on URL, headers, or any other property. Widened to httpClient(String clientName, ApiClientConfigProperties config); the default implementation derives its pool key via the static helper RavenHttpClientFactory.defaultPoolKey but custom factories can use any keying strategy they want. Done now (before merge) so the public contract is right from day one. #6 (CodeRabbit) — disposeLater().block() was unbounded; a stalled ConnectionProvider could hang Spring context shutdown indefinitely. Bounded to a 30-second timeout per provider; failures are logged with the pool key and the loop continues for the remaining providers. #3 (Copilot) — docs/setup/index.md previously enumerated every supported nephren.raven.apiclient.* key; updated to document configs.<name>.isolate-pool and the new shared-pool.* tunables. Tests updated: - defaultPoolKey assertions now expect the timeouts fingerprint suffix. - New test confirming same host + different timeouts → distinct base clients (the regression #2 would have masked). - New test confirming two isolated clients on the same backend each own their own ConnectionProvider entry — the regression #5/#7 would have made this fail silently. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+55
to
+58
| return PoolKeys.fromUrl(config.getUrl()) | ||
| + "|c=" + config.getConnectTimeout().toMillis() | ||
| + ",r=" + config.getReadTimeout().toMillis() | ||
| + ",w=" + config.getWriteTimeout().toMillis(); |
Comment on lines
+24
to
+27
| String scheme = uri.getScheme() == null ? "http" : uri.getScheme().toLowerCase(); | ||
| String host = uri.getHost(); | ||
| if (host == null) { | ||
| // not an absolute URL (e.g. just "localhost"); use the raw value as the key |
Comment on lines
+61
to
+66
| * <p>Modeled as the wrapper {@link Boolean} so that a value set on | ||
| * {@code configs.default.isolate-pool} can be inherited by named configs that omit the | ||
| * key — a primitive default would unconditionally overwrite the inherited value back to | ||
| * {@code false} during property merging.</p> | ||
| */ | ||
| private Boolean isolatePool; |
Comment on lines
+13
to
+15
| * <p>These values apply to every shared pool the factory creates (one per scheme+host:port | ||
| * key). Per-client opt-out via {@code isolate-pool} is configured on each entry in | ||
| * {@code nephren.raven.apiclient.configs.<name>}. |
Comment on lines
+54
to
+60
| * When {@code true}, this client gets its own Reactor Netty {@code HttpClient} (and hence | ||
| * its own connection pool and event loops) instead of sharing the pool keyed by | ||
| * scheme+host:port. Treated as {@code false} when unset (most callers benefit from | ||
| * sharing). Set to {@code true} when this client has materially different TLS/proxy | ||
| * needs from other clients targeting the same host, or when its load profile would | ||
| * starve them. | ||
| * |
Comment on lines
+45
to
+48
| void defaultPoolKey_byDefault_usesSchemeHostPortPlusTimeoutFingerprint() { | ||
| String key = RavenHttpClientFactory.defaultPoolKey( | ||
| "any-name", config("http://example.com:8080/api", 1000, 2000, 3000)); | ||
| Assertions.assertThat(key).isEqualTo("http://example.com:8080|c=1000,r=2000,w=3000"); |
Comment on lines
91
to
+96
| private void prepareWebClient() { | ||
| RavenHttpClientFactory factory = applicationContext.getBean(RavenHttpClientFactory.class); | ||
| HttpClient httpClient = factory.httpClient(name, metadata.getProperties()); | ||
| WebClient.Builder builder = applicationContext.getBean(WebClient.Builder.class) | ||
| .exchangeStrategies(getExchangeStrategies()).baseUrl(metadata.getProperties().getUrl()) | ||
| .clientConnector(new ReactorClientHttpConnector(getHttpClient())) | ||
| .clientConnector(new ReactorClientHttpConnector(httpClient)) |
| poolKey, | ||
| sharedPoolProperties.getMaxConnections(), | ||
| sharedPoolProperties.getPendingAcquireMaxCount()); | ||
| return HttpClient.create(provider); |
Comment on lines
+47
to
+49
| These apply to every shared `HttpClient` pool the default factory creates. Most apps will | ||
| never need to touch them — defaults match Reactor Netty conventions and are sized for | ||
| typical client fan-out. |
Comment on lines
+96
to
+99
| ownedProviders.forEach((key, provider) -> { | ||
| try { | ||
| provider.disposeLater().block(DISPOSE_TIMEOUT); | ||
| } catch (RuntimeException e) { |
Addresses ten new review comments on commit 1214d91: #8/#13 (Copilot) — connectTimeout in the default pool key fragmented pools unnecessarily. CONNECT_TIMEOUT_MILLIS only affects opening new sockets and is reapplied per call as a channel option, so it never persists on a pooled channel; only read/write timeouts (which install channel-bound handlers) need to gate sharing. Default key shape is now scheme://host:port|r=<read>,w=<write>; clients differing only in connect timeout now share a pool. Test updated. #9 (Copilot) — PoolKeys.fromUrl fell back to "raw:" for the "localhost:8080" / "localhost:8080/api" forms used in the setup docs, because URI does not populate host for opaque inputs. Now auto-prepends http:// when there is no scheme so doc-following users actually share a pool. Tests added for both shapes. #10 (Copilot) — Boolean isolatePool's inheritance through PropertiesHelper had no test. Added PropertiesHelperTest covering: default → named inheritance, named explicit override of default, both-unset → null (callers treat as false), and that other optional fields follow the same pattern. #11/#12/#15/#16 (Copilot) — Javadoc and setup docs overstated isolation guarantees. The default factory only isolates the ConnectionProvider; event-loop threads come from Reactor Netty's process-global LoopResources and are still shared. Likewise the shared-pool tunables apply to every ConnectionProvider the default factory creates (including isolated ones), not only shared pools. Wording updated in RavenApiClientProperties.isolatePool, RavenSharedPoolProperties, and docs/setup/index.md to match what the code actually does. #17 (CodeRabbit) — destroy() blocked sequentially with a per-provider 30s timeout, so an app with many pool keys could spend 30s × providerCount in shutdown. Now disposes all providers in parallel via Mono.when, with one overall 30s budget; per-provider failures and timeouts are logged but do not gate the others. #14 (Copilot) — The factory wiring through the interceptor had no integration test. Added RavenHttpClientFactoryWiringTests which boots the full app and verifies that the eight @RavenApiClient interfaces declared in the test resources collapse onto exactly two cached HttpClient base instances (one per backend host:port) and two owned ConnectionProviders — proving (a) the interceptor delegates to the factory and (b) the keying strategy actually shares pools across siblings. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+21
to
+32
| static String fromUrl(String url) { | ||
| if (url == null || url.isBlank()) { | ||
| return "default"; | ||
| } | ||
| try { | ||
| URI uri = new URI(url); | ||
| if (uri.getHost() == null && !url.contains("://")) { | ||
| // Inputs like "localhost:8080" or "localhost:8080/api" parse as opaque URIs with no | ||
| // host; re-parse with an http:// prefix so they normalize to the same scheme/host/port | ||
| // shape as fully-qualified URLs. | ||
| uri = new URI("http://" + url); | ||
| } |
Comment on lines
+50
to
+53
| // Two distinct backends → exactly two pool keys, regardless of how many | ||
| // @RavenApiClient interfaces target each one. | ||
| Assertions.assertThat(baseClients).hasSize(2); | ||
| Assertions.assertThat(ownedProviders).hasSize(2); |
Comment on lines
+10
to
+12
| * <p>The default implementation shares connection pools and event loops across all clients | ||
| * targeting the same scheme+host:port (with the same configured timeouts) and applies the | ||
| * per-client connect/read/write timeouts on top. Replace this bean to take full control of |
Comment on lines
+51
to
+52
| and isolated ones. Most apps will never need to touch them; defaults match Reactor Netty | ||
| conventions and are sized for typical client fan-out. |
Comment on lines
+32
to
+36
| * Maximum number of acquire attempts allowed to queue while the pool is saturated. {@code -1} | ||
| * means unbounded (Reactor Netty default). Set a positive value to fail-fast under load | ||
| * instead of letting backpressure pile up. | ||
| */ | ||
| private int pendingAcquireMaxCount = -1; |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/setup/index.md`:
- Around line 50-52: Update the docs text in docs/setup/index.md to avoid
claiming defaults "match Reactor Netty conventions"; instead state that the
project overrides Reactor Netty defaults (see RavenSharedPoolProperties and its
max-connections=500) and explain that the default max-connections is
intentionally higher than the library default. Reference
RavenSharedPoolProperties (and the max-connections setting) so readers
understand the implementation differs from upstream conventions.
- Around line 54-60: The fenced code block containing the properties lines
(e.g., the keys starting with
"nephren.raven.apiclient.shared-pool.max-connections" and
"nephren.raven.apiclient.shared-pool.pending-acquire-max-count") should include
a language identifier to satisfy MD040; change the opening fence from ``` to
```properties so the block is marked as properties and markdownlint-cli2 stops
flagging it.
In
`@src/main/java/com/nephren/raven/apiclient/http/DefaultRavenHttpClientFactory.java`:
- Around line 57-64: The timeout values used in DefaultRavenHttpClientFactory
must be validated before passing to Netty: add a helper method (e.g.,
validateTimeout(Duration timeout, String name)) that checks for null and
non-negative durations and throws IllegalArgumentException with a clear message;
for the connect timeout, clamp/validate the Duration against Integer.MAX_VALUE
before casting to int to prevent overflow when setting
ChannelOption.CONNECT_TIMEOUT_MILLIS; for read/write timeouts call the validator
and pass the validated millisecond value (as long) into the
ReadTimeoutHandler/WriteTimeoutHandler constructors (or convert safely) so
negative or null values are rejected and overflow is avoided.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 52f6a979-c986-422c-b668-84210f6f7af5
📒 Files selected for processing (11)
docs/setup/index.mdsrc/main/java/com/nephren/raven/apiclient/aop/RavenApiClientMethodInterceptor.javasrc/main/java/com/nephren/raven/apiclient/http/DefaultRavenHttpClientFactory.javasrc/main/java/com/nephren/raven/apiclient/http/PoolKeys.javasrc/main/java/com/nephren/raven/apiclient/http/RavenHttpClientFactory.javasrc/main/java/com/nephren/raven/apiclient/properties/PropertiesHelper.javasrc/main/java/com/nephren/raven/apiclient/properties/RavenApiClientProperties.javasrc/main/java/com/nephren/raven/apiclient/properties/RavenSharedPoolProperties.javasrc/test/java/com/nephren/raven/apiclient/RavenHttpClientFactoryWiringTests.javasrc/test/java/com/nephren/raven/apiclient/unit/DefaultRavenHttpClientFactoryTest.javasrc/test/java/com/nephren/raven/apiclient/unit/PropertiesHelperTest.java
✅ Files skipped from review due to trivial changes (2)
- src/test/java/com/nephren/raven/apiclient/unit/PropertiesHelperTest.java
- src/main/java/com/nephren/raven/apiclient/http/PoolKeys.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/nephren/raven/apiclient/properties/RavenSharedPoolProperties.java
#4 (Copilot) — pendingAcquireMaxCount default of -1 was not the Reactor Netty default. Reactor Netty's library default is 2 * maxConnections (bounded); -1 in our config was being passed straight to the builder and produced an unbounded queue, which would amplify a single backend outage into application-wide heap pressure. Per the user's guidance, the value -1 should mean "use Reactor Netty's default", so the factory now skips calling .pendingAcquireMaxCount() when the property is -1 and lets the library compute 2 * maxConnections. Any non-negative value is still applied as-is. Property javadoc and setup docs updated to describe this contract. #1 (CodeRabbit) — Timeout properties were applied with no bounds checks, so a misconfigured null/negative duration would trip a NullPointerException deep in Netty, and a duration that exceeded Integer.MAX_VALUE millis would silently overflow when cast for CONNECT_TIMEOUT_MILLIS. Added validateTimeoutMillis up front with clear messages naming the offending field and client; covered by three new unit tests (null, non-positive, int overflow). #6 (Copilot) — RavenHttpClientFactory javadoc claimed the default implementation shared event loops by pool key. It does not — Reactor Netty's HttpClient.create(provider) reuses the process-global LoopResources regardless of provider. Reworded to say sharing is at the ConnectionProvider level only and that replacing this bean is the way to also isolate event loops. #7 (Copilot) — RavenHttpClientFactoryWiringTests asserted hasSize(2), but emptyExampleClient has no explicit URL config so it falls through to the ApiClientConfigProperties default of "localhost", producing a third pool key. Updated the assertions and javadoc to expect three backends (localhost:80 / 8080 / 8081). #8 (Copilot) — PoolKeys.fromUrl normalized scheme-less inputs like "localhost:8080" for keying, but the interceptor still passed the raw property value into WebClient.Builder.baseUrl(...), so doc-following users would get a malformed base URL at request time. Added RavenHttpClientFactory.normalizedBaseUrl(...) and routed the interceptor through it so the same normalization applies in both places. #3 / #5 (Copilot, CodeRabbit) — Setup docs claimed the connection pool defaults "match Reactor Netty conventions". They don't — the default max-connections=500 is a Raven override above the library default, and pending-acquire-max-count=-1 is a sentinel that delegates to Reactor Netty's actual default (2 * maxConnections). Reworded. #2 (CodeRabbit) — Fenced code blocks in docs/setup/index.md now carry the `properties` language identifier so markdownlint stops flagging MD040 on the new shared-pool block (and the existing ones). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/com/nephren/raven/apiclient/properties/RavenSharedPoolProperties.java`:
- Around line 24-41: Add explicit validation for RavenSharedPoolProperties'
tunables: ensure maxConnections > 0 and pendingAcquireMaxCount is either -1 or
>= 0, and fail-fast by throwing an IllegalArgumentException with a clear message
when invalid. Implement this as a short validate() method on
RavenSharedPoolProperties (or a `@PostConstruct` that calls validate()) and invoke
it before the properties are used to create the shared pool (reference the
fields maxConnections and pendingAcquireMaxCount and the factory call that
consumes them) so misconfiguration is detected at startup rather than silently
delegated to Reactor Netty.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e89d3619-37e5-4e5e-aeea-7c1a5a388052
📒 Files selected for processing (7)
docs/setup/index.mdsrc/main/java/com/nephren/raven/apiclient/aop/RavenApiClientMethodInterceptor.javasrc/main/java/com/nephren/raven/apiclient/http/DefaultRavenHttpClientFactory.javasrc/main/java/com/nephren/raven/apiclient/http/RavenHttpClientFactory.javasrc/main/java/com/nephren/raven/apiclient/properties/RavenSharedPoolProperties.javasrc/test/java/com/nephren/raven/apiclient/RavenHttpClientFactoryWiringTests.javasrc/test/java/com/nephren/raven/apiclient/unit/DefaultRavenHttpClientFactoryTest.java
✅ Files skipped from review due to trivial changes (1)
- docs/setup/index.md
Comment on lines
+24
to
+41
| /** | ||
| * Maximum number of concurrent connections kept open per pool. Defaults to 500, well above | ||
| * Reactor Netty's library default (which is small) so the shared-pool experience matches | ||
| * what callers used to get when each client owned its own pool. | ||
| */ | ||
| private int maxConnections = 500; | ||
|
|
||
| /** | ||
| * Maximum number of acquire attempts allowed to queue while the pool is saturated. The | ||
| * default of {@code -1} is a sentinel meaning "use Reactor Netty's library default", which | ||
| * is {@code 2 * maxConnections}. Set a positive value to override (e.g. fail-fast under | ||
| * load with a small queue, or raise the ceiling for very bursty workloads). | ||
| * | ||
| * <p>The library default is intentionally bounded — never set this to a value that would | ||
| * let a single wedged downstream accumulate unbounded queued acquires and amplify a | ||
| * single-backend outage into application-wide heap pressure.</p> | ||
| */ | ||
| private int pendingAcquireMaxCount = -1; |
There was a problem hiding this comment.
Validate the pool tunables before binding them.
maxConnections and pendingAcquireMaxCount are fed straight into the factory. Right now, a typo like pendingAcquireMaxCount: -2 silently falls back to Reactor Netty’s default queue sizing, and maxConnections <= 0 would be handed off as-is.
Please fail fast here so misconfiguration is caught at startup.
🛠️ Suggested fix
`@Data`
`@AllArgsConstructor`
`@NoArgsConstructor`
`@ConfigurationProperties`("nephren.raven.apiclient.shared-pool")
public class RavenSharedPoolProperties {
@@
private int maxConnections = 500;
@@
private int pendingAcquireMaxCount = -1;
+
+ public void setMaxConnections(int maxConnections) {
+ if (maxConnections <= 0) {
+ throw new IllegalArgumentException("maxConnections must be > 0");
+ }
+ this.maxConnections = maxConnections;
+ }
+
+ public void setPendingAcquireMaxCount(int pendingAcquireMaxCount) {
+ if (pendingAcquireMaxCount < -1) {
+ throw new IllegalArgumentException(
+ "pendingAcquireMaxCount must be -1 or >= 0");
+ }
+ this.pendingAcquireMaxCount = pendingAcquireMaxCount;
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/nephren/raven/apiclient/properties/RavenSharedPoolProperties.java`
around lines 24 - 41, Add explicit validation for RavenSharedPoolProperties'
tunables: ensure maxConnections > 0 and pendingAcquireMaxCount is either -1 or
>= 0, and fail-fast by throwing an IllegalArgumentException with a clear message
when invalid. Implement this as a short validate() method on
RavenSharedPoolProperties (or a `@PostConstruct` that calls validate()) and invoke
it before the properties are used to create the shared pool (reference the
fields maxConnections and pendingAcquireMaxCount and the factory call that
consumes them) so misconfiguration is detected at startup rather than silently
delegated to Reactor Netty.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+103
to
+111
| ConnectionProvider provider = builder.build(); | ||
| ownedProviders.put(poolKey, provider); | ||
| log.debug( | ||
| "#RavenHttpClientFactory creating HttpClient for {} (maxConnections={}," | ||
| + " pendingAcquireMaxCount={})", | ||
| poolKey, | ||
| sharedPoolProperties.getMaxConnections(), | ||
| pendingMax < 0 ? "default(2*maxConnections)" : pendingMax); | ||
| return HttpClient.create(provider); |
Comment on lines
+49
to
+53
| @Bean | ||
| @ConditionalOnMissingBean | ||
| public RavenHttpClientFactory ravenHttpClientFactory( | ||
| RavenSharedPoolProperties sharedPoolProperties) { | ||
| return new DefaultRavenHttpClientFactory(sharedPoolProperties); |
Comment on lines
+55
to
+57
| int connectMs = validateTimeoutMillis(config.getConnectTimeout(), "connect", clientName); | ||
| long readMs = validateTimeoutMillis(config.getReadTimeout(), "read", clientName); | ||
| long writeMs = validateTimeoutMillis(config.getWriteTimeout(), "write", clientName); |
Comment on lines
+97
to
+101
| int pendingMax = sharedPoolProperties.getPendingAcquireMaxCount(); | ||
| // -1 sentinel → leave the builder unset so Reactor Netty's library default | ||
| // (2 * maxConnections) applies. Any other value is a deliberate override. | ||
| if (pendingMax >= 0) { | ||
| builder.pendingAcquireMaxCount(pendingMax); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…ools
Each @RavenApiClient previously instantiated its own reactor-netty HttpClient (and hence its own ConnectionProvider + LoopResources). For apps with many declared clients targeting one or two backends this multiplied event loops, selector threads, and connection pools without benefit.
Introduce a public RavenHttpClientFactory extension point with a default implementation that shares an HttpClient (with its underlying ConnectionProvider and LoopResources) across all clients sharing a pool key — by default the scheme+host:port of the configured URL.
Per-client opt-out is available via:
nephren.raven.apiclient.configs..isolate-pool: true
which routes that client through a private pool keyed by "isolated:". Use this when a client has materially different timeout/TLS/proxy needs from siblings, or its load profile would starve them.
Shared-pool tunables are exposed under:
nephren.raven.apiclient.shared-pool.max-connections (default 500)
nephren.raven.apiclient.shared-pool.pending-acquire-max-count (default -1)
The factory is wired in RavenApiConfiguration with @ConditionalOnMissingBean so users can replace it with a metric-instrumented or TLS-customized implementation. DefaultRavenHttpClientFactory implements DisposableBean so cached ConnectionProviders are released cleanly on context shutdown.
Per-call timeouts are layered on top of the cached base HttpClient via .option/.doOnConnected; reactor-netty's HttpClient is copy-on-configure so this does not allocate new pool resources per call.
The interceptor's prepareWebClient now resolves the pool key + timeouts and delegates to the factory; the local getHttpClient helper is removed.
Adds DefaultRavenHttpClientFactoryTest covering: scheme+host:port pool keys, default port resolution, isolate-pool key shape, base-client caching (same key → one base, different keys → distinct), isolated clients bypassing the owned-provider cache, and DisposableBean cleanup.
Summary by CodeRabbit
Release Notes
New Features
isolate-poolconfiguration setting.shared-poolproperties allow control over maximum connections and pending acquire counts.Documentation