Skip to content

Conversation

@zeminzhou
Copy link
Contributor

Signed-off-by: zeminzhou zhouzemin@ping.com

What problem does this PR solve?

Keyspans to add & keyspans to remove, must be overlapped. So to be correct, I think we must finish remove jobs first, and then dispatch the add jobs.here

Issue Number: to #48

Problem Description:

Keyspans to add & keyspans to remove, must be overlapped. So to be correct, I think we must finish remove jobs first, and then dispatch the add jobs

What is changed and how does it work?

After each acquisition of the current keyspan, compare the current keyspan with the keyspan of the previous period to establish a corresponding relationship newKeySpan -> needRemoveKeySpan. The running and deployment of the newKeySpan task needs to wait for all the needRemoveKeySpan tasks to stop. This check will be done in Processor.

Code changes

  • Has exported function/method change
  • Has exported variable/fields change
  • Has persistent data change

Signed-off-by: zeminzhou <zhouzemin@ping.com>
@zeminzhou
Copy link
Contributor Author

TODO: Unit Test.

@zeminzhou
Copy link
Contributor Author

@pingyu @zz-jason PTAL~

@zeminzhou zeminzhou requested review from pingyu and zz-jason April 20, 2022 12:17
@zeminzhou zeminzhou changed the title [to #48] try fix scheduler problem [to #48] try to fix scheduler problem Apr 21, 2022
zeminzhou added 4 commits April 21, 2022 11:57
Signed-off-by: zeminzhou <zhouzemin@ping.com>
Signed-off-by: zeminzhou <zhouzemin@ping.com>
Signed-off-by: zeminzhou <zhouzemin@ping.com>
Signed-off-by: zeminzhou <zhouzemin@ping.com>
@codecov
Copy link

codecov bot commented Apr 25, 2022

Codecov Report

Merging #90 (d8e6b55) into main (48a3c74) will increase coverage by 7.8239%.
The diff coverage is 90.3225%.

❗ Current head d8e6b55 differs from pull request most recent head ea39b9d. Consider uploading reports for the commit ea39b9d to get more accurate results

Impacted file tree graph

@@               Coverage Diff                @@
##               main        #90        +/-   ##
================================================
+ Coverage   53.4580%   61.2819%   +7.8239%     
================================================
  Files           197        180        -17     
  Lines         19838      14275      -5563     
================================================
- Hits          10605       8748      -1857     
+ Misses         8265       4910      -3355     
+ Partials        968        617       -351     
Flag Coverage Δ
cdc 61.2819% <90.3225%> (+7.8239%) ⬆️

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

Impacted Files Coverage Δ
cdc/cdc/model/mounter.go 100.0000% <ø> (ø)
cdc/cdc/processor/pipeline/sink.go 62.9629% <ø> (ø)
cdc/cdc/sink/black_hole.go 81.4814% <ø> (+8.1481%) ⬆️
cdc/cdc/sink/sink.go 64.2857% <ø> (ø)
cdc/cdc/sink/tikv.go 27.0270% <ø> (+0.5719%) ⬆️
cdc/pkg/scheduler/workload.go 75.4098% <ø> (ø)
cdc/cdc/owner/scheduler.go 76.0869% <40.0000%> (-0.5798%) ⬇️
cdc/cdc/model/owner.go 80.3681% <100.0000%> (-6.6690%) ⬇️
cdc/cdc/owner/changefeed.go 68.8172% <100.0000%> (+68.8171%) ⬆️
cdc/cdc/owner/scheduler_v1.go 71.4765% <100.0000%> (+71.4765%) ⬆️
... and 15 more

return shouldUpdateState, nil
}

func (s *oldScheduler) diffCurrentKeySpans(currentKeySpans map[model.KeySpanID]regionspan.Span) (map[model.KeySpanID]struct{}, []model.KeySpanID) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add unit test for this method.

// syncKeySpansWithCurrentKeySpans iterates all current keyspans to check whether it should be listened or not.
// this function will return schedulerJob to make sure all keyspans will be listened.
func (s *oldScheduler) syncKeySpansWithCurrentKeySpans() ([]*schedulerJob, error) {
func (s *oldScheduler) syncKeySpansWithCurrentKeySpans(newKeySpans map[model.KeySpanID]struct{}, needRemoveKeySpans []model.KeySpanID) ([]*schedulerJob, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest to add unit test for this method.

return nil
}

func (p *processor) checkRelatedKeyspans(relatedKeySpans []model.KeySpanLocation) bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add unit test for this method.

zeminzhou and others added 9 commits May 10, 2022 13:22
Signed-off-by: zeminzhou <zhouzemin@ping.com>
Signed-off-by: zeminzhou <zhouzemin@ping.com>
Signed-off-by: zeminzhou <zhouzemin@ping.com>
Signed-off-by: zeminzhou <zhouzemin@ping.com>
Signed-off-by: zeminzhou <zhouzemin@pingcap.com>
@zeminzhou zeminzhou requested a review from haojinming June 14, 2022 09:01
Signed-off-by: zeminzhou <zhouzemin@pingcap.com>
// WorkloadInfo records the workload info of a keyspan
type WorkloadInfo struct {
Workload uint64 `json:"workload"`
Workload int64 `json:"workload"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change to int64? Can it be negative?

Signed-off-by: zeminzhou <zhouzemin@pingcap.com>
.
Signed-off-by: zeminzhou <zhouzemin@pingcap.com>

func (w workloads) SelectIdleCapture() model.CaptureID {
minWorkload := uint64(math.MaxUint64)
minWorkload := uint64(math.MaxInt64)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe revert this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

.
Signed-off-by: zeminzhou <zhouzemin@pingcap.com>
Copy link
Collaborator

@pingyu pingyu left a comment

Choose a reason for hiding this comment

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

LGTM~

Copy link
Contributor

@haojinming haojinming left a comment

Choose a reason for hiding this comment

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

LGTM~

@pingyu pingyu merged commit 7aefe87 into tikv:main Jun 15, 2022
@zeminzhou zeminzhou deleted the scheduler branch July 4, 2022 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants