-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
New config framework for the different schedulers #913
Conversation
var _ Config = &ConstantArrivalRateConfig{} | ||
|
||
// Validate makes sure all options are configured and valid | ||
func (carc ConstantArrivalRateConfig) Validate() []error { |
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.
I am pretty sure we will have to either use a validation library or write ourself one. Even if it means adding code generation. Maybe we can leave it until we actually redo the configuration but ... I don't like code like this that will most definitely miss something
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.
I looked at several validation libraries, and none of them seemed like a big improvement. The biggest obstacle were the nullable types we use - we'd have to make a bunch of adapters to support them properly, which isn't very worth it...
I think we can consolidate some of the repeating checks we'll have into helper functions, or reuse some of the validator functions (but not struct tags and reflection-based stuff) from some library for things like IP addresses, urls, etc., but I'm not sure if will make sense to do more than that. Code generation for sure feels an extreme response to this problem...
And I doubt any other solution will allow us the current flexibility. Consider this validation code below:
if !carc.Rate.Valid {
errors = append(errors, fmt.Errorf("the iteration rate isn't specified"))
} else if carc.Rate.Int64 <= 0 {
errors = append(errors, fmt.Errorf("the iteration rate should be positive"))
}
Notice how only one of the errors will be specified, since it doesn't make sense to show both the not specified and should be positive errors at the same time. Not sure how libraries will handle that correctly. Also, we have the flexibility to write very user-friendly and specific error messages. And we can have subtly different validations between the different configs, for example the validation for the variable arrival-rate doesn't require the value and allows it to be 0, but not negative.
I'm sure we can create something with all of the flexibility, but with less boilerplate, it's just likely going to be very difficult, i.e. I'm not sure if it'd end up worth it.
Codecov Report
@@ Coverage Diff @@
## master #913 +/- ##
==========================================
+ Coverage 70.72% 72.07% +1.35%
==========================================
Files 121 131 +10
Lines 9188 9598 +410
==========================================
+ Hits 6498 6918 +420
+ Misses 2283 2267 -16
- Partials 407 413 +6
Continue to review full report at Codecov.
|
This should also warn users that use combinations of executions settings which are going to be deprecated.
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.
Mostly LGTM, just some minor questions and 2 ❤️ ;)
With this, script runs without any execution params will still have a 1 iteration in 1 VU execution plan, but it will be the per-VU scheduler, so it could eventually be executed both locally and in the cloud.
This is woefully insufficient, but even that was painful to write... Further refactoring and tests will follow, especially relating to JSON/JS configs and mixing configuration between the CLI/env-vars/JS/JSON layers. The effort will hopefully be worth it, since we should be able to reuse these tests to verify that we're not breaking things when we finally tackle #883 properly.
I'm mostly pausing work here until #935 is done and merged back in this branch. The main remaining work there involves adding more config tests, especially ones involving different config layers. And cleaning up the JSON config file support by fixing at least some of the issues described in this comment #883 (comment) I still plan to add a separate error type for execution config conflicts ( #913 (comment) ), but still not sure if that will be in the other branch or in this one |
Config refactoring and tests
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.
LGTM
although we should probably do some pretty significant testing before releasing this. I suppose you have tested it manually as well, as the test currently can't test this accurately, as you have mentioned
@mstoykov, yeah, manual testing is definitely still necessary. Unfortunately, despite all of the refactoring efforts to make the config more testable, there are huge gaps... I've done some manual testing already, but I want to do quite a bit more today and tomorrow, so and I won't merge this until I'm fairly confident there aren't any obvious issues |
The `execution` setting is the only one that could cause an error at this time, when both it and a shortcut (duration/iterations/stages) are set, even if it's not used by k6 itself in the PR. So it needs to be zeroed-out when shortcuts are used, otherwise it couldn't be overwritten at all.
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.
LGTM
This is a (very WIP) subset with the config-related changes from the massive refactoring before we can implement the arrival-rate based execution. I haven't committed any of the unfinished tests, since this is mostly intended to serve as a starting point for discussions about the different
TODO
statements that litter the current code. Still, it should be able to parse (and mostly validate) something like this: