Skip to content
This repository has been archived by the owner on Nov 24, 2023. It is now read-only.

syncer: refactor sync dml #2061

Merged
merged 62 commits into from
Oct 19, 2021
Merged

syncer: refactor sync dml #2061

merged 62 commits into from
Oct 19, 2021

Conversation

GMHDBJD
Copy link
Collaborator

@GMHDBJD GMHDBJD commented Aug 30, 2021 •
edited
Loading

What problem does this PR solve?

  • do not add jobWg for DML, only use flush jobs rather than s.jobWg.Wait
  • pipeline sync dml. addJob -> causality -> DMLWorker

Test:

  • original test

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Aug 30, 2021 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • Ehco1996
  • lance6716

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.

@GMHDBJD GMHDBJD added the status/WIP This PR is still work in progress label Aug 30, 2021
@GMHDBJD GMHDBJD changed the title sync: refine sync dml syncer: refine sync dml Aug 30, 2021
lance6716
lance6716 reviewed Aug 30, 2021
View reviewed changes
go.mod Outdated Show resolved Hide resolved
@GMHDBJD GMHDBJD changed the title syncer: refine sync dml syncer: refine sync dml [WIP] Aug 30, 2021
@GMHDBJD GMHDBJD mentioned this pull request Oct 18, 2021
Merged
lance6716
lance6716 reviewed Oct 18, 2021
View reviewed changes
syncer/dml_worker.go Outdated Show resolved Hide resolved
syncer/dml_worker.go Show resolved Hide resolved
syncer/dml_worker.go Show resolved Hide resolved
syncer/dml_worker.go Outdated Show resolved Hide resolved
syncer/syncer.go Show resolved Hide resolved
syncer/syncer.go
metrics.AddJobDurationHistogram.WithLabelValues("flush", s.cfg.Name, s.queueBucketMapping[i], s.cfg.SourceID).Observe(time.Since(startTime).Seconds())
}
s.jobWg.Add(1)
s.dmlJobCh <- job
Copy link
Collaborator

Choose a reason for hiding this comment

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

this case branch we handle flushJob came from caller, and in L952 we'll generate flush job inside this function. Can we merge the handle logic to one place such as L960-1016?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Still has some differences. One is flush job and one is ticker(30s). I think separating them is easier to understand. I change the variable name to distinguish them.

lance6716
lance6716 reviewed Oct 19, 2021
View reviewed changes
Copy link
Collaborator

@lance6716 lance6716 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

syncer/syncer.go Show resolved Hide resolved
Ehco1996
Ehco1996 approved these changes Oct 19, 2021
View reviewed changes
Copy link
Contributor

@Ehco1996 Ehco1996 left a comment

Choose a reason for hiding this comment

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

LGTM but please update the PR description (worker pool...)

@ti-chi-bot ti-chi-bot added the status/LGT1 One reviewer already commented LGTM label Oct 19, 2021
@ti-chi-bot ti-chi-bot added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Oct 19, 2021
@GMHDBJD
Copy link
Collaborator Author

GMHDBJD commented Oct 19, 2021

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 87ba1fb

@ti-chi-bot ti-chi-bot merged commit 256afb4 into pingcap:master Oct 19, 2021
ti-chi-bot pushed a commit to ti-chi-bot/dm that referenced this pull request Oct 19, 2021
@GMHDBJD @ti-chi-bot
syncer: refactor sync dml (pingcap#2061)
2460b43
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created: #2237.

@ti-chi-bot ti-chi-bot mentioned this pull request Oct 19, 2021
Merged
ti-chi-bot added a commit that referenced this pull request Oct 19, 2021
@ti-chi-bot
syncer: refactor sync dml (#2061) (#2237)
4d21188
@GMHDBJD GMHDBJD mentioned this pull request Nov 12, 2021
Merged
@ti-chi-bot ti-chi-bot mentioned this pull request Nov 24, 2021
Closed
@ti-chi-bot ti-chi-bot mentioned this pull request Dec 27, 2021
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@glorv glorv glorv left review comments

@Ehco1996 Ehco1996 Ehco1996 approved these changes

@lance6716 lance6716 lance6716 approved these changes

Assignees
No one assigned
Labels
needs-cherry-pick-release-2.0 This PR should be cherry-picked to release-2.0. Remove this label after cherry-picked to release-2.0 needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated size/XXL status/can-merge status/LGT2 Two reviewers already commented LGTM, ready for merge status/PTAL This PR is ready for review. Add this label back after committing new changes
Projects
None yet
Milestone
v2.1.0
Development

Successfully merging this pull request may close these issues.
5 participants
@GMHDBJD @ti-chi-bot @lance6716 @glorv @Ehco1996