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

Add table namespace classifier #761

Merged
merged 1 commit into from Sep 19, 2017

Conversation

Projects
None yet
3 participants
@ming-relax
Collaborator

ming-relax commented Sep 18, 2017

Add table namespace classifier

"github.com/juju/errors"
)
var tablePrefix = []byte{'t'}

This comment has been minimized.

@siddontang

siddontang Sep 19, 2017

Member

seem we must support index prefix later too.

@siddontang

siddontang Sep 19, 2017

Member

seem we must support index prefix later too.

@@ -0,0 +1,46 @@
package server

This comment has been minimized.

@siddontang

siddontang Sep 19, 2017

Member

use table_namespace_classifier.go , we don't support CamelCase for the file name.

@siddontang

siddontang Sep 19, 2017

Member

use table_namespace_classifier.go , we don't support CamelCase for the file name.

This comment has been minimized.

@disksing

disksing Sep 19, 2017

Member

Also please add license header.

@disksing

disksing Sep 19, 2017

Member

Also please add license header.

@siddontang

This comment has been minimized.

Show comment
Hide comment
@siddontang

siddontang Sep 19, 2017

Member

Please add test

Member

siddontang commented Sep 19, 2017

Please add test

for name, ns := range classifier.nsInfo.namespaces {
startTable := core.DecodeTableID(regionInfo.StartKey)
endTable := core.DecodeTableID(regionInfo.EndKey)
for _, tableID := range ns.TableIDs {

This comment has been minimized.

@siddontang

siddontang Sep 19, 2017

Member

if we have many tables in one namespace, seem this range check will hurt the performance.
Maybe is it better to use a hash map for TableIDs ?

@siddontang

siddontang Sep 19, 2017

Member

if we have many tables in one namespace, seem this range check will hurt the performance.
Maybe is it better to use a hash map for TableIDs ?

This comment has been minimized.

@disksing

disksing Sep 19, 2017

Member

I think it's ok. Because the number of regions may be a lot, now we always try the best to avoid traversing regions.

@disksing

disksing Sep 19, 2017

Member

I think it's ok. Because the number of regions may be a lot, now we always try the best to avoid traversing regions.

@@ -0,0 +1,46 @@
package server

This comment has been minimized.

@disksing

disksing Sep 19, 2017

Member

Also please add license header.

@disksing

disksing Sep 19, 2017

Member

Also please add license header.

)
type tableNamespaceClassifier struct {
nsInfo *namespacesInfo

This comment has been minimized.

@disksing

disksing Sep 19, 2017

Member

Where is namespaceInfo defined? Forget to submit?

@disksing

disksing Sep 19, 2017

Member

Where is namespaceInfo defined? Forget to submit?

This comment has been minimized.

@ming-relax

ming-relax Sep 19, 2017

Collaborator

It's in another PR #747

Saving/Loading namespace and classifier are separated.

@ming-relax

ming-relax Sep 19, 2017

Collaborator

It's in another PR #747

Saving/Loading namespace and classifier are separated.

This comment has been minimized.

@ming-relax

ming-relax Sep 19, 2017

Collaborator

This PR is supposed to merge into dantin/namespace

@ming-relax

ming-relax Sep 19, 2017

Collaborator

This PR is supposed to merge into dantin/namespace

for name, ns := range classifier.nsInfo.namespaces {
startTable := core.DecodeTableID(regionInfo.StartKey)
endTable := core.DecodeTableID(regionInfo.EndKey)
for _, tableID := range ns.TableIDs {

This comment has been minimized.

@disksing

disksing Sep 19, 2017

Member

I think it's ok. Because the number of regions may be a lot, now we always try the best to avoid traversing regions.

@disksing

disksing Sep 19, 2017

Member

I think it's ok. Because the number of regions may be a lot, now we always try the best to avoid traversing regions.

@ming-relax

This comment has been minimized.

Show comment
Hide comment
@ming-relax

ming-relax Sep 19, 2017

Collaborator

@siddontang @disksing merged this PR to dantin/namespace and will add tests in dantin/namespace, so that you can review all the changes in this PR: #747

Collaborator

ming-relax commented Sep 19, 2017

@siddontang @disksing merged this PR to dantin/namespace and will add tests in dantin/namespace, so that you can review all the changes in this PR: #747

@ming-relax ming-relax merged commit 9afaf2d into dantin/namespace Sep 19, 2017

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
jenkins-ci-pd/build Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

ming-relax added a commit that referenced this pull request Sep 20, 2017

@disksing disksing deleted the feature-namespace-classifier branch Oct 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment