sorter: fix Unified Sorter error handling & add more unit test cases #1619

liuzix · 2021-04-08T09:41:55Z

What problem does this PR solve?

Unified Sorter might return an unhelpful error ErrWorkerPoolHandleCancelled for some IO errors.
Some error handling code might cause data race.

What is changed and how it works?

Filter the ErrWorkerPoolHandleCancelled error.
Add appropriate synchronization to avoid races.
Add unit test cases for these fixes.

Check List

Tests

Unit test

Related changes

Need to cherry-pick to the release branch

Release note

Fix Unified Sorter returning unhelpful error messages & other misc. fixes.

liuzix · 2021-04-08T09:50:15Z

/run-all-tests

codecov-io · 2021-04-08T10:16:34Z

Codecov Report

Merging #1619 (e9170ca) into master (6c41167) will increase coverage by 0.1213%.
The diff coverage is 83.3333%.

@@               Coverage Diff                @@
##             master      #1619        +/-   ##
================================================
+ Coverage   52.9231%   53.0445%   +0.1213%     
================================================
  Files           152        152                
  Lines         16027      16045        +18     
================================================
+ Hits           8482       8511        +29     
+ Misses         6646       6641         -5     
+ Partials        899        893         -6

liuzix · 2021-04-08T11:31:55Z

/run-all-tests

amyangfei · 2021-04-09T09:24:51Z

cdc/puller/sorter/heap_sorter.go

 	dataSize      int64
 	lastTs        uint64 // for debugging TODO remove
 	canceller     *asyncCanceller
+
+	isEmpty bool // ready only field


Should ready only be read only?

amyangfei · 2021-04-09T09:28:26Z

cdc/puller/sorter/unified_sorter.go

@@ -223,6 +224,11 @@ func (s *UnifiedSorter) Run(ctx context.Context) error {
 						default:
 						}
 						err := sorter.poolHandle.AddEvent(subctx, event)
