Skip to content
This repository has been archived by the owner on Nov 24, 2023. It is now read-only.

worker: avoid illegal zero value of channel #868

Merged
merged 2 commits into from
Aug 7, 2020

Conversation

lance6716
Copy link
Collaborator

@lance6716 lance6716 commented Aug 6, 2020

What problem does this PR solve?

close #869

What is changed and how it works?

check _, ok <-chan (we may need to check more channel)

Check List

Tests

  • Manual test

Code changes

Side effects

Related changes

@lance6716 lance6716 added priority/unimportant Really minor change, requires approval from secondary reviewers status/PTAL This PR is ready for review. Add this label back after committing new changes type/bug-fix Bug fix labels Aug 6, 2020
@lance6716 lance6716 requested a review from lichunzhu August 6, 2020 12:05
Copy link
Contributor

@lichunzhu lichunzhu left a comment

Choose a reason for hiding this comment

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

What about errCh? Is there more problem like this?
Rest LGTM

@lance6716
Copy link
Collaborator Author

/run-all-tests

@codecov
Copy link

codecov bot commented Aug 6, 2020

Codecov Report

Merging #868 into master will increase coverage by 0.0287%.
The diff coverage is 100.0000%.

@@               Coverage Diff                @@
##             master       #868        +/-   ##
================================================
+ Coverage   56.0760%   56.1048%   +0.0287%     
================================================
  Files           214        220         +6     
  Lines         22712      22859       +147     
================================================
+ Hits          12736      12825        +89     
- Misses         8665       8726        +61     
+ Partials       1311       1308         -3     

Copy link
Collaborator

@GMHDBJD GMHDBJD left a comment

Choose a reason for hiding this comment

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

LGTM

@GMHDBJD GMHDBJD added status/LGT1 One reviewer already commented LGTM and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Aug 7, 2020
@lance6716
Copy link
Collaborator Author

What about errCh? Is there more problem like this?
Rest LGTM

too many channels to check 😂 we're going to do it when refactoring

Copy link
Contributor

@lichunzhu lichunzhu left a comment

Choose a reason for hiding this comment

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

LGTM

@lichunzhu lichunzhu added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Aug 7, 2020
@lance6716 lance6716 merged commit b40e960 into pingcap:master Aug 7, 2020
@lance6716 lance6716 deleted the test-go-mod branch August 7, 2020 07:32
@lance6716 lance6716 mentioned this pull request Aug 24, 2020
28 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
priority/unimportant Really minor change, requires approval from secondary reviewers status/LGT2 Two reviewers already commented LGTM, ready for merge type/bug-fix Bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dm-worker doesn't handle closed sourceBound chan which will make the log a little weird
3 participants