jobmaster(engine/dm): fix empty worker status for OnWorkerOnline/OnWorkerOffline #7689

GMHDBJD · 2022-11-22T14:21:11Z

What problem does this PR solve?

Issue Number: close #7691

What is changed and how it works?

ignore nil extra status bytes for OnWorkerOnline

Check List

Tests

Unit test
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

ti-chi-bot · 2022-11-22T14:21:13Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

amyangfei
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.

amyangfei · 2022-11-22T14:26:09Z

/run-engine-integration-test

GMHDBJD · 2022-11-22T14:31:33Z

/run-engine-integration-test

amyangfei · 2022-11-22T16:09:33Z

/run-engine-integration-test

lance6716 · 2022-11-23T01:32:14Z

engine/jobmaster/dm/dm_jobmaster.go

@@ -189,9 +189,14 @@ func (jm *JobMaster) OnWorkerOnline(worker framework.WorkerHandle) error {

 // handleOnlineStatus is used by OnWorkerOnline and OnWorkerStatusUpdated.
 func (jm *JobMaster) handleOnlineStatus(worker framework.WorkerHandle) error {
+	extBytes := worker.Status().ExtBytes
+	if len(extBytes) == 0 {


maybe we can remove the usage of OnWorkerOnline callback, only use OnWorkerStatusUpdated? That is to say, because DM needs bussiness online message with worker bound information, plain framework online is no use

What if OnWorkerStatusUpdated is not called because the message is discarded for some reasons, like buffer full?

worker will be regard as offline after 2*HeartbeatInterval.

tiflow/engine/jobmaster/dm/worker_manager.go

Line 159 in 822a79f

} else if worker.CreateFailed() {

lance6716 · 2022-11-23T01:36:17Z

engine/jobmaster/dm/dm_jobmaster.go

@@ -201,24 +206,28 @@ func (jm *JobMaster) handleOnlineStatus(worker framework.WorkerHandle) error {

 // OnWorkerOffline implements JobMasterImpl.OnWorkerOffline
 func (jm *JobMaster) OnWorkerOffline(worker framework.WorkerHandle, reason error) error {
+	extBytes := worker.Status().ExtBytes
+	if len(extBytes) == 0 {
+		jm.Logger().Warn("worker status extBytes is empty", zap.String(logutil.ConstFieldWorkerKey, worker.ID()))


it should not happen, otherwise we have no chance to UpdateClient. Maybe we can use DPanic level and let the logger turn on Development for DPanic

tiflow/dm/pkg/log/log.go

Line 118 in 822a79f

Development: inDev,

Let me check whether OnWorkerOffline can happen without receiving OnWorkerOnline

I left a function comment as

// Only after OnWorkerOnline, OnWorkerOffline of the same worker may be called.

Please help modify it if's no longer correct

It may happen when we receive OnWorkerOnline, OnWorkerOffline but not receiving OnWorkerStatusUpdate

Have confirmed, the comment needs to be updated.

✅ OnWorkerOnline -> OnWorkerOffline

✅ OnWorkerOffline, without OnWorkerOnline

❌ OnWorkerOffline -> OnWorkerOnline, can't happen

❌ OnWorkerOnline, without OnWorkerOffline, can't happen.

lance6716 · 2022-11-23T01:37:29Z

engine/jobmaster/dm/dm_jobmaster.go

-		err := json.Unmarshal(workerHandle.Status().ExtBytes, &taskStatus)
+		extBytes := workerHandle.Status().ExtBytes
+		if len(extBytes) == 0 {
+			return nil, nil, errors.Errorf("extBytes of %d is empty", workerHandle.ID())


we can continue to waiting worker's UpdateStatus?

restart and recover until worker's UpdateStatusEvent

amyangfei · 2022-11-23T04:28:29Z

/merge

ti-chi-bot · 2022-11-23T04:28:31Z

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

Commit hash: 9700119

ti-chi-bot · 2022-11-23T04:28:45Z

@GMHDBJD: 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.

codecov-commenter · 2022-11-23T04:45:32Z

Codecov Report

Merging #7689 (7e2e6be) into master (bf9e9ad) will increase coverage by 0.0145%.
The diff coverage is 26.7281%.

Additional details and impacted files

Flag	Coverage Δ
dm	`52.1552% <ø> (-0.0075%)`	⬇️
engine	`64.0328% <38.0000%> (+0.0104%)`	⬆️

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

@@               Coverage Diff                @@
##             master      #7689        +/-   ##
================================================
+ Coverage   59.9132%   59.9277%   +0.0145%     
================================================
  Files           810        809         -1     
  Lines         93098      93052        -46     
================================================
- Hits          55778      55764        -14     
+ Misses        32472      32427        -45     
- Partials       4848       4861        +13

fix empty worker status for OnWorkerOnline/OnWorkerOffline

6d15f4d

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 22, 2022

Merge branch 'master' into fixEmptyExtBytes

5828b75

GMHDBJD added status/ptal Could you please take a look? area/engine Issues or PRs related to Dataflow Engine. labels Nov 22, 2022

log error for offline worker too

87ee9e3

GMHDBJD added 2 commits November 22, 2022 23:25

fix empty worker status for recover

4c61915

return error in recover

ae02b09

amyangfei requested review from sleepymole, lance6716, maxshuang and CharlesCheung96 November 23, 2022 01:27

lance6716 reviewed Nov 23, 2022

View reviewed changes

address comment

9700119

lance6716 approved these changes Nov 23, 2022

View reviewed changes

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

amyangfei approved these changes Nov 23, 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 23, 2022

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

Merge branch 'master' into fixEmptyExtBytes

7e2e6be

ti-chi-bot merged commit 7c67feb into pingcap:master Nov 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

jobmaster(engine/dm): fix empty worker status for OnWorkerOnline/OnWorkerOffline #7689

jobmaster(engine/dm): fix empty worker status for OnWorkerOnline/OnWorkerOffline #7689

GMHDBJD commented Nov 22, 2022 •

edited by amyangfei

Loading

ti-chi-bot commented Nov 22, 2022 •

edited

Loading

amyangfei commented Nov 22, 2022

GMHDBJD commented Nov 22, 2022

amyangfei commented Nov 22, 2022

lance6716 Nov 23, 2022

maxshuang Nov 23, 2022

GMHDBJD Nov 23, 2022

lance6716 Nov 23, 2022 •

edited

Loading

amyangfei Nov 23, 2022 •

edited

Loading

lance6716 Nov 23, 2022

GMHDBJD Nov 23, 2022

amyangfei Nov 23, 2022 •

edited

Loading

lance6716 Nov 23, 2022

GMHDBJD Nov 23, 2022

amyangfei commented Nov 23, 2022

ti-chi-bot commented Nov 23, 2022

ti-chi-bot commented Nov 23, 2022

codecov-commenter commented Nov 23, 2022 •

edited

Loading

jobmaster(engine/dm): fix empty worker status for OnWorkerOnline/OnWorkerOffline #7689

jobmaster(engine/dm): fix empty worker status for OnWorkerOnline/OnWorkerOffline #7689

Conversation

GMHDBJD commented Nov 22, 2022 • edited by amyangfei 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 22, 2022 • edited Loading

amyangfei commented Nov 22, 2022

GMHDBJD commented Nov 22, 2022

amyangfei commented Nov 22, 2022

lance6716 Nov 23, 2022

Choose a reason for hiding this comment

maxshuang Nov 23, 2022

Choose a reason for hiding this comment

GMHDBJD Nov 23, 2022

Choose a reason for hiding this comment

lance6716 Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

amyangfei Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

lance6716 Nov 23, 2022

Choose a reason for hiding this comment

GMHDBJD Nov 23, 2022

Choose a reason for hiding this comment

amyangfei Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

lance6716 Nov 23, 2022

Choose a reason for hiding this comment

GMHDBJD Nov 23, 2022

Choose a reason for hiding this comment

amyangfei commented Nov 23, 2022

ti-chi-bot commented Nov 23, 2022

ti-chi-bot commented Nov 23, 2022

codecov-commenter commented Nov 23, 2022 • edited Loading

Codecov Report

GMHDBJD commented Nov 22, 2022 •

edited by amyangfei

Loading

ti-chi-bot commented Nov 22, 2022 •

edited

Loading

lance6716 Nov 23, 2022 •

edited

Loading

amyangfei Nov 23, 2022 •

edited

Loading

amyangfei Nov 23, 2022 •

edited

Loading

codecov-commenter commented Nov 23, 2022 •

edited

Loading