-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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: dynamic adjust add index worker number. #8295
ddl: dynamic adjust add index worker number. #8295
Conversation
604526b
to
f936c59
Compare
…add-index-worker
/run-all-tests |
/run-all-tests |
/run-integration-ddl-test |
1 similar comment
/run-integration-ddl-test |
@crazycs520 Please find out why integration-ddl-test failed, https://github.com/pingcap/tidb-test/tree/master/ddl_test |
ddl/db_test.go
Outdated
} | ||
if startReorganization { | ||
workerCnt := rand.Intn(8) + 8 | ||
s.mustExec(c, fmt.Sprintf("set @@tidb_ddl_reorg_worker_cnt=%d", workerCnt)) |
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 can we ensure the worker count is changed to workerCnt
?
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.
em... I check the log when do manual test to know the worker count is changed... 😂
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.
main changes LGTM
ddl/index.go
Outdated
} | ||
|
||
reorgInfo.d.mu.Lock() | ||
reorgInfo.d.mu.hook.OnIndexWorkerReorgBefore(len(idxWorkers), len(kvRanges)) |
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.
Could we use gofail
instead of this logic?
ddl/db_test.go
Outdated
@@ -630,8 +680,16 @@ LOOP: | |||
s.mustExec(c, sql) | |||
} | |||
num += step | |||
case <-ticker2.C: | |||
if changeWorkerNumEnable { |
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.
It will have data race
.
…add-index-worker
0befa35
to
d86e5b4
Compare
d86e5b4
to
77f49bd
Compare
/run-all-tests |
…add-index-worker
/run-all-tests |
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
ddl/db_test.go
Outdated
s.tk.MustExec("drop table test_add_index") | ||
} | ||
|
||
func (s *testDBSuite) TestAddIndexWorkerNum(c *C) { |
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 put it to failtest/fail_db_test.go
fcd092d
to
50470ad
Compare
/run-all-tests |
1 similar comment
/run-all-tests |
…add-index-worker
a2d1db9
to
fc6674e
Compare
/run-all-tests |
|
||
// gofail: var checkIndexWorkerNum bool | ||
//if checkIndexWorkerNum { | ||
// num := int(atomic.LoadInt32(&TestCheckWorkerNumber)) |
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 we don't need atomic.LoadInt3
.
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.
Use to avoid data race.
/run-all-tests |
What problem does this PR solve?
Currently, if tidb is doing a add index ddl job, then the client change the
tidb_ddl_reorg_worker_cnt
want to speed up add index, but this change will take effect next add index, won't affect current add index job.What is changed and how it works?
Check List
Tests
Code changes
Side effects
Related changes
This change is