Change ValidateConfiguration to throw ConfigurationValidationError on invalid configs.#134
Conversation
…igs. Return false to stop or true to proceed.
bmoyles0117
left a comment
bmoyles0117
left a comment
There was a problem hiding this comment.
General takeaway is that we should split this into two separate PRs. One to handle the transient errors in config validation, and a subsequent PR to change the signature for config validation, while adding a new method that determines whether or not the user intends to use a specific updater.
| } catch (const QueryException& e) { | ||
| // Already logged. | ||
| return false; | ||
| bool DockerReader::ValidateConfiguration() const |
There was a problem hiding this comment.
We discussed to create a new contract where we return void, and throw an exception in the event of an invalid configuration.
| format::Substitute("Polling period {{period}}s cannot be negative", | ||
| {{"period", format::str(period_.count())}})); | ||
| } | ||
| return period_ > time::seconds::zero(); |
There was a problem hiding this comment.
Let's change this to >= again, as we will have a new abstraction for ShouldStartUpdater that will focus strictly on > 0.
There was a problem hiding this comment.
Actually, just moved this code to ShouldStartUpdater — ValidateConfiguration no longer needs a return value.
| }; | ||
|
|
||
| // A representation of all validation errors. | ||
| class ValidationError { |
There was a problem hiding this comment.
ConfigValidationError to be more specific?
| friend class UpdaterTest; | ||
|
|
||
| bool ValidateConfiguration() const; | ||
| bool ValidateConfiguration() const throw(ValidationError); |
There was a problem hiding this comment.
Let's add a new method for ShouldStartUpdater or some equivalent name, that returns true or false. If for whatever reason the ShouldStartUpdater "fails" to validate, by not being able to QueryMaster, we should set unhealthy, but then return true so that we make a best effort attempt to poll / watch, as we had already validated that the configuration was valid. This will help keep us safe from transient errors preventing the updaters from firing off. Arguably, this hints at supporting a SetHealthy, in the event that something is able to recover properly. Let's save that for another PR though.
There was a problem hiding this comment.
Right, I'll leave this whole discussion in the PR comments for now, as we don't really need any of this functionality yet.
| } catch (const QueryException& e) { | ||
| // Already logged. | ||
| return false; | ||
| bool DockerReader::ValidateConfiguration() const |
| format::Substitute("Polling period {{period}}s cannot be negative", | ||
| {{"period", format::str(period_.count())}})); | ||
| } | ||
| return period_ > time::seconds::zero(); |
There was a problem hiding this comment.
Actually, just moved this code to ShouldStartUpdater — ValidateConfiguration no longer needs a return value.
| }; | ||
|
|
||
| // A representation of all validation errors. | ||
| class ValidationError { |
| friend class UpdaterTest; | ||
|
|
||
| bool ValidateConfiguration() const; | ||
| bool ValidateConfiguration() const throw(ValidationError); |
There was a problem hiding this comment.
Right, I'll leave this whole discussion in the PR comments for now, as we don't really need any of this functionality yet.
igorpeshansky
force-pushed
the
igorp-validation-revamp
branch
2 times, most recently
from
90dbab8 to
ce23529
Compare
igorpeshansky
force-pushed
the
igorp-validation-revamp
branch
from
ce23529 to
dc64b1d
Compare
| // Already logged. | ||
| return false; | ||
| throw MetadataUpdater::ConfigurationValidationError( | ||
| "Docker query retry limit reached: " + e.what()); |
There was a problem hiding this comment.
WDYT about adding "validation" here as well. Same elsewhere.
"Docker query validation retry limit reached"
igorpeshansky
changed the title
… invalid configs. (#134) * Split the bool part of ValidateConfiguration out into ShouldStartUpdater.
2 participants
Return false to stop or true to proceed.