Refactor ClusterMetadata defaults and validation #4385
Conversation
vytautas-karpavicius
force-pushed
the
cluster-config-validate
branch
from
be08fe6 to
aa509b7
Compare
Pull Request Test Coverage Report for Build 8535af03-8e99-4f98-88aa-cca91d8bc4c7
💛 - Coveralls |
| CurrentClusterName string `yaml:"currentClusterName"` | ||
| // ClusterInformation contains information for each cluster within the replication group | ||
| // Key is the clusterName | ||
| ClusterInformation map[string]ClusterInformation `yaml:"clusterInformation"` |
There was a problem hiding this comment.
Can we change this to "ReplicationGroup"?
e.g. ReplicationGroup map[string]ClusterInformation
Or maybe "ClusterGroup" as it may not do any replication in one cluster setup.
There was a problem hiding this comment.
Yeah, having the same name for both map name and item within that map does not seem right.
ClusterGroup seems reasonable. Or maybe just simply Clusters?
I think it would also make sense to rename ClusterMetadata to (maybe) ClusterGroupMetadata as it contains config for multiple clusters not just single one.
There was a problem hiding this comment.
I think having "Group" explicitly is more friendly as the "s' is quite hard to read/recognize.
Yes, I love the idea of ClusterGroupMetadata
There was a problem hiding this comment.
Renamed:
ClusterInformationtoClusterGroupClusterMetadatatoClusterGroupMetadata
This also means introducing new fields in yaml deprecating old ones. Otherwise having them different could lead to some confusion.
vytautas-karpavicius
force-pushed
the
cluster-config-validate
branch
from
d52e893 to
d92a9b6
Compare
vytautas-karpavicius
force-pushed
the
cluster-config-validate
branch
from
d92a9b6 to
62a3720
Compare
common/config/config.go
Outdated
| ClusterMetadata *ClusterMetadata `yaml:"clusterMetadata"` | ||
| // ClusterGroupMetadata is the config containing all valid clusters and active cluster | ||
| ClusterGroupMetadata *ClusterGroupMetadata `yaml:"clusterGroupMetadata"` | ||
| // ClusterMetadata is deprecated. Please use ClusterGroupMetadata |
There was a problem hiding this comment.
To make it deprecated, start with "Deprecated:"
https://stackoverflow.com/questions/7849663/how-do-you-mark-code-as-deprecated-in-go/36360323
There was a problem hiding this comment.
Good tip! Updated.
vytautas-karpavicius
force-pushed
the
cluster-config-validate
branch
from
770674c to
b784832
Compare
What changed?
Refactored ClusterMetadata configuration validation and filling in defaults.
Why?
To follow structure already established with other config sections. They have
validatefunction that checks for invalid values andfillDefaultswhich sets default values if they are not provided.ClusterMetadata had this logic scattered in a few other places. Collected them and put into separate
cluster.gofile.I'm planning to add more validation/defaults to this section later, that is why I decided to refactor it first.
How did you test it?
Added new unit tests for all validation logic. Instead of panics, normal errors are returned that are easier to tests against.
I also suggest returning them as
multierras this would allow listing all the problems at once, instead of fix -> start server -> discover another error -> fix cycle.Potential risks
Release notes
Documentation Changes