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.

Sign up for GitHub

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

Jump to bottom

ddl: support referring objects in runningJobs #54110

Merged
merged 12 commits into from
Jun 24, 2024
Merged

ddl: support referring objects in runningJobs #54110

merged 12 commits into from
Jun 24, 2024

Conversation

lance6716
Copy link
Contributor

@lance6716 lance6716 commented Jun 19, 2024 •
edited
Loading

What problem does this PR solve?

Issue Number: close #52795, ref #53246

Problem Summary:

What changed and how does it work?

a DDL job will modify and refer to multiple objects (database, table, placement policy, resource group). Treat it like the job will lock these objects, in exclusive or shared lock mode. If the job finished all locking, then it can run now. In other words, the job has no dependency on other running jobs. This PR implements this lock-like behaviour.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No need to test
    • I checked and no code files have been changed.

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

@ti-chi-bot ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 19, 2024
@tiprow tiprow
Copy link

tiprow bot commented Jun 19, 2024

Hi @lance6716. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

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 Codecov
Copy link

codecov bot commented Jun 19, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 87.21228% with 50 lines in your changes missing coverage. Please review.

Project coverage is 56.0921%. Comparing base (51ccce2) to head (f14339b).
Report is 30 commits behind head on master.

Additional details and impacted files 
@@                Coverage Diff                @@
##             master     #54110         +/-   ##
=================================================
- Coverage   72.8518%   56.0921%   -16.7597%     
=================================================
  Files          1516       1639        +123     
  Lines        434806     607452     +172646     
=================================================
+ Hits         316764     340733      +23969     
- Misses        98460     243420     +144960     
- Partials      19582      23299       +3717
Flag Coverage Δ
integration 37.0972% <69.8209%> (?)
unit 71.7804% <87.2122%> (-0.0667%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
dumpling 52.9656% <ø> (ø)
parser ∅ <ø> (∅)
br 52.6041% <ø> (+6.4848%) ⬆️

@lance6716
finish!
b4f5a09 
Signed-off-by: lance6716 <[email protected]>
@lance6716 lance6716 changed the title [WIP]ddl: support shared objects in runningJobs ddl: support shared objects in runningJobs Jun 19, 2024
@ti-chi-bot ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 19, 2024
@lance6716 lance6716 changed the title ddl: support shared objects in runningJobs ddl: support referring objects in runningJobs Jun 19, 2024
@lance6716
Copy link
Contributor Author

/cc @tangenta @D3Hunter

@ti-chi-bot ti-chi-bot bot requested review from D3Hunter and tangenta June 19, 2024 12:56
lance6716 added 2 commits June 19, 2024 20:59
@lance6716
fix lint
9c87627 
Signed-off-by: lance6716 <[email protected]>
@lance6716
fix fmt
1317631 
Signed-off-by: lance6716 <[email protected]>
@lance6716
Copy link
Contributor Author

/retest

@tiprow tiprow
Copy link

tiprow bot commented Jun 19, 2024

@lance6716: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

@lance6716 lance6716 changed the title ddl: support referring objects in runningJobs [WIP]ddl: support referring objects in runningJobs Jun 19, 2024
@ti-chi-bot ti-chi-bot bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 19, 2024
@lance6716
fix UT
393b9ff 
Signed-off-by: lance6716 <[email protected]>
@lance6716 lance6716 changed the title [WIP]ddl: support referring objects in runningJobs ddl: support referring objects in runningJobs Jun 19, 2024
@ti-chi-bot ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 19, 2024
lance6716 added 2 commits June 19, 2024 21:37
@lance6716
reorder
6ec10d2 
Signed-off-by: lance6716 <[email protected]>
@lance6716
fix CI
ada7ad5 
Signed-off-by: lance6716 <[email protected]>
D3Hunter
D3Hunter reviewed Jun 20, 2024
View reviewed changes
Copy link
Contributor

@D3Hunter D3Hunter left a comment

Choose a reason for hiding this comment

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

9/14

pkg/ddl/ddl_api.go Outdated Show resolved Hide resolved
@D3Hunter D3Hunter mentioned this pull request Jun 20, 2024
Merged
13 tasks
D3Hunter
D3Hunter reviewed Jun 20, 2024
View reviewed changes
Copy link
Contributor

@D3Hunter D3Hunter left a comment

Choose a reason for hiding this comment

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

11/14

pkg/parser/model/ddl.go Outdated Show resolved Hide resolved
pkg/parser/model/ddl.go Outdated Show resolved Hide resolved
pkg/parser/model/ddl.go Outdated Show resolved Hide resolved
D3Hunter
D3Hunter reviewed Jun 20, 2024
View reviewed changes
pkg/parser/model/ddl.go Outdated Show resolved Hide resolved
D3Hunter
D3Hunter reviewed Jun 20, 2024
View reviewed changes
pkg/ddl/job_table.go
@@ -235,6 +240,7 @@ func (s *jobScheduler) getJob(se *sess.Session, tp jobType) (*model.Job, error)
"[ddl] handle ddl job failed: mark job is processing meet error",
zap.Error(err),
zap.Stringer("job", &job))
s.runningJobs.addPending(involving)
Copy link
Contributor

