Skip to content

fix(datastore): Update initial ChannelPool configs according to Datastore best practice guide#12919

Merged
lqiu96 merged 4 commits intomainfrom
update-datastore-channelpool-defaults
Apr 24, 2026
Merged

fix(datastore): Update initial ChannelPool configs according to Datastore best practice guide#12919
lqiu96 merged 4 commits intomainfrom
update-datastore-channelpool-defaults

Conversation

@lqiu96
Copy link
Copy Markdown
Member

@lqiu96 lqiu96 commented Apr 24, 2026

No description provided.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request updates the Datastore gRPC channel pool configuration by reducing the maximum RPCs per channel and adding a minimum RPCs per channel setting. Unit tests were updated to match these defaults and to follow standard assertion argument ordering. A review comment identifies an inconsistency in the documentation where a comment still refers to the previous limits of 100 streams and 500 QPS, which should be addressed to ensure documentation accuracy.

Comment on lines +57 to +58
// Default to a slightly larger channel count to handle a larger initial QPS (up to ~500 QPS as
// each channel can handle 100 max streams). This default remains a bit conservative and will
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The comment at lines 57-58 refers to a stale implementation (~500 QPS and 100 max streams) which is inconsistent with the updated configuration. According to repository rules, comments referring to stale implementations should be removed rather than updated.

References
  1. Remove comments that refer to stale implementations instead of updating them.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this relevant? If INIT_CHANNEL_COUNT = 5 and CHANNEL_POOL_MAX_RPCS_PER_CHANNEL = 50, the max QPS would be 250, right?

Copy link
Copy Markdown
Member Author

@lqiu96 lqiu96 Apr 24, 2026

Choose a reason for hiding this comment

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

500 streams is the theoretical max as GFE will enforce a limit of 100 streams/ channel. However CHANNEL_POOL_MAX_RPCS_PER_CHANNEL refers to the channel pool config where we aim to have an avg of 50 streams/ channel

@lqiu96
Copy link
Copy Markdown
Member Author

lqiu96 commented Apr 24, 2026

/gemini review

@lqiu96 lqiu96 requested a review from jinseopkim0 April 24, 2026 15:37
@lqiu96 lqiu96 marked this pull request as ready for review April 24, 2026 15:37
@lqiu96 lqiu96 requested a review from a team as a code owner April 24, 2026 15:37
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request modifies the Datastore gRPC channel pool settings by adding a minimum RPCs per channel configuration and lowering the maximum RPCs per channel. It also updates documentation and unit tests. A review comment points out that a documentation block regarding QPS capacity is stale and should be removed rather than updated, following repository guidelines.

@lqiu96 lqiu96 enabled auto-merge (squash) April 24, 2026 15:47
@lqiu96 lqiu96 merged commit 851fb89 into main Apr 24, 2026
127 of 128 checks passed
@lqiu96 lqiu96 deleted the update-datastore-channelpool-defaults branch April 24, 2026 16:03
suztomo pushed a commit to suztomo/google-cloud-java that referenced this pull request Apr 24, 2026
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.

2 participants