support updating zone configs for multi-zone dest and cg #218
Conversation
|
Changes Unknown when pulling 332d4ea on mz_upgrade into ** on master**. |
|
Changes Unknown when pulling 15995f0 on mz_upgrade into ** on master**. |
|
Changes Unknown when pulling 15995f0 on mz_upgrade into ** on master**. |
| if !updateRequest.IsSetZoneConfigs() { | ||
| updateRequest.ZoneConfigs = existing.GetZoneConfigs() | ||
| } else { | ||
| isMultiZone = true |
There was a problem hiding this comment.
So if zone-configs are specified, then this is assumed to be multi-zone? Couldn't we set (one) zone-config for a non-multi-zone destination?
There was a problem hiding this comment.
That's right, we disallow single zone config in CLI part. Zone config(s) are designed for multi-zone only.
| if req.IsSetZoneConfigs() && !common.AreCgZoneConfigsEqual(cgDesc.GetZoneConfigs(), req.GetZoneConfigs()) { | ||
| isChanged = true | ||
| cgDesc.ZoneConfigs = req.GetZoneConfigs() | ||
| cgDesc.IsMultiZone = common.BoolPtr(true) |
There was a problem hiding this comment.
I see the same assumption here .. if even one zone-config is set, then it is multi-zone?
common/util.go
Outdated
| zone := l.GetZone() | ||
| foundMatch := false | ||
| for _, r := range right { | ||
| if r.GetZone() == zone { |
There was a problem hiding this comment.
Should we do a case-insensitive compare here?
There was a problem hiding this comment.
btw, another way to do this would obviously be to do run through one of the slices and put it into a map; and then run through the other while looking up against the map. but i guess the zone-config list would be really small to make a difference between O(n^2) vs O(n)?
common/util.go
Outdated
| zone := l.GetZone() | ||
| foundMatch := false | ||
| for _, r := range right { | ||
| if r.GetZone() == zone { |
| }) | ||
| if err == nil { | ||
| // path exists in remote. If uuid is different, then fail the validation | ||
| if remoteDest.GetDestinationUUID() != updateRequest.GetDestinationUUID() { |
There was a problem hiding this comment.
do you want to add a TODO .. that perhaps we could support this scenario eventually (of tying in two destinations with different UUIDs across zones)?
| // path exists in remote. If uuid is different, then fail the validation | ||
| if remoteDest.GetDestinationUUID() != updateRequest.GetDestinationUUID() { | ||
| logger.WithField(common.TagZoneName, common.FmtZoneName(zoneConfig.GetZone())).Error(`destination exists in remote but UUID is different`) | ||
| return false, nil |
There was a problem hiding this comment.
it might be good to return an error (instead of 'nil').
There was a problem hiding this comment.
actually in caller we check if there's any error. If not (and validation fails), we return BadRequestError
There was a problem hiding this comment.
i do see that .. but this is bug-prone. typically/canonically, the code should always check the error-code and only if it is non-nil look at the 'value' returned.
| // cg exists in remote. If uuid is different, then fail the validation | ||
| if remoteCg.GetDestinationUUID() != cgDesc.GetDestinationUUID() || remoteCg.GetConsumerGroupUUID() != cgDesc.GetConsumerGroupUUID() { | ||
| logger.WithField(common.TagZoneName, common.FmtZoneName(zoneConfig.GetZone())).Error(`cg exists in remote but UUID is different`) | ||
| return false, nil |
There was a problem hiding this comment.
return an error, instead of 'nil'?
| @@ -1251,3 +1279,97 @@ func (mcp *Mcp) CreateRemoteZoneConsumerGroupExtent(ctx thrift.Context, createRe | |||
| lclLg.Info("CreateRemoteZoneConsumerGroupExtent: Extent added to consumer group") | |||
| return nil | |||
| } | |||
|
|
|||
| // ValidateDestZoneConfigUpdateRequest validates the zone configs for a UpdateDestinationRequest | |||
| func (mcp *Mcp) ValidateDestZoneConfigUpdateRequest(ctx thrift.Context, logger bark.Logger, updateRequest *shared.UpdateDestinationRequest) (bool, error) { | |||
There was a problem hiding this comment.
I presume you want to name this to not have "update" in it's name .. since you will be probably calling the exact same function from the CreateDestination path too.
| } | ||
|
|
||
| // ValidateCgZoneConfigUpdateRequest validates the zone configs for a UpdateConsumerGroupRequest | ||
| func (mcp *Mcp) ValidateCgZoneConfigUpdateRequest(ctx thrift.Context, logger bark.Logger, updateRequest *shared.UpdateConsumerGroupRequest) (bool, error) { |
There was a problem hiding this comment.
as with 'ValidateDestZoneConfigUpdateRequest', this could get called from the CreateDestination path too, right?
|
Changes Unknown when pulling 1556cbe on mz_upgrade into ** on master**. |
|
Changes Unknown when pulling 1556cbe on mz_upgrade into ** on master**. |
This patch adds support for upgrading a regular destination or consumer group to a multi-zone one.
related thrift change in: uber-archive/cherami-thrift#23