dm(engine): declarative command should consider time #7640

lance6716 · 2022-11-18T06:21:00Z

Signed-off-by: lance6716 lance6716@gmail.com

What problem does this PR solve?

Issue Number: ref #4287

What is changed and how it works?

If the running "error" stage is updated after expected "running" stage, we skip action.

Check List

Tests

Integration test

Questions

Will it cause performance regression or break compatibility?

Do you need to update user documentation, design documentation or monitoring documentation?

Release note

 `None`.

Signed-off-by: lance6716 <lance6716@gmail.com>

ti-chi-bot · 2022-11-18T06:21:01Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

CharlesCheung96
GMHDBJD

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.

lance6716 · 2022-11-18T06:22:09Z

/cc @CharlesCheung96 @GMHDBJD

codecov-commenter · 2022-11-18T06:39:30Z

Codecov Report

Merging #7640 (6b8731f) into master (01e0f31) will increase coverage by 0.0142%.
The diff coverage is 87.8048%.

Additional details and impacted files

Flag	Coverage Δ
cdc	`66.3265% <ø> (+0.0771%)`	⬆️
dm	`52.1079% <25.0000%> (-0.0336%)`	⬇️
engine	`63.7690% <94.5945%> (-0.0144%)`	⬇️

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

@@               Coverage Diff                @@
##             master      #7640        +/-   ##
================================================
+ Coverage   59.8250%   59.8393%   +0.0142%     
================================================
  Files           812        812                
  Lines         93078      93014        -64     
================================================
- Hits          55684      55659        -25     
+ Misses        32541      32512        -29     
+ Partials       4853       4843        -10

Signed-off-by: lance6716 <lance6716@gmail.com>

GMHDBJD · 2022-11-18T08:10:39Z

engine/executor/dm/worker.go

+			Unit:             w.workerType,
+			Task:             w.taskID,
+			Stage:            stage,
+			StageUpdatedTime: time.Now(),


Seems this assign tasks no effect

I prefer to left this one, remove the write to StageUpdatedTime at jobmaster side

We only update task status when task stage changed, so the StageUpdatedTime may not change for a long time, does this meet user expectations

GMHDBJD · 2022-11-18T08:15:19Z

engine/jobmaster/dm/task_manager.go

@@ -115,6 +115,7 @@ func (tm *TaskManager) UpdateTaskStatus(taskStatus runtime.TaskStatus) {
 		zap.Stringer("unit", taskStatus.Unit),
 		zap.Uint64("config_modify_revison", taskStatus.CfgModRevision),
 	)
+	taskStatus.StageUpdatedTime = time.Now()


Maybe we can save it somewhere other than taskStatus, so we don't have to add unnecessary properties to TaskStatus and change the unit test

I prefer Stage and the StageUpdateTime to be together because they have tight relationship, will think about it soon

Signed-off-by: lance6716 <lance6716@gmail.com>

GMHDBJD

rest LGTM

GMHDBJD · 2022-11-21T04:51:23Z

engine/executor/dm/worker.go

+			Unit:             w.workerType,
+			Task:             w.taskID,
+			Stage:            stage,
+			StageUpdatedTime: time.Now(),


We only update task status when task stage changed, so the StageUpdatedTime may not change for a long time, does this meet user expectations

GMHDBJD

LGTM

amyangfei · 2022-11-21T07:40:36Z

/merge

ti-chi-bot · 2022-11-21T07:40:39Z

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

Commit hash: 4d74d2f

D3Hunter · 2022-11-21T07:54:52Z

/test-engine-integration-test

lance6716 · 2022-11-21T07:57:12Z

/test-engine-integration-test

I'll take a look at failing log

D3Hunter · 2022-11-21T07:58:41Z

/test-engine-integration-test

lance6716 · 2022-11-21T07:59:50Z

/run-engine-integration-test

ti-chi-bot · 2022-11-21T08:47:57Z

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

Commit hash: 47759cd

lance6716 · 2022-11-21T08:48:52Z

/run-all-tests

D3Hunter · 2022-11-21T09:04:16Z

/run-engine-integration-test

Signed-off-by: lance6716 <lance6716@gmail.com>

lance6716 · 2022-11-21T09:28:02Z

/merge

