Skip to content

fix(meta): unify validation logic for system parameters #22289

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jun 20, 2025

Conversation

Chiro11
Copy link
Contributor

@Chiro11 Chiro11 commented Jun 19, 2025

What's changed and what's your intention?

This PR enhances system parameter validation by introducing checks for initial parameter values at startup.

The validation logic is now shared between parameter initialization and runtime updates, ensuring consistent rule enforcement for all parameters, regardless of their mutability.

Checklist

  • I have written necessary rustdoc comments.
  • I have added necessary unit tests and integration tests.
  • I have added test labels as necessary.
  • I have added fuzzing tests or opened an issue to track them.
  • My PR contains breaking changes.
  • My PR changes performance-critical code, so I will run (micro) benchmarks and present the results.
  • I have checked the Release Timeline and Currently Supported Versions to determine which release branches I need to cherry-pick this PR into.

Documentation

  • My PR needs documentation updates.
Release note

Copy link
Contributor

@Copilot Copilot AI left a comment

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 and centralizes validation for system parameters by renaming existing macros/traits, introducing a new “validate all” macro for initialization, and integrating it into the controller initialization path.

  • Renamed impl_default_validation_on_setimpl_default_validation and ValidateOnSetValidate, and moved immutability checks into set_system_param
  • Added impl_validate_all_params macro to validate all parameters (including immutable ones) during initialization
  • Integrated validate_init_system_params in SystemParamsController and added a test_init unit test

Reviewed Changes

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

File Description
src/common/src/system_param/mod.rs Renamed validation macro/trait, updated impl_set_system_param to enforce immutability, added impl_validate_all_params and its tests
src/meta/src/controller/system_param.rs Imported and invoked validate_init_system_params after missing-param checks in initialization
Comments suppressed due to low confidence (2)

src/common/src/system_param/mod.rs:404

  • Add a doc comment explaining when to use validate_init_system_params (e.g., it should be called once on initialization) and how it differs from per‐param validation during updates.
        pub fn validate_init_system_params(params: &PbSystemParams) -> Result<()> {

src/common/src/system_param/mod.rs:536

  • The test_init function only covers barrier_interval_ms. Consider adding cases for other parameter types (e.g., strings, booleans) and edge values to ensure comprehensive coverage.
    fn test_init() {

@Chiro11 Chiro11 requested review from Li0k and wenym1 June 19, 2025 10:01
@Chiro11 Chiro11 force-pushed the chiro/fix_validate_init_param branch from 17ed105 to 849d738 Compare June 19, 2025 10:29
@wenym1 wenym1 requested review from hzxa21 and yezizp2012 June 20, 2025 05:58
Copy link
Member

@yezizp2012 yezizp2012 left a comment

Choose a reason for hiding this comment

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

Rest LGTM.

@Chiro11 Chiro11 requested a review from zwang28 June 20, 2025 08:18
@Chiro11 Chiro11 requested a review from yezizp2012 June 20, 2025 09:40
Copy link
Member

@yezizp2012 yezizp2012 left a comment

Choose a reason for hiding this comment

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

LGTM

@Chiro11 Chiro11 enabled auto-merge June 20, 2025 09:45
@Chiro11 Chiro11 added this pull request to the merge queue Jun 20, 2025
Merged via the queue into main with commit 6d26579 Jun 20, 2025
33 of 34 checks passed
@Chiro11 Chiro11 deleted the chiro/fix_validate_init_param branch June 20, 2025 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants