-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix: improve http connection pooling for smtp notifications #20605
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
fix: improve http connection pooling for smtp notifications #20605
Conversation
cli/exp_scaletest_notifications.go
Outdated
| smtpHTTPTransport := &http.Transport{ | ||
| MaxConnsPerHost: 512, | ||
| MaxIdleConnsPerHost: 512, | ||
| MaxIdleConns: 2048, |
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.
What's the purpose of setting both this and a per-host value?
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.
That was left over from my earlier tests. I forgot that MaxIdleConns is unlimited by default, so MaxIdleConnsPerHost is enough. Fixed.
bb67e96 to
188da9b
Compare

This change updates how SMTP notifications are polled during scale tests.
Before, each of the ~2,000 pollers created its own http.Client, which opened thousands of short-lived TCP connections.
Under heavy load, this ran out of available network ports and caused errors like
connect: cannot assign requested addressNow, all pollers share one HTTP connection pool. This prevents port exhaustion and makes polling faster and more stable.
If a network error happens, the poller will now retry instead of stopping, so tests keep running until all notifications are received.
The
SMTPRequestTimeoutis now applied per request using a context, instead of being set on thehttp.Client.