ti-chi-bot · 2022-11-21T09:28:05Z

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

Commit hash: 6e5c22c

lance6716 · 2022-11-21T09:32:23Z

/hold

waiting to find a better solution

Signed-off-by: lance6716 <lance6716@gmail.com>

lance6716 · 2022-11-21T10:20:34Z

/run-engine-integration-test

ptal @GMHDBJD

lance6716 · 2022-11-21T10:38:11Z

I can say for this time, falling CI should really be a flaky test. The case can be passed in my local PC

[Mon Nov 21 18:35:02 CST 2022] <<<<<< run test case e2e_worker_error success! >>>>>>

GMHDBJD

LGTM

lance6716 · 2022-11-21T10:39:35Z

/unhold

/merge

ti-chi-bot · 2022-11-21T10:39:38Z

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

Commit hash: d7a67aa

lance6716 · 2022-11-21T10:41:38Z

/run-all-tests

lance6716 · 2022-11-21T11:01:22Z

/run-verify

ti-chi-bot · 2022-11-21T11:06:09Z

@lance6716: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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 ti-community-infra/tichi repository.

lance6716 · 2022-11-21T11:07:38Z

/merge

amyangfei · 2022-11-21T11:22:47Z

failure caused by slow MySQL

[2022/11/21 11:15:47.868 +00:00] [WARN] [elector.go:78] ["failed to renew lease after renew deadline"] [error="context deadline exceeded"] [errorVerbose="context deadline exceeded\ngithub.com/pingcap/errors.AddStack\n\tgithub.com/pingcap/errors@v0.11.5-0.20220729040631-518f63d66278/errors.go:174\ngithub.com/pingcap/errors.Trace\n\tgithub.com/pingcap/errors@v0.11.5-0.20220729040631-518f63d66278/juju_adaptor.go:15\ngithub.com/pingcap/tiflow/pkg/retry.run\n\tgithub.com/pingcap/tiflow/pkg/retry/retry_with_opt.go:90\ngithub.com/pingcap/tiflow/pkg/retry.Do\n\tgithub.com/pingcap/tiflow/pkg/retry/retry_with_opt.go:34\ngithub.com/pingcap/tiflow/engine/pkg/election.(*Elector).updateRecord\n\tgithub.com/pingcap/tiflow/engine/pkg/election/elector.go:251\ngithub.com/pingcap/tiflow/engine/pkg/election.(*Elector).renew\n\tgithub.com/pingcap/tiflow/engine/pkg/election/elector.go:123\ngithub.com/pingcap/tiflow/engine/pkg/election.(*Elector).Run\n\tgithub.com/pingcap/tiflow/engine/pkg/election/elector.go:77\ngithub.com/pingcap/tiflow/engine/servermaster.(*Server).Run.func3\n\tgithub.com/pingcap/tiflow/engine/servermaster/server.go:450\ngolang.org/x/sync/errgroup.(*Group).Go.func1\n\tgolang.org/x/sync@v0.1.0/errgroup/errgroup.go:75\nruntime.goexit\n\truntime/asm_amd64.s:1594"] [renew-deadline=8s]
[2022/11/21 11:15:47.868 +00:00] [INFO] [elector.go:211] ["cancel leader callback"] [reason="renew lease failed"]
[2022/11/21 11:15:47.868 +00:00] [INFO] [watch_executors.go:50] ["WatchExecutors exited"] [error="context canceled"]

server-master-2.log
275:[2022/11/21 11:15:59.732 +00:00] [INFO] [server.go:845] ["leader is initialized"] [took=207.391438ms]
667:[2022/11/21 11:16:39.607 +00:00] [INFO] [server.go:845] ["leader is initialized"] [took=4.55990789s]

server-master-0.log
67:[2022/11/21 11:13:52.868 +00:00] [INFO] [server.go:845] ["leader is initialized"] [took=127.998996ms]

server-master-1.log
404:[2022/11/21 11:15:04.314 +00:00] [INFO] [server.go:845] ["leader is initialized"] [took=169.515992ms]
1027:[2022/11/21 11:16:21.163 +00:00] [INFO] [server.go:845] ["leader is initialized"] [took=133.602022ms]

amyangfei · 2022-11-21T11:22:56Z

/run-engine-integration-test

