-
Notifications
You must be signed in to change notification settings - Fork 9
feat: add backward-compatible Specs field to LoadProfile #226
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
base: main
Are you sure you want to change the base?
Conversation
Add Specs []LoadProfileSpec field alongside existing Spec field to enable time-series replay support in future. Update all code to use GetSpecs() accessor method for unified access to both old and new formats. Changes: - Add Specs field with omitempty tags to LoadProfile struct - Add GetSpecs() method to handle both single Spec and Specs formats - Update LoadProfile.Validate() to validate all specs - Update runner, runkperf bench, and warmup commands to use GetSpecs() - Maintain backward compatibility with existing configs All existing configs with 'spec:' continue to work unchanged. New configs can use 'specs:' array format. No functional changes in this commit.
9ff6b04 to
5424357
Compare
api/types/load_traffic.go
Outdated
| // Spec defines behavior of load profile (deprecated, use Specs for single or multiple specs). | ||
| Spec LoadProfileSpec `json:"spec,omitempty" yaml:"spec,omitempty"` | ||
| // Specs defines behaviors of load profile for time-series replay support. | ||
| Specs []LoadProfileSpec `json:"specs,omitempty" yaml:"specs,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
coulc you please explain it more here? Why not just replace existing Spec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not GA yet. so I don't think we should consider deprecation right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
coulc you please explain it more here? Why not just replace existing
Spec?
for backward compatibility we may expect legacy configs still using spec field and hence we need to keep it. if we dont bother backward compatibility on existing configs (and we want to migrate them) then we can simply replace the existing Spec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't upgrade binary during that testcase. so just replace it with new specs and provide more detail about new one spec. thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be resolved
Convert single Spec field to Specs array for time-series replay support. This is a breaking change that removes backward compatibility as the feature is not yet GA. Changes: - Update LoadProfile struct to use Specs []LoadProfileSpec - Remove backward compatibility code (GetSpecs/SetFirstSpec methods) - Update all code references to use .Specs[0] direct access - Convert all YAML configs from spec: to specs: list format - Update tests to match new structure All builds pass and tests verified.
Add Specs []LoadProfileSpec field alongside existing Spec field to enable time-series replay support in future. Update all code to use GetSpecs() accessor method for unified access to both old and new formats.
Changes:
All existing configs with 'spec:' continue to work unchanged. New configs can use 'specs:' array format. No functional changes in this commit.