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

api,owner(ticdc): return error when api fails #4494

Merged
merged 7 commits into from
Feb 10, 2022
Merged

api,owner(ticdc): return error when api fails #4494

merged 7 commits into from
Feb 10, 2022

Conversation

overvenus
Copy link
Member

@overvenus overvenus commented Jan 27, 2022 •
edited
Loading

What problem does this PR solve?

Issue Number: close #1710 ref #3456

What is changed and how it works?

API waits and returns owner response.

Check List

Tests

  • Unit test

Side effects

  • Breaking backward compatibility (only for CLI)

Release note 

Synchronize HTTP API handling

@overvenus
api,owner(ticdc): return error when api fails
3d48f0c 
Signed-off-by: Neil Shen <overvenus@gmail.com>
@overvenus
errors(ticdc): remove confusing "key" in error message
ed487cc 
Signed-off-by: Neil Shen <overvenus@gmail.com>
@overvenus overvenus added type/bugfix This PR fixes a bug. component/cli CLI component. component/owner Owner component. labels Jan 27, 2022
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Jan 27, 2022 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • 3AceShowHand
  • asddongmen

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.

@ti-chi-bot ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 27, 2022
@overvenus
address lints
fb216eb 
Signed-off-by: Neil Shen <overvenus@gmail.com>
@overvenus
fix test
1dc276f 
Signed-off-by: Neil Shen <overvenus@gmail.com>
asddongmen
asddongmen reviewed Jan 27, 2022
View reviewed changes
cdc/owner/owner.go Outdated Show resolved Hide resolved
@overvenus
address comments
427fafb 
Signed-off-by: Neil Shen <overvenus@gmail.com>
asddongmen
asddongmen reviewed Feb 7, 2022
View reviewed changes
cdc/processor/manager.go Outdated Show resolved Hide resolved
asddongmen
asddongmen reviewed Feb 7, 2022
View reviewed changes
cdc/api/open.go Outdated Show resolved Hide resolved
asddongmen
asddongmen reviewed Feb 7, 2022
View reviewed changes
cdc/api/open.go Outdated Show resolved Hide resolved
Rustin170506
Rustin170506 reviewed Feb 7, 2022
View reviewed changes
Copy link
Member

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

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

Will this change break other systems that have cdc cli integrated?(I'm not sure if there are any such customers) Go from asynchronous to synchronous.

cdc/capture/capture.go
if isOwner {
res.owner = &owner.Owner{}
}
res.owner = o
Copy link
Member

Choose a reason for hiding this comment

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

What is the effect if nil is passed in here?

Copy link
Member Author

Choose a reason for hiding this comment

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

No effect, owner is an interface, default value is nil.

asddongmen
asddongmen reviewed Feb 7, 2022
View reviewed changes
cdc/owner/owner.go
type Owner struct {
// Owner managers TiCDC cluster.
//
// The interface is thread-safe, except for Tick, it's only used by etcd worker.
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no Tick in this interface.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tick is in orchestrator.Reactor.

asddongmen
asddongmen reviewed Feb 7, 2022
View reviewed changes
cdc/owner/owner.go Outdated Show resolved Hide resolved
@ti-chi-bot ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Feb 7, 2022
@overvenus
address comments
51e2418 
Signed-off-by: Neil Shen <overvenus@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented Feb 7, 2022 •
edited
Loading

Codecov Report

Merging #4494 (51e2418) into master (9607554) will decrease coverage by 0.1805%.
The diff coverage is 52.5547%.

Flag Coverage Δ
cdc 60.1194% <62.9213%> (+0.1972%) ⬆️
dm 51.5615% <33.3333%> (-0.4674%) ⬇️

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

@@               Coverage Diff                @@
##             master      #4494        +/-   ##
================================================
- Coverage   55.6402%   55.4596%   -0.1806%     
================================================
  Files           494        499         +5     
  Lines         61283      61744       +461     
================================================
+ Hits          34098      34243       +145     
- Misses        23750      24053       +303     
- Partials       3435       3448        +13

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 8, 2022
@overvenus
Copy link
Member Author

/run-kafka-integration-test
/run-integration-tests

@overvenus
Copy link
Member Author

Will this change break other systems that have cdc cli integrated?(I'm not sure if there are any such customers) Go from asynchronous to synchronous.

It is possible, though I think the impact is neglectable, as owner APIs are fast.

@3AceShowHand 3AceShowHand self-requested a review February 9, 2022 10:52
@ti-chi-bot 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 Feb 10, 2022
@overvenus
Copy link
Member Author

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 51e2418

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 10, 2022
@overvenus
Copy link
Member Author

/run-kafka-integration-test /tidb=pr/32081

1 similar comment
@overvenus
Copy link
Member Author

/run-kafka-integration-test /tidb=pr/32081

@ti-chi-bot ti-chi-bot merged commit e2d529c into pingcap:master Feb 10, 2022
zhaoxinyu pushed a commit to zhaoxinyu/ticdc that referenced this pull request Feb 16, 2022
@overvenus @zhaoxinyu
api,owner(ticdc): return error when api fails (pingcap#4494)
7a60841 
close pingcap#1710, ref pingcap#3456
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rustin170506 Rustin170506 Rustin170506 left review comments

@asddongmen asddongmen asddongmen approved these changes

@3AceShowHand 3AceShowHand 3AceShowHand approved these changes

Assignees
No one assigned
Labels
component/cli CLI component. component/owner Owner component. release-note Denotes a PR that will be considered when it comes time to generate release notes. 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/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

changefeed remove has not any output even the operation failed
6 participants
@overvenus @ti-chi-bot @codecov-commenter @3AceShowHand @asddongmen @Rustin170506