-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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: delete ranges when rolling back 'add index' #6188
Conversation
/run-all-tests |
ddl/ddl_db_test.go
Outdated
@@ -323,6 +323,7 @@ func (s *testDBSuite) TestCancelAddIndex(c *C) { | |||
} | |||
|
|||
var checkErr error | |||
var idxInfo *model.IndexInfo |
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.
c3IdxInfo
is more readable.
ddl/ddl_worker.go
Outdated
break | ||
} | ||
// ActionAddIndex needs to delete ranges when it needs to be rolled back. | ||
fallthrough |
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.
fallthrough
is rarely used, it's hard to read.
How about extracting a function shouldDeleteRange
?
ddl/index.go
Outdated
@@ -395,6 +395,7 @@ func (d *ddl) onDropIndex(t *meta.Meta, job *model.Job) (ver int64, _ error) { | |||
job.FinishTableJob(model.JobStateDone, model.StateNone, ver, tblInfo) | |||
} | |||
job.Args = append(job.Args, indexInfo.ID) | |||
log.Warnf("args %v, ts %v", job.Args, t.StartTS) |
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 the warn
log.
PTAL @coocood |
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
ddl/ddl_worker.go
Outdated
} | ||
// ActionAddIndex needs to delete ranges when it needs to be rolled back. |
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.
After rolling back an AddIndex operation, we need to use delete-range to delete the half-done index data.
PTAL @shenli |
LGTM |
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
We need to delete ranges when the operation of 'add index' needs to be rolled back.