Repository navigation
[ISSUE #8194] Add opt-in named upstream connection-pool groups for workload isolation - #8299
Conversation
…for workload isolation
Aias00
left a comment
There was a problem hiding this comment.
Stream Agent analysis
FINAL synthesis of PR #8299 (head 9a5d040) adding opt-in named upstream connection-pool groups for workload isolation. All 26 partition reviews passed with no HIGH/CRITICAL defects; complete diff supplied; CI with 64 checks passed. Cross-file assessment from child summaries shows coherent integration of registry, plugin modifications, configuration and tests. No blocking issues found.
Evidence
- 26/26 partition reviews PASS, no blocking findings (all_parts_passed true)
- CI passed: 64 checks complete, legacy pending but passed true
- complete_diff true, global_complete_diff true for head 9a5d040
Recommended next steps
- Maintainer to perform final code review and merge if satisfied
- Validate runtime isolation in staging with representative workloads
- Monitor connection pool metrics tagged by group name post-deploy
Scope: captured diff and GitHub check metadata only; no local tests were executed.
Task: 09e76a6d-c595-435f-bd70-30ef3ca58d2f · Commit: 9a5d0400dd74ab3f6c3a13bc96f40050d1b3bce5
Review verdict: PASS
SYNTHESIS/FINAL scope: assessed cross-file correctness, compatibility, contracts, tests and interactions from all 26 child partition summaries. Each partition (docs, config, registry, plugins, tests, benchmarks) passed with no HIGH/CRITICAL defects. complete_diff and global_complete_diff true, CI passed (64 checks). No child used NEEDS_CHANGES or INCONCLUSIVE. Therefore FINAL PASS is warranted. Limitations: no local test execution; assessment relies on supplied partition reviews and CI status.
Publication: APPROVE (EXPLICIT_PASS)
Review details
This PR introduces opt-in named upstream connection-pool groups for workload isolation. The cross-file review of all 26 partitions (complete diff) found no concrete blocking defects. CI passed (64 checks). The feature includes documentation, configuration constants, registry implementations, plugin wiring, and comprehensive tests (unit, integration, benchmark). Based on the supplied evidence, I recommend merging after standard maintainer review. Note: I have not executed tests locally; this assessment is based on provided partition reviews and CI status.
Aias00
left a comment
There was a problem hiding this comment.
Stream Agent analysis
FINAL synthesis of PR #8299 (head 9a5d040) adding opt-in named upstream connection-pool groups for workload isolation. All 26 partition reviews passed with no HIGH/CRITICAL defects; complete diff supplied; CI with 64 checks passed. Cross-file assessment from child summaries shows coherent integration of registry, plugin modifications, configuration and tests. No blocking issues found.
Evidence
- 26/26 partition reviews PASS, no blocking findings (all_parts_passed true)
- CI passed: 64 checks complete, legacy pending but passed true
- complete_diff true, global_complete_diff true for head 9a5d040
Recommended next steps
- Maintainer to perform final code review and merge if satisfied
- Validate runtime isolation in staging with representative workloads
- Monitor connection pool metrics tagged by group name post-deploy
Scope: captured diff and GitHub check metadata only; no local tests were executed.
Task: 09e76a6d-c595-435f-bd70-30ef3ca58d2f · Commit: 9a5d0400dd74ab3f6c3a13bc96f40050d1b3bce5
Review verdict: PASS
SYNTHESIS/FINAL scope: assessed cross-file correctness, compatibility, contracts, tests and interactions from all 26 child partition summaries. Each partition (docs, config, registry, plugins, tests, benchmarks) passed with no HIGH/CRITICAL defects. complete_diff and global_complete_diff true, CI passed (64 checks). No child used NEEDS_CHANGES or INCONCLUSIVE. Therefore FINAL PASS is warranted. Limitations: no local test execution; assessment relies on supplied partition reviews and CI status.
Publication: COMMENT (CI_NOT_PASSED)
Review details
This PR introduces opt-in named upstream connection-pool groups for workload isolation. The cross-file review of all 26 partitions (complete diff) found no concrete blocking defects. CI passed (64 checks). The feature includes documentation, configuration constants, registry implementations, plugin wiring, and comprehensive tests (unit, integration, benchmark). Based on the supplied evidence, I recommend merging after standard maintainer review. Note: I have not executed tests locally; this assessment is based on provided partition reviews and CI status.
Fixes #8194.
Problem
HttpClientFactorybuilds a singleConnectionProviderfrom process-wide configuration and both built-in HTTP plugins share one client, so a slow bulk workload calling the same upstream origin can starve an interactive one through that shared pool. There is no way to give explicitly configured workloads their own bounded connection pool, and per-origin pools do not help because the contention is between workloads, not origins.Change
This PR adds opt-in named upstream pool groups defined only in trusted startup configuration.
shenyu.httpclient.pool-groupssupports at most 16 groups; every group must use theFIXEDtype, have an explicit unique name other thanshenyu-proxy, a positivemaxConnections, and an omitted-or-positivemaxPendingAcquires(DISABLED,ELASTIC,0and-1fail startup). Validation runs before any provider is created, and a later build failure disposes already-created providers. A newUpstreamPoolGroupRegistry(interface, empty no-op default and immutable implementation) holds one client per group built byHttpClientFactorythrough the sameconfigureClientpath as the default client, so TLS, proxy, keep-alive, idle handlers, http2 negotiation and the shared loop resources are reused while each group owns a boundedConnectionProvider; when the strategy iswebClientthe registry also builds a per-groupWebClientwith the same codec limits. An optionalpoolGroupfield on the divide rule handle selects a group:DividePluginclears any stale selection when an exchange is re-entered and publishes the handle value as an exchange attribute, the plugins resolve the group client once per request, and a reference to an unknown group (or a registry that has closed) returns HTTP 503 instead of silently falling back. Because the attribute is written before the request and never rewritten by resend, the group is pinned for the full request lifetime including retries, failover and the response body. The registry owns only the group providers, rejects all lookups after close, and disposes each exactly once even if one dispose fails; with no groups configured every existing path is untouched. Group metrics are opt-in Reactor Netty Micrometer provider meters (metrics: true), tagged with the finite configured group name; the shipped bootstrap carries micrometer-core through actuator, while a lean classpath leaves metrics disabled.Tests
DivideRuleHandlePoolGroupTestcovers handle binding plus equals/hashCode/toString.DefaultUpstreamPoolGroupRegistryTestcovers case-insensitive lookups, closed-registry rejection and exactly-once disposal that continues after one provider throws.UpstreamPoolGroupIsolationTestdrives two groups against the same loopback origin: a bulk group bounded to one connection saturates while the interactive group on the identical origin keeps serving; an unknown group returns 503 without affecting the default path; failover resolves the selected group for both attempts.HttpClientPoolGroupConfigTestcovers startup wiring, empty configuration, duplicate/oversized/blank/omitted/sentinel names, non-FIXED and unbounded limits, the webClient strategy, and custom-client rejection.HttpClientGroupRegistryRollbackTestproves validation precedes provider creation and a partial build disposes what was already built.HttpClientPoolGroupMetricsTestasserts the group provider meters and name tag with a real loopback request.DividePluginPoolGroupTestcovers handle-to-attribute publication and re-entered exchanges dropping or replacing the previous group. Removing the group routing turns the isolation and re-entry cases red.Verification
mvn -pl shenyu-common,shenyu-plugin/shenyu-plugin-httpclient,shenyu-plugin/shenyu-plugin-proxy/shenyu-plugin-divide,shenyu-spring-boot-starter/shenyu-spring-boot-starter-plugin/shenyu-spring-boot-starter-plugin-httpclient -am test -DskipTests=falsepasses 1042 tests (666 + 174 + 162 + 40); checkstyle is clean on all four modules. Configuration, the FIXED-only/name/bound rules, per-gateway/per-origin scope, 503 behavior, group metrics (including the Micrometer-vs-ShenYu-Prometheus caveat) and restart-only rollback are documented indocs/upstream-pool-groups.md, with a commented example in the bootstrapapplication.ymland a release-note migration entry. The dashboard form does not exposepoolGroupyet; rules can be configured through the Admin API handle JSON.