Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
New config framework for the different schedulers #913
Changes from 1 commit
029db7c
15a4128
4a1c941
b50ff18
4821c50
99b0994
ad3f6f4
62c910c
8113438
ce1c967
2451ead
507c5c5
54eb412
9cab261
977b10e
d1943ec
effbf30
2120a12
2c1adf3
62ff4aa
704dcc3
a7d0b6f
b86d5cd
c8c902a
13e506d
5d8b354
588ef66
e76972a
295abd2
9eb4379
2de0923
bf84bc0
93e4eae
227f22c
079282a
238e224
6d19e61
9404af6
2498121
d88be34
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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:
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.