balancer: support hierarchical paths in addresses #3494
Conversation
balancer/base.SplitHierarchicalAddresses
balancer/base/hierarchy/hierarchy.go
Outdated
| */ | ||
|
|
||
| // Package hierarchy contains functions to set and get hierarchy string from | ||
| // addresses. |
There was a problem hiding this comment.
// Experimental
(or in an internal directory)
There was a problem hiding this comment.
Did both.
Moved to internal.
Does this even belong to balancer? This is modifying resolver.Address, and can be set by a resolver. We need a bar package.
balancer/base/hierarchy/hierarchy.go
Outdated
|
|
||
| // Package hierarchy contains functions to set and get hierarchy string from | ||
| // addresses. | ||
| package hierarchy |
There was a problem hiding this comment.
Does this package logically belong under base? I feel like it's its own thing under balancer, but not strongly.
There was a problem hiding this comment.
See the other comment. Maybe not even balancer?
balancer/base/hierarchy/hierarchy.go
Outdated
| } | ||
|
|
||
| // Set overrides the hierarchical path in addr with path. | ||
| func Set(addr resolver.Address, path []string) resolver.Address { |
There was a problem hiding this comment.
What do you think about accepting a pointer and modifing in place?
There was a problem hiding this comment.
Copying is cleaner to use?
// Copy
for _, addr := range addrs {
newAddrs = append(newAddrs, Set(addr, ["a","b"]))
}
// Update
for _, addr := range addrs {
// Hmm, is the local variable `addr` already a copy?
Set(&addr, ["a","b"])
newAddrs = append(newAddrs, addr)
}
// With update, or even do everything in place?
for i, addr := range addrs {
// Does this work?? addr is a copy?
Set(&addr, ["a","b"]
// Does this work?
Set(&(addrs[i]), ["a","b"])
}Another question is, what should Group() do? Should it modify the passed in addresses, or copy?
internal/hierarchy.Group()