+						if cerror.ErrWorkerPoolHandleCancelled.Equal(errors.Cause(err)) {


Suggested change

if cerror.ErrWorkerPoolHandleCancelled.Equal(errors.Cause(err)) {

if cerror.ErrWorkerPoolHandleCancelled.Equal(err) {

Equal will unwrap the err first

amyangfei · 2021-04-09T09:28:35Z

cdc/puller/sorter/unified_sorter.go

@@ -240,6 +246,11 @@ func (s *UnifiedSorter) Run(ctx context.Context) error {
 				default:
 					err := heapSorters[targetID].poolHandle.AddEvent(subctx, event)
 					if err != nil {
+						if cerror.ErrWorkerPoolHandleCancelled.Equal(errors.Cause(err)) {


amyangfei · 2021-04-09T09:44:41Z

cdc/puller/sorter/heap_sorter.go

+			backEnd := task.GetBackEnd()
+			if backEnd != nil {
+				defer task.markDeallocated()


What happens if

In L154-L155, backEnd is not nil

When calling pool.dealloc(backEnd), task is marked as deallocated and task.backend becomes nil, through backEnd is not nil

dealloc will not race with itself, because dealloc calls from different goroutines are always synchronized by a write to finished channel.

…d-sorter-error-report-fix

liuzix · 2021-04-12T05:09:15Z

/run-all-tests

amyangfei · 2021-04-13T08:58:02Z

/lgtm

zier-one · 2021-04-15T02:14:07Z

/lgtm

ti-chi-bot · 2021-04-15T02:14:09Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

amyangfei
leoppro

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 writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

zier-one · 2021-04-15T02:14:21Z

/merge

ti-chi-bot · 2021-04-15T02:14:23Z

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

Commit hash: 988c07c

amyangfei · 2021-04-15T10:24:31Z

unit tests are still unstable, PTAL @liuzix

liuzix · 2021-04-15T11:07:49Z

/merge

ti-chi-bot · 2021-04-15T11:07:53Z

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

Commit hash: 1dd20c4

liuzix · 2021-04-15T11:52:29Z

/merge

ti-chi-bot · 2021-04-15T11:52:33Z

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

Commit hash: e6c8a5a

amyangfei · 2021-04-16T02:58:20Z

cdc/puller/sorter/sorter_test.go

+		err := testSorter(ctx, c, sorter, 10000000, true)
+		c.Assert(err, check.ErrorMatches, ".*injected alloc error.*")


The timeout still happens, can we decrease the count 10000000

zier-one · 2021-04-16T05:40:52Z

/merge

ti-chi-bot · 2021-04-16T05:40:54Z

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

Commit hash: 156e52e

zier-one · 2021-04-16T06:18:58Z

/run-integration-tests

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot · 2021-04-16T06:58:11Z

cherry pick to release-4.0 in PR #1677

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot · 2021-04-16T06:58:36Z

cherry pick to release-5.0 in PR #1678

…1619) (#1678)

…ingcap#1619)

liuzix and others added 2 commits April 8, 2021 14:20

fix Unified Sorter error handling

17988b1

fix unit test

e9170ca

liuzix requested review from zier-one and amyangfei April 8, 2021 09:42

ti-chi-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 8, 2021

liuzix added type/bugfix This PR fixes a bug. component/puller Puller component. status/ptal Could you please take a look? labels Apr 8, 2021

ti-chi-bot requested a review from kennytm April 8, 2021 09:42

update errors.toml

e7e99c7

amyangfei reviewed Apr 9, 2021

View reviewed changes

liuzix added 2 commits April 12, 2021 11:30

address comments

2f78f73

Merge branch 'master' of github.com:pingcap/ticdc into zixiong-unifie…

988c07c

…d-sorter-error-report-fix

liuzix added the needs-cherry-pick-release-5.0 Should cherry pick this PR to release-5.0 branch. label Apr 12, 2021

zier-one added this to the v5.0.1 milestone Apr 13, 2021

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 13, 2021

zier-one added the needs-cherry-pick-release-4.0 Should cherry pick this PR to release-4.0 branch. label Apr 14, 2021

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 Apr 15, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 15, 2021

Merge branch 'master' into zixiong-unified-sorter-error-report-fix

ab342be

adjust unit test timeout

1dd20c4

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Apr 15, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 15, 2021

add debug log

e6c8a5a

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Apr 15, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 15, 2021

Merge branch 'master' into zixiong-unified-sorter-error-report-fix

84f909c

amyangfei reviewed Apr 16, 2021

View reviewed changes

adjust unit test event count

156e52e

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Apr 16, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 16, 2021

Merge branch 'master' into zixiong-unified-sorter-error-report-fix

32e9e86

ti-chi-bot merged commit 2277431 into pingcap:master Apr 16, 2021

ti-srebot pushed a commit to ti-srebot/ticdc that referenced this pull request Apr 16, 2021

cherry pick pingcap#1619 to release-4.0

3a0a74e

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot mentioned this pull request Apr 16, 2021

sorter: fix Unified Sorter error handling & add more unit test cases (#1619) #1677

Closed

ti-srebot pushed a commit to ti-srebot/ticdc that referenced this pull request Apr 16, 2021

cherry pick pingcap#1619 to release-5.0

85aedf5

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot mentioned this pull request Apr 16, 2021

sorter: fix Unified Sorter error handling & add more unit test cases (#1619) #1678

Merged

ti-chi-bot pushed a commit that referenced this pull request Apr 16, 2021

sorter: fix Unified Sorter error handling & add more unit test cases (#…

6363c48

…1619) (#1678)

liuzix added a commit to liuzix/ticdc that referenced this pull request Apr 30, 2021

sorter: fix Unified Sorter error handling & add more unit test cases (p…

2c1fcfe

…ingcap#1619)

liuzix added a commit to liuzix/ticdc that referenced this pull request May 6, 2021

sorter: fix Unified Sorter error handling & add more unit test cases (p…

2c14bf3

…ingcap#1619)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sorter: fix Unified Sorter error handling & add more unit test cases #1619

sorter: fix Unified Sorter error handling & add more unit test cases #1619

liuzix commented Apr 8, 2021

liuzix commented Apr 8, 2021

codecov-io commented Apr 8, 2021

liuzix commented Apr 8, 2021

amyangfei Apr 9, 2021

amyangfei Apr 9, 2021

amyangfei Apr 9, 2021

amyangfei Apr 9, 2021

liuzix Apr 12, 2021

liuzix commented Apr 12, 2021

amyangfei commented Apr 13, 2021

zier-one commented Apr 15, 2021

ti-chi-bot commented Apr 15, 2021

zier-one commented Apr 15, 2021

ti-chi-bot commented Apr 15, 2021

amyangfei commented Apr 15, 2021

liuzix commented Apr 15, 2021

ti-chi-bot commented Apr 15, 2021

liuzix commented Apr 15, 2021

ti-chi-bot commented Apr 15, 2021

amyangfei Apr 16, 2021

zier-one commented Apr 16, 2021

ti-chi-bot commented Apr 16, 2021

zier-one commented Apr 16, 2021

ti-srebot commented Apr 16, 2021

ti-srebot commented Apr 16, 2021

	if cerror.ErrWorkerPoolHandleCancelled.Equal(errors.Cause(err)) {
	if cerror.ErrWorkerPoolHandleCancelled.Equal(err) {

		err := testSorter(ctx, c, sorter, 10000000, true)
		c.Assert(err, check.ErrorMatches, ".injected alloc error.")

sorter: fix Unified Sorter error handling & add more unit test cases #1619

sorter: fix Unified Sorter error handling & add more unit test cases #1619

Conversation

liuzix commented Apr 8, 2021

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

liuzix commented Apr 8, 2021

codecov-io commented Apr 8, 2021

Codecov Report

liuzix commented Apr 8, 2021

amyangfei Apr 9, 2021

Choose a reason for hiding this comment

amyangfei Apr 9, 2021

Choose a reason for hiding this comment

amyangfei Apr 9, 2021

Choose a reason for hiding this comment

amyangfei Apr 9, 2021

Choose a reason for hiding this comment

liuzix Apr 12, 2021

Choose a reason for hiding this comment

liuzix commented Apr 12, 2021

amyangfei commented Apr 13, 2021

zier-one commented Apr 15, 2021

ti-chi-bot commented Apr 15, 2021

zier-one commented Apr 15, 2021

ti-chi-bot commented Apr 15, 2021

amyangfei commented Apr 15, 2021

liuzix commented Apr 15, 2021

ti-chi-bot commented Apr 15, 2021

liuzix commented Apr 15, 2021

ti-chi-bot commented Apr 15, 2021

amyangfei Apr 16, 2021

Choose a reason for hiding this comment

zier-one commented Apr 16, 2021

ti-chi-bot commented Apr 16, 2021

zier-one commented Apr 16, 2021

ti-srebot commented Apr 16, 2021

ti-srebot commented Apr 16, 2021