Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ctl: replace doRequest with HTTP client to get health status #8212
ctl: replace doRequest with HTTP client to get health status #8212
Changes from 16 commits
f1fb830
ee483b0
9f60979
27a74a5
09bc239
ec44793
fd7e738
794c735
b123494
d31faf8
70d567c
cef9427
f6f0f28
120c0d4
a425cba
eea4858
76a0902
132fe16
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, could you please add tests for this new method in
/tests/integrations/client/http_client_test.go
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
b123494
Check warning on line 350 in client/http/interface.go
Codecov / codecov/patch
client/http/interface.go#L350
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TestSendAndGetComponent
ci failed :(This is because the
requirePDClient
command executes the functionshouldInitPDClient
, which requires queryingpd/api/v1/cluster
.The handler is replaced in
TestSendAndGetComponent
, so you need to add this router to the handler inTestSendAndGetComponent
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can check code here
pd/tools/pd-ctl/tests/global_test.go
Lines 37 to 52 in 7e18a69
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your reminder and suggestion. I will fix it tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there need to be
pd-ctl
, maybe You can reuse the variables in theglobal.go
file directlypd/tools/pd-ctl/pdctl/command/global.go
Line 36 in 7e18a69
FYI
pd/tools/pd-ctl/pdctl/command/global.go
Lines 103 to 112 in 7e18a69
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a little strange here. The unit test requires it to be
pd-http-client
. I'll take a look for itThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pd/client/http/client.go
Line 310 in 4cd42b3
The HTTP client created in the
pd-ctl
is assigned bydefaultCallerID
. Do we need to change it tosource
.source
equals to "pd-ctl" string.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a bug :( Have been fixed in this pr. #8214
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you don't need open a new pr to fix it. We even need to modify the conflict for it. If it is bug, I can fix it in this pr easily. How was my propose?
Create an issue, link this pr, and then I can fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a different issue, maybe with a new pr will be a little clearer 0 0
Because some background information is needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is ok for me. Let's wait for #8214 merged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this test wants to focus on
calledID
, so I think it's no need to change it.FYI https://github.com/tikv/pd/pull/4491/files#diff-05102750a57e22445f5760fb1545227c9130a3a093f3b879e1f6e48ce27ea890
change
cluster
API because it is a required pathThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
L55 has tested it. If we keep returning
calledID
, json.UnMarshal will be failed.