amyangfei · 2022-11-21T11:36:22Z

/run-engine-integration-test

add some test case

3490c3f

Signed-off-by: lance6716 <lance6716@gmail.com>

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 18, 2022

ti-chi-bot requested review from CharlesCheung96 and GMHDBJD November 18, 2022 06:22

finish!

8c0ac94

Signed-off-by: lance6716 <lance6716@gmail.com>

ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 18, 2022

lance6716 changed the title ~~test(engine): explain how to resume from an error~~ dm(engine): declarative command should consider time Nov 18, 2022

GMHDBJD reviewed Nov 18, 2022

View reviewed changes

lance6716 added 2 commits November 18, 2022 17:37

fix test

2aa445d

Signed-off-by: lance6716 <lance6716@gmail.com>

change impl

dec82af

Signed-off-by: lance6716 <lance6716@gmail.com>

CharlesCheung96 approved these changes Nov 21, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 21, 2022

GMHDBJD reviewed Nov 21, 2022

View reviewed changes

Merge branch 'master' into refine-engine-IT

4d74d2f

GMHDBJD approved these changes Nov 21, 2022

View reviewed changes

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 Nov 21, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 21, 2022

ti-chi-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 21, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 21, 2022

change a behaviour, will fix it later

6e5c22c

Signed-off-by: lance6716 <lance6716@gmail.com>

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Nov 21, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 21, 2022

ti-chi-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 21, 2022

change impl

d7a67aa

Signed-off-by: lance6716 <lance6716@gmail.com>

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Nov 21, 2022

GMHDBJD approved these changes Nov 21, 2022

View reviewed changes

ti-chi-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 21, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 21, 2022

Merge branch 'master' into refine-engine-IT

6b8731f

ti-chi-bot merged commit 3d22bf4 into pingcap:master Nov 21, 2022

dm(engine): declarative command should consider time #7640

dm(engine): declarative command should consider time #7640

Conversation

lance6716 commented Nov 18, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Tests

Questions

Will it cause performance regression or break compatibility?

Do you need to update user documentation, design documentation or monitoring documentation?

Release note

ti-chi-bot commented Nov 18, 2022 • edited Loading

lance6716 commented Nov 18, 2022

codecov-commenter commented Nov 18, 2022 • edited Loading

Codecov Report

GMHDBJD Nov 18, 2022

Choose a reason for hiding this comment

lance6716 Nov 18, 2022

Choose a reason for hiding this comment

GMHDBJD Nov 21, 2022

Choose a reason for hiding this comment

GMHDBJD Nov 18, 2022

Choose a reason for hiding this comment

lance6716 Nov 18, 2022

Choose a reason for hiding this comment

GMHDBJD left a comment

Choose a reason for hiding this comment

GMHDBJD Nov 21, 2022

Choose a reason for hiding this comment

GMHDBJD left a comment

Choose a reason for hiding this comment

amyangfei commented Nov 21, 2022

ti-chi-bot commented Nov 21, 2022

D3Hunter commented Nov 21, 2022

lance6716 commented Nov 21, 2022

D3Hunter commented Nov 21, 2022

lance6716 commented Nov 21, 2022

ti-chi-bot commented Nov 21, 2022

lance6716 commented Nov 21, 2022

D3Hunter commented Nov 21, 2022

lance6716 commented Nov 21, 2022

ti-chi-bot commented Nov 21, 2022

lance6716 commented Nov 21, 2022

lance6716 commented Nov 21, 2022

lance6716 commented Nov 21, 2022

GMHDBJD left a comment

Choose a reason for hiding this comment

lance6716 commented Nov 21, 2022

ti-chi-bot commented Nov 21, 2022

lance6716 commented Nov 21, 2022

lance6716 commented Nov 21, 2022

ti-chi-bot commented Nov 21, 2022

lance6716 commented Nov 21, 2022

amyangfei commented Nov 21, 2022 • edited Loading

amyangfei commented Nov 21, 2022

amyangfei commented Nov 21, 2022

lance6716 commented Nov 18, 2022 •

edited

Loading

ti-chi-bot commented Nov 18, 2022 •

edited

Loading

codecov-commenter commented Nov 18, 2022 •

edited

Loading

amyangfei commented Nov 21, 2022 •

edited

Loading