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

consumer: reuse lookupd http client #333

Merged
merged 1 commit into from
Oct 17, 2021

Conversation

JieTrancender
Copy link
Contributor

Problem

After some wrong happend on my machine, i found there are many TIME_WAIT status.
I have many nsqd topics and n consumers, consumer making http request which not using Keep-Alive causes this problem.

[xxx@xxx-inc.com server]$ netstat -an |grep 19991|awk '{print $6}'|sort|uniq -c
      2 ESTABLISHED
      1 LISTEN
   1401 TIME_WAIT

Solution

  1. using go http.Transport features(keepalives,timeouts,dualstack)
  2. reuse http client
[xxx@xxx-inc.com server]$ netstat -an |grep 19991|awk '{print $6}'|sort|uniq -c
   1217 ESTABLISHED
      1 LISTEN
    305 TIME_WAIT
  1. support setting lookupd http client
[xxx@xxx-inc.com server]$ netstat -an |grep 19991|awk '{print $6}'|sort|uniq -c
    131 ESTABLISHED
      1 LISTEN
    355 TIME_WAIT

api_request.go Outdated Show resolved Hide resolved
config.go Outdated Show resolved Hide resolved
consumer.go Outdated Show resolved Hide resolved
consumer.go Outdated Show resolved Hide resolved
config.go Outdated Show resolved Hide resolved
@JieTrancender
Copy link
Contributor Author

JieTrancender commented Sep 29, 2021

@ploxiln Thank you for your review, i have finished these modifications, please review it again.

UPGRADING.md Outdated
@@ -173,7 +173,7 @@ conforming to standard library logging conventions.

#### Misc.

Un-exported `NewDeadlineTransport` and `ApiRequest`, which never should have been exported in the
Un-exported `ApiRequest`, which never should have been exported in the
Copy link
Member

Choose a reason for hiding this comment

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

you can leave this as it was; it's still true that NewDeadlineTransport was un-exported (and then now has been un-existed ;)

Copy link
Member

Choose a reason for hiding this comment

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

If you undo this little change to UPGRADING, and squash down to one commit (with cleaned-up commit message), I'll merge.

Timeout: r.config.LookupdPollTimeout,
KeepAlive: 30 * time.Second,
}).DialContext,
ResponseHeaderTimeout: r.config.LookupdPollTimeout,
Copy link
Member

Choose a reason for hiding this comment

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

This seems redundant with the http.Client Timeout but I guess it's fine.
(Dialer Timeout above could probably be a shorter default, probably matching the 10s TLSHandshakeTimeout below. But again, I guess it's fine, doesn't really hurt.)

@JieTrancender
Copy link
Contributor Author

JieTrancender commented Oct 12, 2021

emmm, can request a review to other people? @ploxiln

@ploxiln ploxiln changed the title feat: reuse http client consumer: reuse lookupd http client Oct 17, 2021
@ploxiln ploxiln merged commit 61df38e into nsqio:master Oct 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants