-
Notifications
You must be signed in to change notification settings - Fork 726
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
*: fix table namespace classifier. #808
Conversation
disksing
commented
Oct 19, 2017
- GetAllNamespaces should contain DefaultNamespace, or the stores in default namespace will not be balanced.
- _ IsPureTableID_ need decode the key before check if it is table prefix.
table/codec.go
Outdated
@@ -84,7 +84,11 @@ func decodeCmpUintToInt(u uint64) int64 { | |||
|
|||
// IsPureTableID return true iff b is consist of tablePrefix and 8-byte tableID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/iff/if
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rest LGtM
@@ -82,9 +82,13 @@ func decodeCmpUintToInt(u uint64) int64 { | |||
return int64(u ^ signMask) | |||
} | |||
|
|||
// IsPureTableID return true iff b is consist of tablePrefix and 8-byte tableID | |||
// IsPureTableID returns true if b is consist of tablePrefix and 8-byte tableID | |||
func IsPureTableID(b []byte) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add test
@@ -123,6 +123,7 @@ func (c *tableNamespaceClassifier) GetAllNamespaces() []string { | |||
for name := range c.nsInfo.namespaces { | |||
nsList = append(nsList, name) | |||
} | |||
nsList = append(nsList, namespace.DefaultNamespace) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to check duplicated default here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe forbid creating namespace using global
instead.
LGTM |