[dbnode] [coordinator] Refactor config to use defaults wherever possible #2839
Conversation
| @@ -76,25 +100,25 @@ type DBConfiguration struct { | |||
| Transforms TransformConfiguration `yaml:"transforms"` | |||
|
|
|||
| // Logging configuration. | |||
| Logging xlog.Configuration `yaml:"logging"` | |||
| Logging *xlog.Configuration `yaml:"logging"` | |||
There was a problem hiding this comment.
Thoughts on avoiding pointers in config where it makes sense? e.g., len(str) and *string == nil are effectively equivalent checks, same with struct equivalence (against a zero value), with the benefit of not introducing another zero-like state that needs to be validated as well.
What do you think?
There was a problem hiding this comment.
Unfortunately it's much harder to check if zero struct or not, since sometimes defaults means it should be created into a new default, but other times it's semi valid to have a zero struct. This has burnt us a few times.
Opting to go with the canonical way we do this elsewhere, make it pointer type and adding an accessor "FooOrDefault" was the way we wanted to standardize on unless we have other suggestions that works in pretty much every case we want to cover?
Also need to sometimes return an error, so probably should also standardize on it if needing error return an error as second arg.
| // We use this method to validate fields where the validator package falls short. | ||
| func (c *DBConfiguration) InitDefaultsAndValidate() error { | ||
| // LoggingOrDefault returns the logging configuration or defaults. | ||
| func (c *DBConfiguration) LoggingOrDefault() xlog.Configuration { |
There was a problem hiding this comment.
Can we just go with Logging() and have it default if not set as you have it written? Not sure XOrDefault() provides a ton of informational value.
There was a problem hiding this comment.
Since DBConfiguration already has the field of the same name Logging we have to have some different name (can't have both field and method name the same), and so XOrDefault seems to be the best option for these cases (as opposed to something like GetLogging())
There was a problem hiding this comment.
Yeah it's mainly because of the name collision we can't make it the same.
| @@ -58,6 +58,9 @@ type LimitsConfiguration struct { | |||
| // load on the CPU, which can prevent other DB operations. | |||
| // A setting of 0 means there is no maximum. | |||
| MaxEncodersPerBlock int `yaml:"maxEncodersPerBlock" validate:"min=0"` | |||
|
|
|||
| // Write new series limit per second to limit overwhelming during new ID bursts. | |||
| WriteNewSeriesPerSecond int `yaml:"writeNewSeriesPerSecond" validate:"min=0"` | |||
There was a problem hiding this comment.
nit: This isn't referenced anywhere in this diff, is this relevant?
There was a problem hiding this comment.
This got moved since we had a comment // TODO move v1.
Codecov Report
@@ Coverage Diff @@
## master #2839 +/- ##
========================================
+ Coverage 70.6% 72.0% +1.4%
========================================
Files 1099 1100 +1
Lines 100193 99828 -365
========================================
+ Hits 70789 71965 +1176
+ Misses 24446 22918 -1528
+ Partials 4958 4945 -13
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
|
LGTM, TY @rallen090 ! |
robskillington
deleted the
refactor-config-use-defaults-wherever-possible
branch
Making all the possible breaking changes for cleanup as possible.