Add support for colon separator of keyspace:shard#2685
Add support for colon separator of keyspace:shard#2685michael-berlin merged 4 commits intovitessio:masterfrom
Conversation
go/vt/topo/topoproto/shard.go
Outdated
| } | ||
|
|
||
| func parseKeyspaceShard(param string) []string { | ||
| rp := regexp.MustCompile("[:/]") |
There was a problem hiding this comment.
regexp is probably overkill for this. Given that this will be a high QPS call, prefer using https://golang.org/pkg/strings/#LastIndexAny instead.
|
We'll wait for @michael-berlin to look at this also. |
|
Thanks, I changed to LastIndexAny. I also removed the test which fails on a path with multiple slashes, because with LastIndexAny it's harder to determine that. Not sure how strict you want to be about that case. I can add it back and go a different route if you'd like |
…appropriately. Update tests
|
@michael-berlin @sougou updated per conversation in slack |
michael-berlin
left a comment
michael-berlin
left a comment
There was a problem hiding this comment.
LGTM after my two comments are addressed.
go/vt/vtgate/vtgate.go
Outdated
| // parseKeyspaceOptionalShard parses a "keyspace/shard" or "keyspace:shard" string | ||
| // and extracts the parts. If a shard is not specified, it's | ||
| // returned as empty string. We need to support : and / in vtgate because some clients | ||
| // can't support our default of slash. Everywhere else we only support / |
There was a problem hiding this comment.
nit: Missing period in the last sentence after /.
go/vt/vtgate/vtgate_test.go
Outdated
| keyspace: "ks", | ||
| shard: "", | ||
| }, { | ||
| keyspaceShard: "/-80", |
There was a problem hiding this comment.
I suggest that we remove the test cases where one of the parts is empty.
IMO this is outside of the "specification" and therefore the behaviour for such an input should not be defined. But by having it in the test, we're implicitly defining it and making it part of our "specification". For example, we may consider to error on this input in the future.
Therefore, I prefer if you delete this case and the cases ks/, ks: and :-80.
|
@michael-berlin fixed up, thanks |
|
Thanks! :) |
4 participants
@sougou @michael-berlin