lrs: use JSON for locality's String representation #4135
Conversation
|
Why is "#" better than ":"? It seems the only sure fix would be to escape any pre-existing special characters, or percent-encode, etc. |
Instead of string separated by ":", which can be in the fields.
menghanl
force-pushed
the
LRS_locality_ID_parsing
branch
from
785a6be to
08285c7
Compare
|
Updated to json. |
menghanl
changed the title
menghanl
changed the title
xds/internal/internal.go
Outdated
| return fmt.Sprintf("%s:%s:%s", l.Region, l.Zone, l.SubZone) | ||
| // ToJSON generates a string representation of LocalityID by marshalling it into | ||
| // json. | ||
| func (l LocalityID) ToJSON() (string, error) { |
There was a problem hiding this comment.
Looking at the changes elsewhere...should we just delete this function (and its inverse) and call the json package directly instead? They don't seem to add much.
There was a problem hiding this comment.
But that hardcodes json in the balancers? Using json should be internal to the locality id.
Maybe I should call this function ToString() and FromString() instead of To/From JSON?
There was a problem hiding this comment.
I still don't like calling json.Marshall directly. Because it adds the implicit contract that all locality ID string in the balancers are JSON. And if I were to add a new code to convert a string ID into a struct, I need to know that it's JSON, instead of just calling the function that does the job. And to remind people of that, I would need to add comments to all the fields where ID string is stored, and say it is JSOn.
And there's the minor annoyance that Marshal returns []byte that needs to be explicitly converted into string.
I'm keeping these two functions, but calling them ToString and FromString. Let me know what you think.
There was a problem hiding this comment.
Because it adds the implicit contract
This is internal, so I'm not sure any concerns like this are important.
Let me know what you think.
This is fine, since it's short-lived.
Instead of ":" separated string, as ":" can be in the fields, like
jv:us-central1-a_12345.A long term fix is to wrap the subconns to keep track of its locality.