Skip to content

refactor: extract shared gate config - #295

Merged
shimib merged 3 commits into
llm-d:mainfrom
evacchi:refactor-gate-config
Jul 9, 2026
Merged

refactor: extract shared gate config#295
shimib merged 3 commits into
llm-d:mainfrom
evacchi:refactor-gate-config

Conversation

@evacchi

@evacchi evacchi commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

Refactors common parsing code for Gates to a single place (under pipeline/gate.go), exports a common GateConfig struct, and uses it everywhere; the unified parser accepts a typed JSON, and additional gate-specific config is now passed as map[string]any (instead of map[string]string`, which caused all typed values to be coerced)

Notice this is stacked on #287, so ignore the corresponding commit b99a773 (PR is hence draft for now)

Why is this change needed?

Simplify Gate handling, currently there was quite a bit of duplication. It is also a step further towards improving the config parsing described in #284

How was this tested?

  • Unit tests added/updated
  • Integration/e2e tests added/updated
  • Manual testing performed

Checklist

  • Commits are signed off (git commit -s) per DCO
  • Code follows project contributing guidelines
  • Tests pass locally (make test)
  • Linters pass (make lint)
  • Documentation updated (if applicable)

Related Issues

Stacked on top of #287, related to #284

Copilot AI review requested due to automatic review settings June 29, 2026 12:04
@evacchi
evacchi marked this pull request as draft June 29, 2026 12:05
@evacchi evacchi changed the title Refactor gate config refactor: extract shared gate config Jun 29, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors gate configuration handling by introducing a shared pipeline.GateConfig (with typed map[string]any params) and updating gate creation/parsing across flows and tests to avoid coercing typed JSON values into strings.

Changes:

  • Introduces pipeline.GateConfig and updates the pipeline.GateFactory interface to accept it.
  • Refactors flowcontrol.GateFactory to parse typed params via helper extractors (paramFloat, paramInt, etc.), including structured composite/wait-on-refuse configs.
  • Updates Redis SortedSet / PubSub flows and integration/unit tests to pass typed gate params.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
test/integration/prometheus_gate_factory_test.go Updates integration tests to use pipeline.GateConfig with typed params.
test/integration/gate_factory_redis_test.go Updates Redis quota gate integration tests to use typed params via GateConfig.
test/integration/endpoint_scrape_gate_factory_test.go Updates endpoint-scrape integration tests to use typed params via GateConfig.
pkg/server/runner.go Refactors runner setup and updates pool-gate creation to use GateConfig.
pkg/redis/sortedset_impl.go Switches sortedset queue config gate params to typed map[string]any and passes GateConfig to factory.
pkg/redis/sortedset_impl_test.go Updates tests to assert numeric gate params are preserved as native types.
pkg/pubsub/pubsubimpl.go Embeds pipeline.GateConfig in topic config and passes it to gate factory.
pkg/async/inference/flowcontrol/promql_metric_source_factory.go Updates saturation PromQL source config parsing to accept typed params.
pkg/async/inference/flowcontrol/metric_source_test.go Updates metric source tests for typed params.
pkg/async/inference/flowcontrol/gate_factory.go Refactors gate creation around GateConfig and adds typed param extractors.
pkg/async/inference/flowcontrol/gate_factory_test.go Updates gate factory tests to use GateConfig and typed params.
pkg/async/inference/flowcontrol/gate_factory_composite_test.go Updates composite gate tests to use GateConfig inputs.
pipeline/gate.go Adds GateConfig and updates GateFactory interface signature.
Comments suppressed due to low confidence (1)

pkg/server/runner.go:143

  • Gate params coming from worker-pool config files are still decoded via pipeline.StringMap (which coerces numbers/bools to strings). Even after converting to map[string]any here, those values remain strings, so typed gate params from pool configs are not preserved as described in the PR. Consider switching WorkerPoolConfig to use pipeline.GateConfig / map[string]any to keep types end-to-end.
	poolGates := make(map[string]pipeline.Gate)
	for poolID, pool := range poolsMap {
		if pool.GateType != "" {
			gate, err := gateFactory.CreateGate(pipeline.GateConfig{GateType: pool.GateType, GateParams: toAnyMap(pool.GateParams)})
			if err != nil {
				setupLog.Error(err, "Failed to create pool gate", "poolID", poolID, "gateType", pool.GateType)
				os.Exit(1)
			}
			poolGates[poolID] = gate
			setupLog.Info("Created pool gate", "poolID", poolID, "gateType", pool.GateType, "gateParams", pool.GateParams)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pkg/server/runner.go
Comment thread pkg/async/inference/flowcontrol/gate_factory.go
Comment thread pkg/async/inference/flowcontrol/gate_factory.go
Comment thread pkg/redis/sortedset_impl.go Outdated
@jtechapps

Copy link
Copy Markdown
Collaborator

@evacchi is this ready for review? The other PR has been merged.

@evacchi

evacchi commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

ah yes I missed that merge!

@evacchi
evacchi marked this pull request as ready for review July 8, 2026 07:26
@shimib

shimib commented Jul 8, 2026

Copy link
Copy Markdown
Member

Two non blocking comments:

  1. Test coverage for the new structured branches. The whole point is native/structured config, but those paths aren't exercised: paramGateConfigs []any, paramGateConfig map[string]any, and paramStringMap map[string]any are all still only tested via the JSON-string form. Also paramInt's non-integer-float rejection and the unsupported type branches are untested, and the deleted TestStringMap* tests removed coverage without replacement. A small table test for the param helpers would cover it.
  2. Rebase onto main to drop the stacked refactor: cleanup server/runner #287 commit — that also pulls in fix: TestPoolGating_Timeout may hang indefinitely #296, otherwise the pre-commit job will hang on the integration tests (same issue docs(guide): multi-tenant quota, priority & saturation (Pub/Sub or Redis SortedSet) #285 hit).

Minor: paramString silently %v-coerces a wrong-typed value instead of erroring (inconsistent with the numeric helpers), and dropping StringMap means non-scalar gate_params are now silently accepted rather than rejected — worth a note in the description.

evacchi added 2 commits July 9, 2026 08:35
Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
@evacchi
evacchi force-pushed the refactor-gate-config branch from 77f0134 to 1615bd2 Compare July 9, 2026 06:35
@evacchi
evacchi requested a review from shimib July 9, 2026 06:36
Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
@evacchi
evacchi force-pushed the refactor-gate-config branch from 1615bd2 to c3ca7ae Compare July 9, 2026 06:52
@shimib
shimib merged commit 651c53e into llm-d:main Jul 9, 2026
7 checks passed
shimib added a commit to shimib/llm-d-async that referenced this pull request Jul 9, 2026
Pick up llm-d#295 (gate_params now accepts native typed values) and llm-d#300
(structured ResultMessage) so the guide reflects current config/wire format.

Signed-off-by: Shimi Bandiel <shimib@google.com>
@evacchi
evacchi deleted the refactor-gate-config branch July 30, 2026 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants