-
Notifications
You must be signed in to change notification settings - Fork 85
Setup Semian Configuration Validation #646
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
Conversation
55eea74 to
ffd990c
Compare
9f81ac5 to
98383d1
Compare
5a21caa to
4b9daca
Compare
d947b5a to
c821272
Compare
error_threshold_timeout contradiction validation + rubocop
a238fe2 to
1787285
Compare
6d454cc to
c3d6143
Compare
c3d6143 to
efaecc7
Compare
d2ffee6 to
9c62bf9
Compare
|
Note: CI on this PR (and others) have been failing because (Ideally we would put these changes in a separate PR, but they are sufficiently trivial) |
|
edit: my previous comment was in error, but this should likely be a minor version bump when we release it |
iandelahorne
left a comment
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.
Looks good, excited to see this in production later
Addressing this issue: #633
Changes
Created
ConfigurationValidatorclass that handles all validations and constraints for the following Semian config values:name: Must be string / symbol and cannot be a duplicate Semian name of one that exists inSemian.resources:circuit_breaker&:bulkhead: Cannot both be disabled:tickets&:quota: If bulkhead, can only have:ticketsor:quota, cannot be both:tickets: Must be between 0 andSemian::MAX_TICKETS, inclusive:quota: Must be a ratio between 0 and 1, inclusive:success_threshold,:error_threshold&:error_timeout: If circuit breaker, validate that theserequiredparams exist.:success_threshold&:error_threshold: If circuit breaker, validate that they are positive numbers:error_timeout,:error_threshold_timeout,:half_open_resource_timeout,:lumping_interval: If circuit breaker and exists, validate that they are non-negative.We have also added constraints to the timeout values to ensure that Semians don't end up in weird scenarios (never opens, closes, never half-opens, etc.):
lumping_interval * (error_threshold - 1) <= error_threshold_timeoutWe also added a new flag -
force_config_validationto pass as an argument to Semian to force configuration validation at runtime if desired (otherwise it will be logged verbosely in I/O)Finally, we added test cases to address each of the above constraints, and modified non-validation related test cases to conform to
ConfigurationValidatorclass.Rationale
We are looking to add validations to configuration options on Semians, to ensure that invalid or otherwise impossible Semian configuration are not created (result in an Exception raised) and do not persist in production. This will help alleviate any cases of incidents or investigations where Semians do not open due to impossible conditions.
Why are some validations required and others are not?
This is to avoid regression in our previous validation logic -- we want to make sure that we don't unexpectedly crash developers' Semians, but we don't allow things that we have previously disallowed.
This includes:
success_threshold, error_threshold, and error_timeoutto be required in circuit breakersticketsorquotato be required in bulkheadsticketsare within bounds(0, MAX_TICKETS](tickets != MAX_TICKETSwas not previously enforced so we use flag instead)quotais within bounds(0, 1](quota != 1was not previously enforced so we use the flag instead)