Remove legacy top-level Configuration.Concurrency and dead set command - #1093
Remove legacy top-level Configuration.Concurrency and dead set command#1093evanphx wants to merge 2 commits into
set command#1093Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
Included review availability: 1 review is currently available. Your included PR review attempts over the past 7 days set your current allowance at 6 reviews per hour. 📝 WalkthroughWalkthroughThe RPC configuration contract removes Merge Risk: ⚪ Minimal · up to This PR removes unreachable legacy concurrency configuration and preserves per-service concurrency behavior; no actionable merge-blocking risk remains after normal checks and review. Comment |
There was a problem hiding this comment.
🍪 biscuit: ✅ ready to merge — auto-review, non-blocking
This is a clean, focused removal of two dead pieces: the top-level Configuration.Concurrency/AutoConcurrency fields (superseded by per-service concurrency) and the set CLI command that was the only consumer of those fields.
What I checked:
- No surviving callers of
HasConcurrency,SetConcurrency,AutoConcurrency, orcommands.Setanywhere in the codebase — the search came back empty across all 400 files scanned. - The CBOR/JSON index reservation in
rpc.ymlis handled correctly: indices 1 and 2 are left as a gap (with an explanatory comment) rather than recycled, so old clients decoding a new payload won't misread a future field through a stale index. That's the right call for a binary-serialised protocol. - The generated
rpc.gen.gomatches the schema change exactly — theAutoConcurrencytype and the twoconfigurationDatafields drop together cleanly. app_status.goremoves both the human-readable and JSON output paths for the old concurrency field, and correctly tightens theoutput.Configurationguard fromcfg.Concurrency > 0 || len(cfg.EnvVars) > 0to justlen(cfg.EnvVars) > 0.- The remaining concurrency machinery (
ServiceConfig.ConcurrencyMode,ServiceConcurrencyincore_v1alpha, theappconfigper-service concurrency block) is untouched — this change is precisely scoped to the legacy top-level fields.
One minor cosmetic note: removing the three concurrency lines from app_status.go around line 106 left a blank line immediately after ctx.Printf("\n%s\n", ...). It's harmless but mildly untidy — not worth blocking on.
Overall the change is safe, well-scoped, and the protocol-safety reasoning in the comment is sound.
🍪 full review note · comment /biscuit review to run biscuit again.
phinze
left a comment
There was a problem hiding this comment.
🤖 I chased the three claims and they all hold. Set never made it into RegisterAll, GetConfiguration never populated the field, and regenerating the schema gives back exactly what's committed.
The one thing I'd note for later is that the app.toml side of this outlives the wire side. appconfig.go:285 still parses a top-level concurrency that nothing reads, and it's fairly stuck there, because DisallowUnknownFields() means dropping the field would turn any app.toml still setting the key into a parse error.
Vestigial code from before concurrency became per-service, noticed while reviewing #667.
cli/commands/set.gois gone. It definedm set -c <n>, writing the top-levelConfiguration.Concurrency. It was never registered in the dispatch table, so there was no way to invoke it — confirmed by grepping for any reference tocommands.Set.HasConcurrency()checks inapp_status.goare gone, in both the human and JSON paths.GetConfigurationnever populated that field, so both checks were always false: the "Concurrency:" line could not print, and theconcurrencykey could not appear in--format json. The JSON field wasomitempty, so nothing consuming that output sees a change.concurrencyandauto_concurrencyare out ofConfigurationinapi/app/rpc.yml, along with the now-unreferencedAutoConcurrencytype. Indices 1 and 2 are left unused with a comment rather than reassigned, so an old client cannot read a new field through an old index.Per-service concurrency (
services.*.concurrency,SetConcurrencyMode/SetNumInstances) is untouched — that is the live path and this removes none of it.go build ./...,go test ./cli/commands/... ./appconfig/...andgo generate ./api/app/...are clean.Closes MIR-796