-
Notifications
You must be signed in to change notification settings - Fork 259
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 cannot reach chhost bug #121
base: master
Are you sure you want to change the base?
fix cannot reach chhost bug #121
Conversation
proxy.go
Outdated
Transport: &http.Transport { | ||
DisableKeepAlives: true, | ||
Proxy: http.ProxyFromEnvironment, | ||
DialContext: (&net.Dialer{ | ||
Timeout: 30 * time.Second, | ||
KeepAlive: 30 * time.Second, | ||
DualStack: true, | ||
}).DialContext, | ||
ForceAttemptHTTP2: true, | ||
MaxIdleConns: 100, | ||
IdleConnTimeout: 90 * time.Second, | ||
TLSHandshakeTimeout: 10 * time.Second, | ||
ExpectContinueTimeout: 1 * time.Second, | ||
}, |
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.
Hi, thanks for your PR.
First, how to properly check that there is no regression in chproxy bringing this feature ? Do. you see any way to properly test that ?
Also, I have the feeling that we won't have the annoying error message in the source application but way more connections made over time, one query generating one connection. This could be very impactful on heavy loaded systems.
Couldn't we achieve the same behavior with a custom retry implemented ?
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.
Thank you very much for your advice,this test is not very easy to test, because this problem is not easy to repeat. We tested it through pressure test.We also tried to use retry, but we couldn't guarantee idempotency.Proxy requests one connection for each connection, observe that the system is not loaded high, we can also configure to close this KeepAlive
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.
observe that the system is not loaded high
is not a fact but subject to appreciation.
I'am not in favor of adding to CHProxy something that fits you, but could potentially impair a majority.
For the record, One of my production cluster is actually running between 50 an 100 QPS. The keep alive thing might have a disastrous effect on both microservices / chproxy and clickhouse.
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.
ok,I first submit the cannot reach chhost error log PR, which provides details about the connection CH exception, and then I will see if I can choose to close KeepAlive by adding configuration
1、print http transport error log information
2、close chproxy KeepAlive. The CH socket is closed over time, but the CHProxy connection is reused, causing the connection to reset by peer
fix this problem:#92