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

feat: Improve upon the default gRPC Connection Pool size #1706

Open
wants to merge 9 commits into
base: grpc-experimental
Choose a base branch
from

Conversation

cindy-peng
Copy link
Contributor

@cindy-peng cindy-peng commented Jan 9, 2025

The current Java SDK uses a default gRPC connection pool with only one channel . This can cause performance issues for clients with high request rates (over 100 queries per second) because they may experience throttling.

To address this, this PR update proposes changing the default connection pool settings to allow for multiple channels (minimum 1, maximum 4). This should improve performance for clients with high request rates, without requiring any changes to their code. Customers can still customize the connection pool settings if needed.

Please refer here for more details:
configure a more reasonable default grpc connection pool config

@cindy-peng cindy-peng requested review from a team as code owners January 9, 2025 19:22
@product-auto-label product-auto-label bot added size: s Pull request size is small. api: datastore Issues related to the googleapis/java-datastore API. labels Jan 9, 2025
Copy link

@gkevinzheng gkevinzheng left a comment

Choose a reason for hiding this comment

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

Could you use constants for gRPC minimum and maximum channel counts?

Copy link

@daniel-sanche daniel-sanche left a comment

Choose a reason for hiding this comment

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

LGTM, but I agree with Kevin that constants would be good to have

@cindy-peng
Copy link
Contributor Author

Could you use constants for gRPC minimum and maximum channel counts?

Good catch! Fixed.

@cindy-peng cindy-peng requested a review from a team as a code owner January 14, 2025 03:43
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Jan 14, 2025
Copy link

Warning: This pull request is touching the following templated files:

  • .github/workflows/ci.yaml

@product-auto-label product-auto-label bot added size: s Pull request size is small. and removed size: m Pull request size is medium. labels Jan 14, 2025
Copy link

@gkevinzheng gkevinzheng left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@jimit-j-shah jimit-j-shah left a comment

Choose a reason for hiding this comment

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

Have you tested and verified that this indeed improves latency at the SDK? Running a client with high enough operation qps should be able to verify this.

Also, you can use tcpdump or netstat to verify the work done over each gcp connection .

@cindy-peng
Copy link
Contributor Author

Have you tested and verified that this indeed improves latency at the SDK? Running a client with high enough operation qps should be able to verify this.

Also, you can use tcpdump or netstat to verify the work done over each gcp connection .

Hi Jimit, I ran some benchmark tests with 1000 QPS and this change improved lookup and update average latencies by 2%-6%: go/connection-pooling-benchmark. I think this is a good indicator that the change improved latency at the SDK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: datastore Issues related to the googleapis/java-datastore API. size: s Pull request size is small.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants