ddl: manage job lifecycle in worker #53747
Conversation
ti-chi-bot
bot
added
release-note-none
|
Hi @D3Hunter. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #53747 +/- ##
=================================================
- Coverage 70.8894% 55.4905% -15.3989%
=================================================
Files 1506 1629 +123
Lines 412726 607029 +194303
=================================================
+ Hits 292579 336844 +44265
- Misses 100750 246794 +146044
- Partials 19397 23391 +3994
Flags with carried forward coverage won't be shown. Click here to find out more.
|
pkg/ddl/ddl_worker.go
Outdated
| if err != nil { | ||
| logutil.DDLLogger().Warn("unexpected error when clean mdl info", zap.Int64("job ID", job.ID), zap.Error(err)) | ||
| return | ||
| } | ||
| if cleanETCD && ec != nil { | ||
| // TODO do we need clean it for JobStateRollbackDone? |
There was a problem hiding this comment.
JobStateRollbackDone does trigger schema version changes for some DDLs, if we don't clean keys here, there may be a leak? I can have a test later.
There was a problem hiding this comment.
yes, that's why i want to add a JobStateRollbackSynced, so we can check it easily
| j.updateInternalRunningJobIDs() | ||
|
|
||
| if job.IsFinished() || job.IsSynced() { |
There was a problem hiding this comment.
reorg jobs will check whether there are jobs of same table-id are in-processing in getReorgJob, so for a table-id there shouldn't be 2 jobs running concurrently, so it's ok to delete those unfinishedXXX all the time
There was a problem hiding this comment.
@D3Hunter Is it not necessary to distinguish processingIDs from unfinishedIDs?
There was a problem hiding this comment.
like what we have discussed, runningJobs.remove will only called when the job is in final state like synced/paused/cancelled or it's done by another owner, before that another job operating the same table-id or name should be blocked
|
/hold |
ti-chi-bot
bot
added
the
do-not-merge/hold
| @@ -1776,14 +1777,14 @@ func TestMDLViewIDConflict(t *testing.T) { | |||
| wg := &sync.WaitGroup{} | |||
| wg.Add(1) | |||
| go func() { | |||
| ddlTK1.MustExec("ALTER TABLE t ADD COLUMN b INT;") | |||
| ddlTK1.MustExec("ALTER TABLE t ADD index(a);") | |||
There was a problem hiding this comment.
now the routine will hold worker until it exist, so 2 general ddl cannot be run at same time now while it's possible when they have no dependency previously, so we change to reorg job to make this test work
|
/retest |
|
@D3Hunter: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest |
|
@D3Hunter: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/unhold |
ti-chi-bot
bot
removed
the
do-not-merge/hold
|
@lance6716 @tangenta test pass, ptal |
| @@ -892,13 +893,13 @@ func (r *reorgInfo) UpdateReorgMeta(startKey kv.Key, pool *sess.Pool) (err error | |||
| defer pool.Put(sctx) | |||
|
|
|||
| se := sess.NewSession(sctx) | |||
| err = se.Begin() | |||
| err = se.Begin(context.Background()) | |||
There was a problem hiding this comment.
that's no much difference with background, it's only cancelled at shutdown. we should refactor to use job worker ctx or disttask ctx later
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lance6716, tangenta The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
bot
added
approved
lgtm
and removed
needs-1-more-lgtm
What problem does this PR solve?
Issue Number: ref #53246
Problem Summary:
What changed and how does it work?
backgroundcontexts to DDL worker ctxbegin/commit,rollbackis not changed as it doesn't return err and the main usages of ctx is to extract valueCheck List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.