[Dataflow Java Streaming] Fix defaulted flow control settings for grpc stream.#35793
[Dataflow Java Streaming] Fix defaulted flow control settings for grpc stream.#35793scwhittle merged 2 commits intoapache:masterfrom
Conversation
There was a problem hiding this comment.
Summary of Changes
Hello @scwhittle, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request addresses a critical issue in the Dataflow Java Streaming worker where gRPC stream flow control settings were not correctly defaulting. The core problem stemmed from the system's reliance on a null check to determine if default settings should be applied, which was flawed because the configuration was always updated, bypassing the default logic. My changes ensure that default flow control settings are properly honored by shifting the default handling mechanism to recognize an 'empty' proto as the indicator for applying defaults, rather than a null value. This improves the robustness and predictability of flow control behavior.
Highlights
- Flow Control Default Handling: I've updated the
ChannelCache'scurrentFlowControlSettingsinitialization. Previously, it was initialized tonull, which led to incorrect default handling. Now, it's initialized toUserWorkerGrpcFlowControlSettings.getDefaultInstance(). This change ensures that the system correctly recognizes and applies default flow control settings when no specific configuration has been provided, resolving a bug where defaults were never effectively used. - Logic for Applying Defaults: In the
resolveFlowControlSettingsmethod withinChannelCache, I've modified the condition for determining whether to apply default settings. Instead of checking forcurrentFlowControlSettings == null, the logic now checks ifcurrentFlowControlSettings.equals(UserWorkerGrpcFlowControlSettings.getDefaultInstance()). This aligns the default handling with the new initialization strategy, ensuring that an 'empty' proto is treated as a signal to use the system's defaults. - Redundant Null Check Removal: I've removed a redundant
currentFlowControlSettings == nullcheck from theconsumeFlowControlSettingsmethod. Since thecurrentFlowControlSettingsfield is now always initialized to a non-null default instance, this check is no longer necessary and simplifies the code. - Unit Testing: I've added two new unit tests,
testConsumeFlowControlSettings_UsesDefaultOverridesForDirectandtestConsumeFlowControlSettings_UsesDefaultOverridesForCloudPath, toChannelCacheTest. These tests specifically verify that the default flow control settings are correctly applied and that channels are reloaded as expected when custom settings are provided or when settings are reset to an empty proto, ensuring the fix behaves as intended for both direct and cloud paths.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
|
remaining failure is unrelated. |
The previous logic only used the default before any config update was issued. However we always update the config, so this never used the default. Fixed by changing default handling to be based upon empty proto instead of null.
The added unit test fails with the previous logic.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.