Factor out task queue name mangling into new package #3549
Conversation
common/tqname/tqname.go
Outdated
| func MustFromBaseName(name string) Name { | ||
| n, err := FromBaseName(name) | ||
| if err != nil { | ||
| return Name{baseName: "__INVALID__"} |
There was a problem hiding this comment.
I want to use a value that makes it obvious that there is a bug, and not cover it up
common/tqname/tqname.go
Outdated
| return mangledTaskQueuePrefix + tn.baseName + suffixDelimiter + strconv.Itoa(tn.partition) | ||
| } | ||
| // versioned always use prefix | ||
| return mangledTaskQueuePrefix + tn.baseName + suffixDelimiter + tn.versionSet + versionSetDelimiter + strconv.Itoa(tn.partition) |
There was a problem hiding this comment.
I changed it. Not sure it's any clearer 🤷
common/tqname/tqname.go
Outdated
| // Parent returns a Name for the parent partition, using the given branching degree. | ||
| func (tn Name) Parent(degree int) Name { | ||
| if tn.IsRoot() || degree == 0 { | ||
| return Name{} |
There was a problem hiding this comment.
should it return itself or an error?
There was a problem hiding this comment.
I do want to return an error, not itself. I changed it to use a shared error value
| // this should never happen when forwardedFrom is empty | ||
| return taskQueue.GetName() | ||
| } | ||
| tqName, err := tqname.FromBaseName(taskQueue.GetName()) |
There was a problem hiding this comment.
I would prefer to leave that for tests only. In other contexts I want an explicit error when the name is not a base name.
| // mangled name. | ||
| func FromBaseName(name string) (Name, error) { | ||
| if strings.HasPrefix(name, mangledTaskQueuePrefix) { | ||
| return Name{}, fmt.Errorf("base name %q must not have prefix %q", name, mangledTaskQueuePrefix) |
common/tqname/tqname.go
Outdated
| // Parent returns a Name for the parent partition, using the given branching degree. | ||
| func (tn Name) Parent(degree int) Name { | ||
| if tn.IsRoot() || degree == 0 { | ||
| return InvalidName |
There was a problem hiding this comment.
I don't see any code checking against the InvalidName sentinel value, and it's not obvious from the API that it should be checked. So I think (Name, bool) or (Name, error) with a sentinel error are better return types here. I also think we should get rid of InvalidName because this is the only non-test use case, and we can panic in other cases because they're initiated by tests.
There was a problem hiding this comment.
Ah, that's a good idea.. the callers check IsRoot() || degree == 0 themselves already, but it makes the callers simpler if we just do it once. Actually I'll just get rid of MustFromBaseName and move it into test files. No reason to clutter the api
What changed?
Add package with task queue name mangling.
Add support for version set ids in mangled task queue names.
Why?
This logic is getting more complicated with versioning so we need to encapsulate it.
How did you test it?
unit tests
Potential risks