client: support to set timeout in http client #7847

CabinfeverB · 2024-02-22T16:37:23Z

What problem does this PR solve?

Issue Number: ref #7300

What is changed and how does it work?

don't set Timeout when create http.Clent
add timeout field in PD http client, and set context timeout

Check List

Tests

Unit test
Integration test

Code changes

Has the configuration change
Has HTTP API interfaces changed (Don't forget to add the declarative for the new API)
Has persistent data change

Side effects

Possible performance regression
Increased code complexity
Breaking backward compatibility

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
PR to update pingcap/tiup:
Need to cherry-pick to the release branch

Release note

None.

Signed-off-by: Cabinfever_B <[email protected]>

ti-chi-bot · 2024-02-22T16:37:26Z

[REVIEW NOTIFICATION]

This pull request has not been approved.

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 · 2024-02-22T16:37:26Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

codecov · 2024-02-22T16:51:02Z

Codecov Report

Merging #7847 (2dce385) into master (7179657) will decrease coverage by 0.21%.
The diff coverage is 100.00%.

❗ Current head 2dce385 differs from pull request most recent head d181242. Consider uploading reports for the commit d181242 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7847      +/-   ##
==========================================
- Coverage   73.68%   73.48%   -0.21%     
==========================================
  Files         436      432       -4     
  Lines       48525    47928     -597     
==========================================
- Hits        35754    35218     -536     
+ Misses       9723     9666      -57     
+ Partials     3048     3044       -4

Flag	Coverage Δ
unittests	`73.48% <100.00%> (-0.21%)`	⬇️

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

Signed-off-by: Cabinfever_B <[email protected]>

JmPotato · 2024-02-26T03:03:39Z

client/http/request_info.go

 }

 // newRequestInfo creates a new request info.
 func newRequestInfo() *requestInfo {
-	return &requestInfo{}
+	return &requestInfo{
+		timeout: defaultTimeout,


What do you think about setting a default timeout for the client and passing it through? Leave therequestInfo always empty to customize.

Do you mean that it is better to set default timeout for http.Client?
I can have a try, but we should replace innter client to do it.
The reason why I made this change is because:

I just want to initialize an http.Client

The timeout value of http.Client used in TiDB is 0

The timeout value of http.Client used in client-go is 30s

niubell · 2024-02-26T08:01:42Z

client/http/client.go

@@ -181,6 +183,8 @@ func (ci *clientInner) doRequest(
 		zap.String("caller-id", callerID),
 	}
 	log.Debug("[pd] request the http url", logFields...)
+	ctx, cancel = context.WithTimeout(ctx, reqInfo.timeout)


Set a timeout for each request may not be a good idea. How about just supporting only one way to set timeout: init timeout in client with WithTimeout function, and timeout mechanism also through Timeout in http.Client/http.Transport?

Got it. My initial idea was to create as few http.clients as possible, but I've found that this is not a good way to modify it.

Signed-off-by: Cabinfever_B <[email protected]>

JmPotato · 2024-02-27T06:26:28Z

client/http/client.go

+// WithTimeout sets and returns a new client with a new `http.Client` whose timeout is the given one.
+func (c *client) WithTimeout(dur time.Duration) Client {
+	newClient := *c
+	newClient.inner = c.inner.clone(withInnerTimeout(dur))
+	return &newClient
+}


The initial purpose of introducing clientInner is to allow safe sharing of the same inner core, even with multiple Client interfaces. This ensures that common components such as service discovery are reused effectively. However, this constraint appears to be violated. What about just keeping the timeout option as an initialization parameter instead of a dynamic variable?

Or we could make the timeout an upper layer's behavior rather than the HTTP client configuration. Like a Context with a timeout.

Currently, Common components don't rely on clientInner(http.Client). So I think it's safe to clone the clientInner. However, I do not consider this parameter to be a dynamic variable because it creates a new clientInner without affecting the previous clientInner.

In fact, the reason of why I do this is because the timeout of http. Client in the store helper in TiDB is 0. However, the timeout value of http. Client in client-go is 30 seconds. Setting a default timeout for http.Client eliminates the need to set a timeout in context each time for most requests.

Signed-off-by: Cabinfever_B <[email protected]>

ti-chi-bot · 2024-07-05T08:24:12Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

support timeout in http client

7543a99

Signed-off-by: Cabinfever_B <[email protected]>

ti-chi-bot bot added do-not-merge/needs-linked-issue do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. labels Feb 22, 2024

ti-chi-bot bot requested review from HunDunDM and JmPotato February 22, 2024 16:37

ti-chi-bot bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 22, 2024

add test

62220cf

Signed-off-by: Cabinfever_B <[email protected]>

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 23, 2024

CabinfeverB marked this pull request as ready for review February 23, 2024 09:57

ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 23, 2024

CabinfeverB requested review from rleungx and HuSharp and removed request for HunDunDM February 23, 2024 09:58

add test

43fd76f

Signed-off-by: Cabinfever_B <[email protected]>

ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/needs-linked-issue labels Feb 23, 2024

JmPotato reviewed Feb 26, 2024

View reviewed changes

CabinfeverB changed the title ~~client: support timeout in http client~~ client: support to set timeout in http client Feb 26, 2024

niubell reviewed Feb 26, 2024

View reviewed changes

address comment

2dce385

Signed-off-by: Cabinfever_B <[email protected]>

CabinfeverB mentioned this pull request Feb 26, 2024

*: set timeout for PD HTTP client pingcap/tidb#51286

Closed

13 tasks

JmPotato reviewed Feb 27, 2024

View reviewed changes

ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 2, 2024

merge master

d181242

Signed-off-by: Cabinfever_B <[email protected]>

ti-chi-bot bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 7, 2024

ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client: support to set timeout in http client #7847

client: support to set timeout in http client #7847

CabinfeverB commented Feb 22, 2024 •

edited

Loading

ti-chi-bot bot commented Feb 22, 2024

ti-chi-bot bot commented Feb 22, 2024

codecov bot commented Feb 22, 2024 •

edited

Loading

JmPotato Feb 26, 2024

rleungx Feb 26, 2024

CabinfeverB Feb 26, 2024

niubell Feb 26, 2024 •

edited

Loading

CabinfeverB Feb 26, 2024

JmPotato Feb 27, 2024

JmPotato Feb 27, 2024 •

edited

Loading

CabinfeverB Feb 27, 2024

ti-chi-bot bot commented Jul 5, 2024

client: support to set timeout in http client #7847

Are you sure you want to change the base?

client: support to set timeout in http client #7847

Conversation

CabinfeverB commented Feb 22, 2024 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Feb 22, 2024

ti-chi-bot bot commented Feb 22, 2024

codecov bot commented Feb 22, 2024 • edited Loading

Codecov Report

JmPotato Feb 26, 2024

Choose a reason for hiding this comment

rleungx Feb 26, 2024

Choose a reason for hiding this comment

CabinfeverB Feb 26, 2024

Choose a reason for hiding this comment

niubell Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

CabinfeverB Feb 26, 2024

Choose a reason for hiding this comment

JmPotato Feb 27, 2024

Choose a reason for hiding this comment

JmPotato Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

CabinfeverB Feb 27, 2024

Choose a reason for hiding this comment

ti-chi-bot bot commented Jul 5, 2024

CabinfeverB commented Feb 22, 2024 •

edited

Loading

codecov bot commented Feb 22, 2024 •

edited

Loading

niubell Feb 26, 2024 •

edited

Loading

JmPotato Feb 27, 2024 •

edited

Loading