feat: old reset strategy #72
Conversation
modules/firehose/driver_plan.go
Outdated
| func getNewConsumerGroupID(currentConsumerGroupID, deploymentID string) (string, error) { | ||
| const groupNumberSuffixLength = 4 | ||
| suffix := "0000" | ||
|
|
||
| currentConsumerGroupSuffix := currentConsumerGroupID[len(currentConsumerGroupID)-groupNumberSuffixLength:] | ||
| currentConsumerGroupNumber, err := strconv.Atoi(currentConsumerGroupSuffix) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| currentConsumerGroupNumber++ | ||
| newConsumerGroupSuffix := strconv.Itoa(currentConsumerGroupNumber) | ||
|
|
||
| if len(newConsumerGroupSuffix) > groupNumberSuffixLength { | ||
| return "", errGroupNumberLimitCrossed | ||
| } | ||
|
|
||
| newConsumerGroupSuffix = suffix[:groupNumberSuffixLength-len(newConsumerGroupSuffix)] + newConsumerGroupSuffix | ||
|
|
||
| return fmt.Sprintf("%s-%s", deploymentID, newConsumerGroupSuffix), nil | ||
| } |
There was a problem hiding this comment.
why do we care about the suffix length and the range being 0-9999?
why not do this:
var suffixRegex = regexp.MustCompile(`(?P<prefix>^[A-Za-z0-9-]+)(?P<seq>-[0-9]+)?$`)
func getNewConsumerGroupID(curGroup string) string {
matches := suffixRegex.FindStringSubmatch(curGroup)
prefix, sequence := matches[1], matches[2]
seq, err := strconv.Atoi(sequence)
if err != nil {
// ignore the error. but put whatever was here back.
prefix += sequence
} else {
seq++
}
return fmt.Sprintf("%s-%d", prefix, seq)
}🚨 I have not tested this properly. so the regex and index handling for the matches might need some testing & tweaking. but the idea remains the same.
There was a problem hiding this comment.
yeah this looks good 💯
but let's not ignore the error and propagate it back to the user? just in case someone updated it via API?
OR, we can add a suffix "-1", but there is a chance of group name confict. wdys?
There was a problem hiding this comment.
The reason for ignoring is the initial case for example where a sequence is not added as a suffix yet.
During creation, you could simply pass the deployment ID in place of curGroup arg, and get back a valid starting point.
The same goes with update. If someone doesn't put a sequence suffix in the end, this will add one automatically
But yea, the update behaviour is something you need to take a call on.
There was a problem hiding this comment.
hmm I get it, but just to avoid any conflicts, I think we should start with a sequence and keep increasing it.
ishanarya0
requested review from
mabdh and
spy16
and removed request for
spy16
Use new group creation for reset strategy.