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.

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

api(ticdc): add /api/v2/tso: get tso from pd #5685

Merged
merged 6 commits into from
Jun 1, 2022

Conversation

crelax
Copy link
Contributor

@crelax crelax commented May 31, 2022

What problem does this PR solve?

Issue Number: close #5586

What is changed and how it works?

Add API Get tso from PD to OpenAPI v2

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Questions

Will it cause performance regression or break compatibility?

No

Do you need to update user documentation, design documentation or monitoring documentation?

No

Release note

TiCDC Open API V2: return tso get from pd 

@ti-chi-bot
Copy link
Member

ti-chi-bot commented May 31, 2022

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • liuzix
  • sdojjy

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 Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 31, 2022
@crelax
Copy link
Contributor Author

crelax commented May 31, 2022

/run-all-tests

@crelax
Copy link
Contributor Author

crelax commented May 31, 2022

/cc @liuzix @sdojjy @asddongmen

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label May 31, 2022
@crelax
Copy link
Contributor Author

crelax commented May 31, 2022

/run-verify-test

)

// OpenAPIV2 provides CDC v2 APIs
type OpenAPIV2 struct {
capture *capture.Capture
// use for unit test only
testStatusProvider owner.StatusProvider
Copy link
Contributor

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 the best way to mock the behavior of OpenAPIV2.

We can use failpoint into the method statusProvider, eg:

func (h *OpenAPIV2) statusProvider() owner.StatusProvider {
	failpoint.Inject("mockStatusProvider", func(){
		failpoint.Return(newMockStatusProvider())
	})
	return h.capture.StatusProvider()
}

To avoid pollute the definition of type OpenAPIV2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thx & Fixed. For now, I erased the dependence on StatusProvider by removal since tso_test didn't use it.

@lonng
Copy link
Contributor

lonng commented May 31, 2022

Rest LGTM

@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 Jun 1, 2022
@crelax crelax requested review from liuzix and lonng June 1, 2022 03:09
@crelax
Copy link
Contributor Author

crelax commented Jun 1, 2022

/run-all-tests

@crelax
Copy link
Contributor Author

crelax commented Jun 1, 2022

/run-verify-test

1 similar comment
@sdojjy
Copy link
Member

sdojjy commented Jun 1, 2022

/run-verify-test

@crelax
Copy link
Contributor Author

crelax commented Jun 1, 2022

/run-all-tests

@sdojjy
Copy link
Member

sdojjy commented Jun 1, 2022

/run-integration-tests

@crelax
Copy link
Contributor Author

crelax commented Jun 1, 2022

/run-integration-test

@asddongmen
Copy link
Contributor

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 2484f33

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 1, 2022
@asddongmen
Copy link
Contributor

/run-verify

@codecov-commenter
Copy link

codecov-commenter commented Jun 1, 2022

Codecov Report

Merging #5685 (2484f33) into master (fcea4d5) will increase coverage by 0.5594%.
The diff coverage is 60.8327%.

Flag Coverage Δ
cdc 61.9094% <62.3994%> (+0.7613%) ⬆️
dm 52.0348% <18.0000%> (-0.0079%) ⬇️
engine 58.7645% <ø> (?)

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

@@               Coverage Diff                @@
##             master      #5685        +/-   ##
================================================
+ Coverage   56.0768%   56.6363%   +0.5594%     
================================================
  Files           535        679       +144     
  Lines         70143      79027      +8884     
================================================
+ Hits          39334      44758      +5424     
- Misses        27078      30067      +2989     
- Partials       3731       4202       +471     

@ti-chi-bot ti-chi-bot merged commit 6e066ed into pingcap:master Jun 1, 2022
@crelax crelax deleted the cdc-api-v2-get-tso branch June 2, 2022 05:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TiCDC Open API V2: return tso get from pd
7 participants