Config Parsing and Validation #9
Conversation
| * Constructor that loads just the defaults. | ||
| */ | ||
| public BulletConfig() { | ||
| super(DEFAULT_CONFIGURATION_NAME); |
There was a problem hiding this comment.
As we discussed - can we do validate() here in the constructor?
|
|
||
| // It is ok for this to be static since the VALIDATOR itself does not change for different values for fields | ||
| // in the BulletConfig. | ||
| private static final Validator VALIDATOR = new Validator(); |
There was a problem hiding this comment.
I know we normally do member/static variables at the top of the file, but in this case I would vote to put all this at the bottom just for readability? And maybe a comment line above it separating/explaining it?
There was a problem hiding this comment.
IDEs can collapse stuff as an alternative? Anyway, I left it out for now so the changes to this are easier to spot in my review addressing commits here.
|
|
||
| /** | ||
| * This represents a binary relationship between two fields in a {@link BulletConfig}. You should have defined | ||
| * {@link Entry} for these fields before you try to define relationships between them. You can use this apply a |
There was a problem hiding this comment.
"You can use this to* apply"?
| .castTo(Validator::asInt); | ||
| VALIDATOR.define(COUNT_DISTINCT_AGGREGATION_SKETCH_SAMPLING) | ||
| .defaultTo(DEFAULT_COUNT_DISTINCT_AGGREGATION_SKETCH_SAMPLING) | ||
| .checkIf(Validator::isPositive) |
|
|
||
| VALIDATOR.define(GROUP_AGGREGATION_SKETCH_ENTRIES) | ||
| .defaultTo(DEFAULT_GROUP_AGGREGATION_SKETCH_ENTRIES) | ||
| .checkIf(Validator::isPositiveInt) |
There was a problem hiding this comment.
Check if is a power of two? ...Things don't break if it's not, but it's a waste and might indicate someone is doing something unintended.
|
|
||
| VALIDATOR.define(DISTRIBUTION_AGGREGATION_SKETCH_ENTRIES) | ||
| .defaultTo(DEFAULT_DISTRIBUTION_AGGREGATION_SKETCH_ENTRIES) | ||
| .checkIf(Validator::isPositiveInt) |
There was a problem hiding this comment.
Check if a power of two? Also for TOP_K_AGGREGATION_SKETCH_ENTRIES below?
| .defaultTo(DEFAULT_PUBSUB_CLASS_NAME) | ||
| .checkIf(Validator::isString); | ||
|
|
||
| VALIDATOR.relate("Max should be less or equal to default", SPECIFICATION_MAX_DURATION, SPECIFICATION_DEFAULT_DURATION) |
There was a problem hiding this comment.
"Max should be greater* than or equal to default"? or if this message is an error message: "Max is* less than default"?
|
|
||
| VALIDATOR.relate("Max should be less or equal to default", SPECIFICATION_MAX_DURATION, SPECIFICATION_DEFAULT_DURATION) | ||
| .checkIf(Validator::isGreaterOrEqual); | ||
| VALIDATOR.relate("Max should be less or equal to default", AGGREGATION_MAX_SIZE, AGGREGATION_DEFAULT_SIZE) |
There was a problem hiding this comment.
"Max should be greater* than or equal to default"? or if this message is an error message: "Max is* less than default"?
| .checkIf(Validator::isGreaterOrEqual); | ||
| VALIDATOR.relate("Max should be less or equal to default", AGGREGATION_MAX_SIZE, AGGREGATION_DEFAULT_SIZE) | ||
| .checkIf(Validator::isGreaterOrEqual); | ||
| VALIDATOR.relate("Metadata is enabled and keys are not defined", RESULT_METADATA_ENABLE, RESULT_METADATA_METRICS) |
There was a problem hiding this comment.
If this is an error message: "Metadata is enabled, but* keys are not defined"?
| * Add a piece of meta information. | ||
| * @param key The name of the meta tag | ||
| * | ||
| * @param key The name of the meta tag |
4 participants
Addresses #5