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

Use connection pooling when querying CosmosDB #3017

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kate-goldenring
Copy link
Contributor

@kate-goldenring kate-goldenring commented Feb 21, 2025

I noticed that KV reads/writes had a really high latency with the CosmosDB provider. I crated some simple test cases for interacting with CosmosDB and found that while the go SDK reuses connections between requests, the Rust SDK always created a new client. @lann discovered that the Azure Rust SDK disabled connection pooling because of this upstream issue: hyperium/hyper#2312. The maintainers closed the issue deeming it to be irreproducible. I am getting latencies 3x higher without connection pooling (from 15ms reads to 60ms reads). I think it is worth enabling the pooling by creating our own reqwest client and passing it.

I put it behind a feature flag in case we do hit the deadlock scenario that some have experienced.

Marking this as draft for discussion Seems like this is a change we want

@rylev
Copy link
Collaborator

rylev commented Feb 21, 2025

For reference this is the PR that disabled connection pooling in the Azure SDK.

I'd personally prefer to see this land. The hanging issue in hyper seems to be quite mysterious, and there's lots of people who do not see this issue. Given that this is behind a feature flag, we can easily introduce this without changing current behavior giving folks the ability to test this out and report issues if they find them.

@kate-goldenring kate-goldenring marked this pull request as ready for review February 21, 2025 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants