-
Notifications
You must be signed in to change notification settings - Fork 717
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
namespace isolation: http interface and classifier #747
Conversation
dantin
commented
Sep 12, 2017
•
edited by ming535
Loading
edited by ming535
- Configure namespace related info through pd-ctl/api
- Add table_namespace_classifier to return namespace name for region/store based on the namespace configuration
Hi contributor, thanks for your PR. This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically. |
0fa7d11
to
e6fb6f8
Compare
a8efc47
to
1199eda
Compare
pdctl/command/namespace_command.go
Outdated
"github.com/spf13/cobra" | ||
"net/http" | ||
"strconv" | ||
) |
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 3rd-part packages from standard packages and leave a blank line between them. We prefer the default output of goimports
tool.
pdctl/command/namespace_command.go
Outdated
func NewAppendTableIDCommand() *cobra.Command { | ||
c := &cobra.Command{ | ||
Use: "append <name> <table_id>", | ||
Short: "add table id in namespace", |
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.
in -> to.
pdctl/command/namespace_command.go
Outdated
return | ||
} | ||
if _, err := strconv.Atoi(args[1]); err != nil { | ||
fmt.Println("table_id shoud be a number") |
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.
Remove extra space.
server/api/namespace.go
Outdated
|
||
// Get list namespace mapping | ||
func (h *namespaceHandler) Get(w http.ResponseWriter, r *http.Request) { | ||
//TODO |
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.
Remove redundant line.
server/api/namespace.go
Outdated
} | ||
} | ||
|
||
// Get list namespace mapping |
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.
list -> lists.
@@ -0,0 +1,101 @@ | |||
// Copyright 2016 PingCAP, Inc. |
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.
2016->2017.
server/table_namespace_classifier.go
Outdated
func (c tableNamespaceClassifier) GetRegionNamespace(regionInfo *core.RegionInfo) string { | ||
for name, ns := range c.nsInfo.namespaces { | ||
startTable := c.tableIDDecoder.DecodeTableID(regionInfo.StartKey) | ||
endTable := c.tableIDDecoder.DecodeTableID(regionInfo.EndKey) |
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.
We can move the 2 lines out of for loop. Also we can check if start == end before the loop, if they are not equal, we can return defaultNamespace directly.
server/table_namespace_classifier.go
Outdated
} | ||
|
||
func (c tableNamespaceClassifier) GetAllNamespaces() []string { | ||
nsList := []string{} |
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.
nsList := make([]string, 0, len(c.nsInfo.namespaces))
server/core/tablecodec.go
Outdated
"encoding/binary" | ||
"github.com/juju/errors" | ||
) | ||
|
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.
I guess we can add some references to the table codec implementation on tidb.
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.
@siddontang any comments on this?
server/core/namespaces.go
Outdated
// 1. relation between a name and several tables | ||
// 2. relation between a name and several stores | ||
// It is used to bind tables with stores | ||
type Namespace struct { |
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.
How about moving it to server/table_namespace_classifier.go
? Since it is only used by server
and server/api
package.
8b018e2
to
ab84e88
Compare
ede8762
to
ae256aa
Compare
ba0b634
to
acdf062
Compare
acdf062
to
fb96f63
Compare
// 1. relation between a Name and several tables | ||
// 2. relation between a Name and several stores | ||
// It is used to bind tables with stores | ||
type Namespace struct { |
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.
Seem it is TableNamespace, not a Namespace.
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.
Will there be other namespace? How about change table_namespace_classifier.go
to namespace_classifier.go
?
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.
We may have other namespace implementation later. So we can't assume there is only one table namespace.
} | ||
} | ||
|
||
func (c tableNamespaceClassifier) GetAllNamespaces() []string { |
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.
Will we call this function frequently?
/cc @disksing
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.
Now it is called every time we call scheduler.Schedule()
.
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.
Seem this is not effective.
|
||
type defaultTableIDDecoder struct{} | ||
|
||
var tablePrefix = []byte{'t'} |
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.
seem we should also consider table index.
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.
Just table prefix is ok. We have no plan to separate rows and indexes into different namespaces.
d7622b1
to
7d855fb
Compare
* new GetRegionNamespace Method, checking both StartKey and EndKey * go fmt server/table_namespace_classifier.go * go fmt server/core/tablecodec.go
LGTM. |
PTAL @nolouch @siddontang |
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.
LGTM