sink(ticdc): fix broken test in cdc/sink/mq & migrate test-infra to testify #5386

CharlesCheung96 · 2022-05-11T04:09:12Z

What problem does this PR solve?

Issue Number: close #2907

What is changed and how it works?

fix goroutine leak in cdc/sink/mq/producer
migrate test-infra to testify & enable leak testing
fix broken mq_test

Check List

Tests

Unit test

Code changes

Has exported function/method change
Has exported variable/fields change

Release note

`None`.

ti-chi-bot · 2022-05-11T04:09:13Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

amyangfei
hi-rustin

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.

CharlesCheung96 · 2022-05-17T08:24:46Z

/run-all-tests

Rustin170506

Thanks!

Rustin170506 · 2022-05-17T08:55:16Z

cdc/sink/mq/mq_flush_worker_test.go

-					err := worker.addEvent(context.Background(), event)
-					require.NoError(t, err)
+					err := worker.addEvent(ctx, event)
+					if event.row == nil || event.row.CommitTs != math.MaxUint64 {


I don't quite understand this judgment, can you explain it?

Ref: https://github.com/pingcap/tiflow/pull/5386/files#diff-f95a3eed29e34d64995f1240ff99fc59e9bf2c95fbc9b88a9bd1cc0d0b32fc7dR169

This event will not be processed by the batch function, so it needs to be judged separately. Otherwise either the test fails here or a Goroutine leak occurs

Could you please add a comment here? Thanks!

cdc/sink/mq/mq_test.go

Rustin170506

Thanks for working on this. Could you please resolve the conflict?

Rustin170506 · 2022-05-18T06:52:28Z

/merge

ti-chi-bot · 2022-05-18T06:52:30Z

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

Commit hash: 0078166

Rustin170506 · 2022-05-18T08:41:37Z

/run-dm-integration-test

CharlesCheung96 · 2022-05-18T09:09:33Z

/run-integration-test

codecov-commenter · 2022-05-18T10:06:07Z

Codecov Report

Merging #5386 (fe8bdf9) into master (7a80b61) will increase coverage by 0.3128%.
The diff coverage is 51.1784%.

Flag	Coverage Δ
cdc	`61.6261% <43.7500%> (+0.6619%)`	⬆️
dm	`52.0011% <68.5393%> (+0.0408%)`	⬆️

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

@@               Coverage Diff                @@
##             master      #5386        +/-   ##
================================================
+ Coverage   55.9233%   56.2362%   +0.3128%     
================================================
  Files           528        530         +2     
  Lines         69935      69962        +27     
================================================
+ Hits          39110      39344       +234     
+ Misses        27100      26858       -242     
- Partials       3725       3760        +35

Rustin170506 · 2022-05-18T10:11:24Z

/run-all-tests

ti-chi-bot · 2022-05-18T11:14:48Z

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

Rustin170506 · 2022-05-18T11:41:58Z

/run-verify

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 11, 2022

CharlesCheung96 changed the title ~~sink(ticdc): fix goroutine leak in cdc/sink/mq & migrate test-infra to testify~~ [WIP] sink(ticdc): fix goroutine leak in cdc/sink/mq & migrate test-infra to testify May 11, 2022

ti-chi-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 11, 2022

CharlesCheung96 added the area/ticdc Issues or PRs related to TiCDC. label May 11, 2022

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 12, 2022

CharlesCheung96 force-pushed the fix_mq_test branch from d6b42fc to 60ac05f Compare May 17, 2022 08:20

CharlesCheung96 requested a review from asddongmen May 17, 2022 08:22

CharlesCheung96 added component/sink Sink component. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 17, 2022

CharlesCheung96 changed the title ~~[WIP] sink(ticdc): fix goroutine leak in cdc/sink/mq & migrate test-infra to testify~~ sink(ticdc): fix goroutine leak in cdc/sink/mq & migrate test-infra to testify May 17, 2022

CharlesCheung96 changed the title ~~sink(ticdc): fix goroutine leak in cdc/sink/mq & migrate test-infra to testify~~ sink(ticdc): fix broken test in cdc/sink/mq & migrate test-infra to testify May 17, 2022

CharlesCheung96 requested review from Rustin170506, 3AceShowHand and amyangfei May 17, 2022 08:25

Rustin170506 reviewed May 17, 2022

View reviewed changes

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 17, 2022

Rustin170506 approved these changes May 18, 2022

View reviewed changes

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

amyangfei approved these changes May 18, 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 May 18, 2022

CharlesCheung96 force-pushed the fix_mq_test branch from f21ba51 to 0eab07d Compare May 18, 2022 04:25

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 18, 2022

fix mq test

5c54e1a

CharlesCheung96 force-pushed the fix_mq_test branch from 0eab07d to 5c54e1a Compare May 18, 2022 06:25

add some comments

0078166

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

Merge branch 'master' into fix_mq_test

64c9c92

Merge branch 'master' into fix_mq_test

d156249

Merge branch 'master' into fix_mq_test

a0439d6

ti-chi-bot added 2 commits May 18, 2022 18:26

Merge branch 'master' into fix_mq_test

2c94029

Merge branch 'master' into fix_mq_test

fe8bdf9

ti-chi-bot merged commit d76503b into pingcap:master May 18, 2022

This was referenced May 22, 2022

migrate test-infra to testify for cdc/sink/mq pkg #2909

Closed

migrate test-infra to testify for cdc/sink/producer/kafka pkg #2911

Closed

migrate test-infra to testify for cdc/sink/dispatcher pkg #2910

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sink(ticdc): fix broken test in cdc/sink/mq & migrate test-infra to testify #5386

sink(ticdc): fix broken test in cdc/sink/mq & migrate test-infra to testify #5386

CharlesCheung96 commented May 11, 2022 •

edited

Loading

ti-chi-bot commented May 11, 2022 •

edited

Loading

CharlesCheung96 commented May 17, 2022

Rustin170506 left a comment

Rustin170506 May 17, 2022

CharlesCheung96 May 17, 2022

Rustin170506 May 18, 2022

Rustin170506 left a comment

Rustin170506 commented May 18, 2022

ti-chi-bot commented May 18, 2022

Rustin170506 commented May 18, 2022

CharlesCheung96 commented May 18, 2022

codecov-commenter commented May 18, 2022 •

edited

Loading

Rustin170506 commented May 18, 2022

ti-chi-bot commented May 18, 2022

Rustin170506 commented May 18, 2022

sink(ticdc): fix broken test in cdc/sink/mq & migrate test-infra to testify #5386

sink(ticdc): fix broken test in cdc/sink/mq & migrate test-infra to testify #5386

Conversation

CharlesCheung96 commented May 11, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented May 11, 2022 • edited Loading

CharlesCheung96 commented May 17, 2022

Rustin170506 left a comment

Choose a reason for hiding this comment

Rustin170506 May 17, 2022

Choose a reason for hiding this comment

CharlesCheung96 May 17, 2022

Choose a reason for hiding this comment

Rustin170506 May 18, 2022

Choose a reason for hiding this comment

Rustin170506 left a comment

Choose a reason for hiding this comment

Rustin170506 commented May 18, 2022

ti-chi-bot commented May 18, 2022

Rustin170506 commented May 18, 2022

CharlesCheung96 commented May 18, 2022

codecov-commenter commented May 18, 2022 • edited Loading

Codecov Report

Rustin170506 commented May 18, 2022

ti-chi-bot commented May 18, 2022

Rustin170506 commented May 18, 2022

CharlesCheung96 commented May 11, 2022 •

edited

Loading

ti-chi-bot commented May 11, 2022 •

edited

Loading

codecov-commenter commented May 18, 2022 •

edited

Loading