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

lightning: refine write and ingest logic more like producer-consumer #40692

Merged
merged 41 commits into from
Feb 27, 2023
Merged

lightning: refine write and ingest logic more like producer-consumer #40692

merged 41 commits into from
Feb 27, 2023

Conversation

lance6716
Copy link
Contributor

@lance6716 lance6716 commented Jan 18, 2023 •
edited
Loading

What problem does this PR solve?

Issue Number: close #40725

Problem Summary:

What is changed and how it works?

propose regionJob which is dedicated to do data importing on a region. Add worker to handle regionJob and let producer to generate regionJob.

Check List

Tests

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

@lance6716
--wip-- [skip ci]
cabcb29
@lance6716
sync code
2e71066 
Signed-off-by: lance6716 <[email protected]>
@lance6716
save work
4047783 
Signed-off-by: lance6716 <[email protected]>
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Jan 18, 2023 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • D3Hunter
  • gozssky

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added do-not-merge/needs-linked-issue do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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. labels Jan 18, 2023
hawkingrei and others added 7 commits January 18, 2023 15:08
@hawkingrei
Merge branch 'master' into region-job
a892b56
@lance6716
save work
086ff97 
Signed-off-by: lance6716 <[email protected]>
@lance6716
revert mis-rename
c7d3e63 
Signed-off-by: lance6716 <[email protected]>
@lance6716
revert more
2310879 
Signed-off-by: lance6716 <[email protected]>
@lance6716
save work
e5ecb07 
Signed-off-by: lance6716 <[email protected]>
@lance6716
almost finished
b7bb3c2 
Signed-off-by: lance6716 <[email protected]>
@lance6716
fix make bazel
0782c3f 
Signed-off-by: lance6716 <[email protected]>
sleepymole
sleepymole reviewed Jan 19, 2023
View reviewed changes
br/pkg/lightning/backend/local/local.go Outdated
}
}

err := local.writeAndIngestPairs(ctx, job)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not move this logic to the job?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now I want the job to provide single actions, so we may extend more action like scanRegion in future and the worker can call single action like a state machine

Copy link
Contributor

Choose a reason for hiding this comment

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

How about abstracting a worker struct and moving all this logic to the worker?

Copy link
Contributor

Choose a reason for hiding this comment

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

extend more action like scanRegion in future

can make job a interface, worker can just call job.execute(ctx)

Copy link
Contributor

Choose a reason for hiding this comment

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

How about abstracting a worker struct and moving all this logic to the worker?

What about this? I think a struct like jobWorker is more clear.

Copy link
Contributor Author

@lance6716 lance6716 Feb 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

That require a lof of work, I prefer we finish this PR timely and leave it for future work.

we can do it when integrate engine/backend with LOAD DATA/DDL.

br/pkg/lightning/backend/local/local.go Outdated Show resolved Hide resolved
br/pkg/lightning/backend/local/local.go Outdated Show resolved Hide resolved
@lance6716
fix more
1ce72c8 
Signed-off-by: lance6716 <[email protected]>
@lance6716
address comments
d029015 
Signed-off-by: lance6716 <[email protected]>
@lance6716
Copy link
Contributor Author

/run-integration-br-tests

@lance6716
save work
36bedcb 
Signed-off-by: lance6716 <[email protected]>
@lance6716

This comment was marked as outdated.

Sign in to view
@lance6716
fix UT
1621953 
Signed-off-by: lance6716 <[email protected]>
@lance6716
fix UT
df937f9 
Signed-off-by: lance6716 <[email protected]>
@lance6716
fix IT
e147bbe 
Signed-off-by: lance6716 <[email protected]>
@lance6716
fix IT
1aefe14 
Signed-off-by: lance6716 <[email protected]>
@lance6716
Copy link
Contributor Author

/run-integration-br-tests

@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 23, 2023
lichunzhu
lichunzhu reviewed Feb 23, 2023
View reviewed changes
Copy link
Contributor

@lichunzhu lichunzhu left a comment

Choose a reason for hiding this comment

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

Will review later

br/pkg/lightning/backend/local/region_job.go Show resolved Hide resolved
br/pkg/lightning/backend/local/local.go Outdated Show resolved Hide resolved
@lance6716
make bazel_prepare
3ab75c2 
Signed-off-by: lance6716 <[email protected]>
@lance6716
address comment
2c097f3 
Signed-off-by: lance6716 <[email protected]>
@lance6716
fix a bug that batch is used twice
5235648 
Signed-off-by: lance6716 <[email protected]>
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 27, 2023
@lance6716 lance6716 requested a review from sleepymole February 27, 2023 03:00
sleepymole
sleepymole reviewed Feb 27, 2023
View reviewed changes
Copy link
Contributor

@sleepymole sleepymole left a comment

Choose a reason for hiding this comment

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

Please address existing comments. Almost LGTM.

@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Feb 27, 2023
@sleepymole sleepymole added the component/lightning This issue is related to Lightning of TiDB. label Feb 27, 2023
@sleepymole
Copy link
Contributor

/run-integration-br-test

@lance6716
Copy link
Contributor Author

br test is passed https://ci.pingcap.net/blue/organizations/jenkins/tidb_ghpr_integration_br_test/detail/tidb_ghpr_integration_br_test/12151/pipeline

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: 7ceb901

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 27, 2023
@ti-chi-bot ti-chi-bot merged commit 7aedf31 into pingcap:master Feb 27, 2023
@lance6716 lance6716 mentioned this pull request Mar 31, 2023
Closed
@lance6716 lance6716 mentioned this pull request Aug 15, 2023
Closed
@lance6716 lance6716 mentioned this pull request Sep 26, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lichunzhu lichunzhu lichunzhu left review comments

@D3Hunter D3Hunter D3Hunter approved these changes

@sleepymole sleepymole sleepymole approved these changes

@GMHDBJD GMHDBJD Awaiting requested review from GMHDBJD

Assignees
No one assigned
Labels
component/lightning This issue is related to Lightning of TiDB. 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. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

lightning local backend has too many levels to do the real job
6 participants
@lance6716 @ti-chi-bot @sleepymole @D3Hunter @lichunzhu @hawkingrei