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

Support for max_requests_connection in downstream connections of thrift proxy #14560

Closed
wbpcode opened this issue Jan 5, 2021 · 4 comments
Closed

Comments

@wbpcode
Copy link
Member

wbpcode commented Jan 5, 2021

Support for max_requests_connection in downstream connections of thrift proxy which make the envoy can actively disconnect from the thrift client at the right time.

Consider such a scenario. Assume that the thrift server has opened port 10000 and the IP is 1.2.3.4. But the envoy sidecar of the thrift client initially only had a virtual listener of 0.0.0.0:10000. Then the connection and request of the thrift client will be processed by 0.0.0.0:10000. But after a while, we created a new listener of 1.2.3.4:10000 for the sidecar, and at this time we expect that the new request will be processed by 1.2.3.4:10000. However, because the connection between the thrift client and the sidecar is a keep-alive connection, unless the client actively disconnects, subsequent requests will still be processed by 0.0.0.0:10000.

@wbpcode wbpcode added enhancement Feature requests. Not bugs or questions. triage Issue requires triage labels Jan 5, 2021
@rgs1
Copy link
Member

rgs1 commented Jan 5, 2021

It would still not be a clean drain, because there's no way in thrift to drain a connection cleanly afaik. But maybe that's OK for most use-cases?

Happy to review a PR if you want to give this a go @wbpcode.

cc: @fishcakez

@mattklein123 mattklein123 added area/thrift and removed enhancement Feature requests. Not bugs or questions. triage Issue requires triage labels Jan 5, 2021
@wbpcode
Copy link
Member Author

wbpcode commented Jan 6, 2021

It would still not be a clean drain, because there's no way in thrift to drain a connection cleanly afaik. But maybe that's OK for most use-cases?

Happy to review a PR if you want to give this a go @wbpcode.

cc: @fishcakez

I will have a try.

@github-actions
Copy link

github-actions bot commented Feb 5, 2021

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Feb 5, 2021
@wbpcode
Copy link
Member Author

wbpcode commented Feb 5, 2021

/no-stale

@github-actions github-actions bot removed the stale stalebot believes this issue/PR has not been touched recently label Feb 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants