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

thriftbp: Tweak client pool connection reuse logic #668

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

fishy
Copy link
Member

@fishy fishy commented Jan 9, 2025

Only exceptions defined in thrift IDL is safe for reuse (getting them means the roundtrip is done fully and the response is read fully). All other errors have different degree of risks for reusing.

@fishy fishy requested a review from a team as a code owner January 9, 2025 18:43
@fishy fishy requested review from kylelemons, konradreiche and mathyourlife-reddit and removed request for a team January 9, 2025 18:43
Copy link

@mathyourlife-reddit mathyourlife-reddit left a comment

Choose a reason for hiding this comment

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

Has this change been tested in a service yet to get a feel for how many more connections will be closed?

@fishy
Copy link
Member Author

fishy commented Jan 9, 2025

@mathyourlife-reddit

Has this change been tested in a service yet to get a feel for how many more connections will be closed?

No tests done, but the number should be small, and for the majority of the connections that's reused before and will not be reused now, they really should not be reused (reusing before is a bug)

See https://issues.apache.org/jira/browse/THRIFT-5845 for more context.

Copy link

@mathyourlife-reddit mathyourlife-reddit left a comment

Choose a reason for hiding this comment

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

approving. Interested to hear how it looks when it goes out.

Only exceptions defined in thrift IDL is safe for reuse (getting them
means the roundtrip is done fully and the response is read fully). All
other errors have different degree of risks for reusing.
@fishy fishy force-pushed the thriftbp-client-pool-reuse-logic branch from 1d2aecc to d41e8bf Compare January 9, 2025 20:02
@fishy fishy merged commit f761d85 into master Jan 9, 2025
2 checks passed
@fishy fishy deleted the thriftbp-client-pool-reuse-logic branch January 9, 2025 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants