refactor(spider-storage)!: Remove the redundant _config suffix from RuntimeConfig's fields. - #433
Conversation
Walkthrough
ChangesRuntime configuration field rename
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
sitaowang1998
left a comment
There was a problem hiding this comment.
Instead of changing the variable name, how about changing the serialization name?
By looking at the diff I don't really see a benefit of doing that. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
components/spider-storage/tests/runtime_recovery_test.rs (1)
204-207: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd coverage for the renamed configuration keys.
This helper only constructs
RuntimeConfigin Rust. It does not verify thatdb,ready_queue,task_instance_pool, andjob_cache_gcare accepted from the storage YAML path.Add a focused deserialisation fixture or test for the rendered configuration. Include a migration test if legacy keys must remain supported.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/spider-storage/tests/runtime_recovery_test.rs` around lines 204 - 207, Add focused YAML deserialization coverage for the renamed RuntimeConfig fields db, ready_queue, task_instance_pool, and job_cache_gc, rather than only constructing them directly in Rust. Verify the rendered storage configuration accepts the new keys, and add a migration test for legacy key support if that compatibility is required.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@components/spider-storage/tests/runtime_recovery_test.rs`:
- Around line 204-207: Add focused YAML deserialization coverage for the renamed
RuntimeConfig fields db, ready_queue, task_instance_pool, and job_cache_gc,
rather than only constructing them directly in Rust. Verify the rendered storage
configuration accepts the new keys, and add a migration test for legacy key
support if that compatibility is required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: eee1b9ee-eb10-4a2c-83d2-3cb1f03029bc
📒 Files selected for processing (5)
components/spider-storage/src/state/runtime.rscomponents/spider-storage/tests/runtime_recovery_test.rstools/deployment/spider-helm/Chart.yamltools/deployment/spider-helm/templates/configmap.yamltools/deployment/spider-helm/values.yaml
Description
Every field in spider-storage's
RuntimeConfigcarries a_configsuffix. The suffix is redundant: the struct is already namedRuntimeConfig, each field's type is already a*Config, and the fields are reached asconfig.db_config,config.ready_queue_config, and so on. The suffix also leaks into the YAML configuration schema, making the keys underruntime:needlessly verbose.This PR drops the suffix from all four fields:
db_configdbready_queue_configready_queuetask_instance_pool_configtask_instance_pooljob_cache_gc_configjob_cache_gcSince these fields are deserialized from
storage.yaml, the configuration keys change accordingly:The Helm chart is updated in lockstep:
templates/configmap.yamlrendersdb:instead ofdb_config:, andvalues.yamlexposesspiderConfig.storage.runtime.ready_queueinstead ofready_queue_config. The chart version is bumped to satisfy the chart linting workflow.Existing storage configuration files must rename these four keys. This applies to custom Helm values as well: the chart passes
spiderConfig.storage.runtimethrough withtoYaml, so a staleready_queue_config(or any other old key) in a user's overrides lands in the rendered config and is rejected by Serde when the storage server starts.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
Breaking Changes
Deployment
Bug Fixes