Choose a reason for hiding this comment

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

we can remove this markJobProcessing completely later, memory check is enough, i will record a task item

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need more reviewers to careful think about that, if it's essential to persist the result of which job is chosen as running.

pkg/ddl/job_table.go
@@ -184,7 +184,10 @@ func (s *jobScheduler) close() {
}
}

// getJob reads tidb_ddl_job and returns the first runnable DDL job.
func (s *jobScheduler) getJob(se *sess.Session, tp jobType) (*model.Job, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

we need merge 2 loadDDLJobAndRun into 1 to hand a 'fair' case below:

  • job 1: add column to test.t
  • job 2: truncate test.t: skipped scan since job 1 is running
  • job 3: add index to test.t: before we check it, job 1 finished, so it can be run, but it should be run after job 2

since it's already a large pr, i add a task item to track and we can change it later, and can do it together with remove the sub-query inside getJobSQL to speedup.

@lance6716
address comment
df425d6 
Signed-off-by: lance6716 <[email protected]>
lance6716 added 2 commits June 20, 2024 14:06
@lance6716
fix lint
9c52e13 
Signed-off-by: lance6716 <[email protected]>
@lance6716
change GetInvolvingSchemaInfo
0ae0949 
Signed-off-by: lance6716 <[email protected]>
@lance6716
Copy link
Contributor Author

/retest

@tiprow tiprow
Copy link

tiprow bot commented Jun 20, 2024

@lance6716: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

@lance6716
Copy link
Contributor Author

/retest

@tiprow tiprow
Copy link

tiprow bot commented Jun 20, 2024

@lance6716: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

D3Hunter
D3Hunter reviewed Jun 20, 2024
View reviewed changes
Copy link
Contributor

@D3Hunter D3Hunter left a comment

Choose a reason for hiding this comment

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

11/13

pkg/ddl/multi_schema_change.go Show resolved Hide resolved
pkg/ddl/ddl_api.go Outdated Show resolved Hide resolved
pkg/ddl/ddl_api.go Outdated Show resolved Hide resolved
pkg/parser/model/ddl.go Outdated Show resolved Hide resolved
pkg/ddl/ddl_running_jobs_test.go Outdated Show resolved Hide resolved
pkg/ddl/ddl_running_jobs.go Outdated Show resolved Hide resolved
pkg/ddl/ddl_running_jobs.go Show resolved Hide resolved
pkg/ddl/ddl_running_jobs.go Outdated Show resolved Hide resolved
pkg/ddl/ddl_running_jobs.go Outdated Show resolved Hide resolved
pkg/ddl/ddl_running_jobs.go Outdated Show resolved Hide resolved
@lance6716
address comment
13a2e57 
Signed-off-by: lance6716 <[email protected]>
@D3Hunter D3Hunter mentioned this pull request Jun 20, 2024
Closed
18 tasks
D3Hunter
D3Hunter reviewed Jun 21, 2024
View reviewed changes
Copy link
Contributor

@D3Hunter D3Hunter left a comment

Choose a reason for hiding this comment

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

rest lgtm

pkg/ddl/ddl_api.go Show resolved Hide resolved
@lance6716
address comment
f14339b 
Signed-off-by: lance6716 <[email protected]>
@ti-chi-bot ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jun 21, 2024
@lance6716
Copy link
Contributor Author

ping @tangenta

@ti-chi-bot ti-chi-bot
Copy link

ti-chi-bot bot commented Jun 24, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: D3Hunter, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot added lgtm approved and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jun 24, 2024
@ti-chi-bot ti-chi-bot
Copy link

ti-chi-bot bot commented Jun 24, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-06-21 06:06:55.050745499 +0000 UTC m=+354141.536234329: ☑️ agreed by D3Hunter.
  • 2024-06-24 05:54:59.247875376 +0000 UTC m=+612625.733364203: ☑️ agreed by tangenta.

@ti-chi-bot ti-chi-bot bot merged commit 324ee4c into pingcap:master Jun 24, 2024
23 checks passed
@D3Hunter D3Hunter mentioned this pull request Jul 4, 2024
Open
54 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tangenta tangenta tangenta approved these changes

@D3Hunter D3Hunter D3Hunter approved these changes

Assignees
No one assigned
Labels
approved lgtm release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

some schema level DDLs are not considered in dependency check
3 participants
@lance6716 @D3Hunter @tangenta