Skip to content

feat(gax-grpc): add configurable resize delta and warning for repeated resizing#12838

Draft
lqiu96 wants to merge 12 commits intomainfrom
feat/channelpool-resizing
Draft

feat(gax-grpc): add configurable resize delta and warning for repeated resizing#12838
lqiu96 wants to merge 12 commits intomainfrom
feat/channelpool-resizing

Conversation

@lqiu96
Copy link
Copy Markdown
Member

@lqiu96 lqiu96 commented Apr 17, 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 introduces a configurable maxResizeDelta for the ChannelPool and adds a warning log when the pool resizes consecutively for five cycles. Feedback highlights a logic error in the resizing detection that could lead to false-positive warnings when the pool is at its limits; a suggestion is provided to track actual size changes instead. Additionally, the removal of test exclusions in the pom.xml was flagged as a potential cause for CI failures.

I am having trouble creating individual review comments. Click here to see my feedback.

sdk-platform-java/gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/ChannelPool.java (322-327)

high

The logic for determining if the pool is 'resizing' is flawed. By checking if the current size is outside the load-based bounds (minChannels and maxChannels), the counter will increment even when the pool is at its hard limits (e.g., at minChannelCount while idle). This will lead to false positive warnings after 5 idle cycles because minChannels will be 0 while the pool is correctly clamped at its minimum size (e.g., 1).

Instead, you should track whether the pool is actually attempting to change its size by comparing dampenedTarget with currentSize. This correctly identifies when the pool is either oscillating or slowly growing/shrinking due to the maxResizeDelta cap.

    boolean resized = (dampenedTarget != currentSize);
    if (resized) {
      consecutiveResizes++;
    } else {
      consecutiveResizes = 0;
    }

sdk-platform-java/gax-java/gax-grpc/pom.xml (145-149)

medium

The removal of the maven-surefire-plugin configuration seems unintended. This block was used to exclude specific tests that require environment variables to be set. Removing it will likely cause these tests to run and fail in standard CI environments. If this change was intentional, please provide justification; otherwise, it should be reverted.

@lqiu96 lqiu96 force-pushed the feat/channelpool-resizing branch from b17aa56 to 827b22d Compare April 17, 2026 21:09
@lqiu96
Copy link
Copy Markdown
Member Author

lqiu96 commented Apr 17, 2026

/gemini review

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 introduces a configurable maxResizeDelta for the ChannelPool in gax-grpc and implements a warning mechanism for repeated resizing cycles. Feedback focuses on ensuring thread-safety for the resize counter, optimizing redundant size checks, and restoring accidentally deleted metadata and documentation links in the java-iam-policy module.

Comment thread java-iam-policy/.repo-metadata.json
Comment thread java-iam-policy/README.md
@lqiu96
Copy link
Copy Markdown
Member Author

lqiu96 commented Apr 17, 2026

/gemini review

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 introduces a configurable maxResizeDelta in ChannelPoolSettings, adds logic to ChannelPool to track consecutive resizes and log warnings, and removes certain metadata and documentation links. Feedback indicates that the removal of the enable-api link in the README appears accidental and should be restored. Additionally, a stale comment in ChannelPool.java incorrectly describing the resize() method as synchronized should be removed.

Comment thread java-iam-policy/README.md
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