Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

new_owner: a feed state manager for owner #1826

Merged
merged 13 commits into from
May 30, 2021
Merged

new_owner: a feed state manager for owner #1826

merged 13 commits into from
May 30, 2021

Conversation

zier-one
Copy link
Contributor

What problem does this PR solve?

the feed state manager: responsible for changefeed lifecycle management, start, stop, etc. read error report by the processor; handle admin jobs; retry and backoff changefeed....

What is changed and how it works?

Check List

Tests

  • Unit test

Release note

  • No release note

@ti-chi-bot ti-chi-bot requested review from amyangfei and lonng May 24, 2021 07:02
@zier-one zier-one added status/ptal Could you please take a look? needs-cherry-pick-release-4.0 Should cherry pick this PR to release-4.0 branch. needs-cherry-pick-release-5.0 Should cherry pick this PR to release-5.0 branch. labels May 24, 2021
@ti-chi-bot ti-chi-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 24, 2021
liuzix
liuzix reviewed May 25, 2021
View reviewed changes
cdc/model/changefeed.go Outdated Show resolved Hide resolved
cdc/owner/feed_state_manager.go Outdated Show resolved Hide resolved
cdc/owner/feed_state_manager.go Outdated Show resolved Hide resolved
cdc/owner/feed_state_manager.go Outdated Show resolved Hide resolved
cdc/owner/feed_state_manager.go
func (m *feedStateManager) handleAdminJob() (pendingJobs bool) {
job := m.popAdminJob()
if job == nil || job.CfID != m.state.ID {
return false
Copy link
Contributor

Choose a reason for hiding this comment

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

Panic or Warn here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's not necessary, we can just skip the invalid admin job.

cdc/owner/feed_state_manager.go Outdated Show resolved Hide resolved
cdc/owner/feed_state_manager.go Outdated Show resolved Hide resolved
cdc/owner/feed_state_manager.go Outdated Show resolved Hide resolved
pkg/filter/errors.go Outdated Show resolved Hide resolved
lonng
lonng reviewed May 27, 2021
View reviewed changes
cdc/model/changefeed.go Outdated Show resolved Hide resolved
lonng
lonng reviewed May 27, 2021
View reviewed changes
cdc/model/changefeed.go Outdated Show resolved Hide resolved
lonng
lonng reviewed May 27, 2021
View reviewed changes
pkg/filter/errors.go Outdated Show resolved Hide resolved
ben1009
ben1009 reviewed May 27, 2021
View reviewed changes
Copy link
Contributor

@ben1009 ben1009 left a comment

Choose a reason for hiding this comment

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

do we need change all // Copyright 2020 PingCAP, Inc. to // Copyright 2021 PingCAP, Inc.

cdc/model/changefeed.go Outdated
@@ -56,13 +57,13 @@ const (
// ErrorHistoryGCInterval represents how long we keep error record in changefeed info
ErrorHistoryGCInterval = time.Minute * 10

// errorHistoryCheckInterval represents time window for failure check
errorHistoryCheckInterval = time.Minute * 2
// ErrorHistoryCheckInterval represents time window for failure check
Copy link
Contributor

Choose a reason for hiding this comment

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

is it just me, but prefix with Error kind like a error const

cdc/model/changefeed.go Outdated Show resolved Hide resolved
pkg/filter/errors.go Outdated Show resolved Hide resolved
liuzix
liuzix reviewed May 28, 2021
View reviewed changes
Copy link
Contributor

@liuzix liuzix left a comment

Choose a reason for hiding this comment

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

rest lgtm

cdc/owner/feed_state_manager.go Outdated Show resolved Hide resolved
@liuzix
Copy link
Contributor

liuzix commented May 28, 2021

/lgtm

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • liuzix

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.

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label May 28, 2021
@ben1009
Copy link
Contributor

ben1009 commented May 28, 2021

/looks gtm

@zier-one zier-one added the require-LGT1 Indicates that the PR requires an LGTM. label May 28, 2021
@zier-one
Copy link
Contributor Author

/merge

@zier-one
Copy link
Contributor Author

/run-kafka-tests

2 similar comments
@zier-one
Copy link
Contributor Author

/run-kafka-tests

@zier-one
Copy link
Contributor Author

/run-kafka-tests

@ti-chi-bot ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label May 30, 2021
@zier-one
Copy link
Contributor Author

/run-all-tests

zier-one
zier-one commented May 30, 2021
View reviewed changes
cdc/model/changefeed.go Outdated Show resolved Hide resolved
@zier-one
Copy link
Contributor Author

/run-all-tests

@zier-one
Copy link
Contributor Author

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: c94210e

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label May 30, 2021
@codecov-commenter
Copy link

Codecov Report

Merging #1826 (2c789ec) into master (99ee8fb) will increase coverage by 0.7132%.
The diff coverage is 69.7029%.

@@               Coverage Diff                @@
##             master      #1826        +/-   ##
================================================
+ Coverage   53.4083%   54.1216%   +0.7132%     
================================================
  Files           154        159         +5     
  Lines         16166      16644       +478     
================================================
+ Hits           8634       9008       +374     
- Misses         6608       6678        +70     
- Partials        924        958        +34

@ti-chi-bot ti-chi-bot merged commit 36ecfc4 into pingcap:master May 30, 2021
@ti-chi-bot ti-chi-bot mentioned this pull request May 30, 2021
Closed
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created: #1881.

@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created: #1882.

@ti-chi-bot ti-chi-bot mentioned this pull request May 30, 2021
Closed
zier-one pushed a commit to zier-one/ticdc that referenced this pull request Jun 17, 2021
new_owner: a feed state manager for owner (pingcap#1826)
71e2f74
zier-one pushed a commit to zier-one/ticdc that referenced this pull request Jun 22, 2021
new_owner: a feed state manager for owner (pingcap#1826)
0bc3b79
zier-one pushed a commit to zier-one/ticdc that referenced this pull request Jun 22, 2021
new_owner: a feed state manager for owner (pingcap#1826)
89c2b24
zier-one pushed a commit to zier-one/ticdc that referenced this pull request Jun 22, 2021
new_owner: a feed state manager for owner (pingcap#1826)
c15ce88
zier-one pushed a commit to zier-one/ticdc that referenced this pull request Jun 22, 2021
new_owner: a feed state manager for owner (pingcap#1826)
7ca2e89
zier-one pushed a commit to zier-one/ticdc that referenced this pull request Jun 22, 2021
new_owner: a feed state manager for owner (pingcap#1826)
a0ed0f0
ti-chi-bot pushed a commit that referenced this pull request Jun 22, 2021
@ti-chi-bot
new_owner: a feed state manager for owner (#1826)
03fd1aa
ti-chi-bot pushed a commit that referenced this pull request Jun 23, 2021
@ti-chi-bot
new_owner: a feed state manager for owner (#1826)
420dd99
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lonng lonng lonng left review comments

@ben1009 ben1009 ben1009 left review comments

@liuzix liuzix liuzix left review comments

@amyangfei amyangfei Awaiting requested review from amyangfei

Assignees
No one assigned
Labels
needs-cherry-pick-release-4.0 Should cherry pick this PR to release-4.0 branch. needs-cherry-pick-release-5.0 Should cherry pick this PR to release-5.0 branch. require-LGT1 Indicates that the PR requires an LGTM. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT1 Indicates that a PR has LGTM 1. status/ptal Could you please take a look?
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@zier-one @liuzix @ti-chi-bot @ben1009 @codecov-commenter @lonng