Skip to content
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

ddl: Deal with discrete handles #5102

Merged
merged 9 commits into from Nov 20, 2017
Merged

Conversation

zimulala
Copy link
Contributor

@zimulala zimulala commented Nov 14, 2017

The test has been added to the function of TestAddIndex.

ddl/index.go Outdated
defaultTaskHandleCnt = 128
maxTaskHandleCnt = 2048
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is too small.

@coocood
Copy link
Member

coocood commented Nov 14, 2017

We should also decrease the batch size if the actual count is too large.

@zimulala
Copy link
Contributor Author

@coocood @shenli
If the transaction doesn't fail(because of writing conflict), I think we needn't decrease the batch size although the actual count is too large. If the transaction is failed, we will decrease.

@coocood
Copy link
Member

coocood commented Nov 15, 2017

@zimulala
We don't need to decrease if the max is 2048, but If we support decreasing, we can set the max to very large, like 1000000.

@coocood
Copy link
Member

coocood commented Nov 15, 2017

The threshold to decrease can be 1024 actual keys, but the handle range can be as large as 1 million.

@shenli
Copy link
Member

shenli commented Nov 15, 2017

@coocood PTAL

ddl/index.go Outdated
@@ -525,6 +529,13 @@ type handleInfo struct {
endHandle int64
}

func chooseInt64(baseHandle, batch int64) int64 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename it to getEndHandle is better.

ddl/index.go Outdated
}
}

func getCountAndHandle(workers []*worker) (int64, int64, bool, error) {
taskAddedCount, nextHandle := int64(0), workers[0].taskRange.startHandle
var err error
var isEnd bool
lessMinWorkers := 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/lessMinWorkers/starvingWorkers/

nextHandle = ret.outOfRangeHandle
isEnd = ret.isAllDone
}

// Adjust the worker's batch size.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add more comment here to describe why we should increase the batch size. What is the problem we are trying to solve.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.
PTAL @shenli

ddl/index.go Outdated
defaultTaskHandleCnt = 128
maxTaskHandleCnt = 1 << 20
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add comment here.

@shenli
Copy link
Member

shenli commented Nov 20, 2017

LGTM
@winkyao @coocood PTAL

@coocood
Copy link
Member

coocood commented Nov 20, 2017

LGTM

@coocood coocood added the status/LGT2 Indicates that a PR has LGTM 2. label Nov 20, 2017
coocood
coocood previously approved these changes Nov 20, 2017
@coocood
Copy link
Member

coocood commented Nov 20, 2017

/run-all-tests

@coocood
Copy link
Member

coocood commented Nov 20, 2017

/run-unit-test

@zimulala zimulala merged commit a220cff into pingcap:master Nov 20, 2017
@zimulala zimulala deleted the discrete-index branch November 20, 2017 03:30
zimulala added a commit to zimulala/tidb that referenced this pull request Nov 20, 2017
* ddl: deal with discrete handles
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants