feat(cfg): add safe configuration and storage API for MRD connection pool autoscaling#4739
feat(cfg): add safe configuration and storage API for MRD connection pool autoscaling#4739PranjalC100 wants to merge 3 commits into
Conversation
PranjalC100
added
execute-integration-tests
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4739 +/- ##
==========================================
- Coverage 83.69% 83.68% -0.01%
==========================================
Files 168 168
Lines 20782 20796 +14
==========================================
+ Hits 17393 17403 +10
- Misses 2740 2743 +3
- Partials 649 650 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
PranjalC100
force-pushed
the
pr1-mrd-config-storage-api
branch
from
a45d385 to
f043adf
Compare
github-actions
Bot
added
the
remind-reviewers
Summary of ChangesHello, 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 establishes the foundational configuration and validation framework for the Multi-Range Downloader (MRD) connection pool autoscaling feature. By introducing new configuration parameters and updating the storage request interface, this change sets the stage for future implementation of dynamic connection management while ensuring that all new settings are safely validated at the configuration level. Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
Customization To customize the 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 Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces new configuration parameters for Multi-Range Downloader (MRD) autoscaling, including minimum/maximum connections and target pending bytes/ranges, along with their command-line flags, validation logic, and unit tests. The feedback suggests strengthening the validation logic to enforce consistency checks—specifically ensuring that MaxConnections is specified when scaling triggers are set, and that at least one scaling trigger is provided when the connection pool is dynamic—and adding corresponding unit tests for these new validation rules.
|
Hi @abhishek10004, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you! |
PranjalC100
force-pushed
the
pr1-mrd-config-storage-api
branch
from
23776fc to
3016fde
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces configuration parameters (TargetPendingBytes and TargetPendingRanges) for Multi-Range Downloader (MRD) autoscaling, including command-line flags, YAML parameters, validation logic, and unit tests. A bug was identified in the configuration rationalization logic (resolveMrdConfig) where explicitly setting mrd-target-pending-ranges to its default value of 20 would incorrectly trigger the fallback logic for TargetPendingBytes. A cleaner implementation using v.IsSet directly was suggested to resolve this issue.
PranjalC100
force-pushed
the
pr1-mrd-config-storage-api
branch
from
3016fde to
0522490
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new configuration parameters, target-pending-bytes and target-pending-ranges, for the Multi-Range Downloader (MRD) to support multistream autoscaling. These parameters are exposed as hidden command-line flags, integrated into the configuration rationalization and validation processes, and added to the MRD request structure. The review feedback suggests using flagSet.Int64P instead of flagSet.IntP to avoid potential value truncation on 32-bit systems since the underlying struct fields are int64. Additionally, it is recommended to simplify the default value resolution logic in resolveMrdConfig to ensure consistent and predictable behavior when only one of the parameters is configured.
| flagSet.IntP("mrd-target-pending-bytes", "", 10485760, "The target threshold for pending bytes to trigger Multi-Range Downloader autoscaling.") | ||
|
|
||
| if err := flagSet.MarkHidden("mrd-target-pending-bytes"); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| flagSet.IntP("mrd-target-pending-ranges", "", 20, "The target threshold for pending ranges to trigger Multi-Range Downloader autoscaling.") | ||
|
|
||
| if err := flagSet.MarkHidden("mrd-target-pending-ranges"); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
The flags mrd-target-pending-bytes and mrd-target-pending-ranges are defined using flagSet.IntP, which corresponds to an int type. However, the associated fields in the MrdConfig struct (TargetPendingBytes and TargetPendingRanges) are of type int64.
On a 32-bit system, an int is 32 bits, which could lead to value truncation if a user provides a value larger than math.MaxInt32.
To ensure correctness and prevent potential data loss on 32-bit architectures, it's better to use flagSet.Int64P, which corresponds to the int64 type of the struct fields.
| flagSet.IntP("mrd-target-pending-bytes", "", 10485760, "The target threshold for pending bytes to trigger Multi-Range Downloader autoscaling.") | |
| if err := flagSet.MarkHidden("mrd-target-pending-bytes"); err != nil { | |
| return err | |
| } | |
| flagSet.IntP("mrd-target-pending-ranges", "", 20, "The target threshold for pending ranges to trigger Multi-Range Downloader autoscaling.") | |
| if err := flagSet.MarkHidden("mrd-target-pending-ranges"); err != nil { | |
| return err | |
| } | |
| flagSet.Int64P("mrd-target-pending-bytes", "", 10485760, "The target threshold for pending bytes to trigger Multi-Range Downloader autoscaling.") | |
| if err := flagSet.MarkHidden("mrd-target-pending-bytes"); err != nil { | |
| return err | |
| } | |
| flagSet.Int64P("mrd-target-pending-ranges", "", 20, "The target threshold for pending ranges to trigger Multi-Range Downloader autoscaling.") | |
| if err := flagSet.MarkHidden("mrd-target-pending-ranges"); err != nil { | |
| return err | |
| } |
| func resolveMrdConfig(v *viper.Viper, c *MrdConfig) { | ||
| if !v.IsSet("mrd.target-pending-ranges") && c.TargetPendingRanges == 0 { | ||
| if c.TargetPendingBytes == 0 { | ||
| c.TargetPendingRanges = 20 | ||
| } | ||
| } | ||
| if !v.IsSet("mrd.target-pending-bytes") && c.TargetPendingBytes == 0 { | ||
| if c.TargetPendingRanges == 0 || c.TargetPendingRanges == 20 { | ||
| c.TargetPendingBytes = 10 * 1024 * 1024 // 10MB | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic in resolveMrdConfig for setting default values is complex and has an asymmetric behavior that could be confusing for users.
Specifically:
- If a user sets
mrd.target-pending-rangesto its default value (20) but notmrd.target-pending-bytes, thenmrd.target-pending-bytesis automatically set to its default of 10MB. - However, if a user sets
mrd.target-pending-bytesto its default (10MB) but notmrd.target-pending-ranges,mrd.target-pending-rangesremains 0.
This inconsistency is unintuitive. A simpler and more predictable approach is to set both defaults only when neither parameter is configured by the user. This makes the behavior consistent.
Here is a suggested simplification that passes all existing tests and removes the asymmetry:
| func resolveMrdConfig(v *viper.Viper, c *MrdConfig) { | |
| if !v.IsSet("mrd.target-pending-ranges") && c.TargetPendingRanges == 0 { | |
| if c.TargetPendingBytes == 0 { | |
| c.TargetPendingRanges = 20 | |
| } | |
| } | |
| if !v.IsSet("mrd.target-pending-bytes") && c.TargetPendingBytes == 0 { | |
| if c.TargetPendingRanges == 0 || c.TargetPendingRanges == 20 { | |
| c.TargetPendingBytes = 10 * 1024 * 1024 // 10MB | |
| } | |
| } | |
| } | |
| func resolveMrdConfig(v *viper.Viper, c *MrdConfig) { | |
| rangesSet := v.IsSet("mrd.target-pending-ranges") | |
| bytesSet := v.IsSet("mrd.target-pending-bytes") | |
| // If neither is set by the user (e.g. not in config file), and they have | |
| // their zero values, set them to the defaults. This handles cases where | |
| // config is not from flags, which have their own defaults. | |
| if !rangesSet && !bytesSet && c.TargetPendingRanges == 0 && c.TargetPendingBytes == 0 { | |
| c.TargetPendingRanges = 20 | |
| c.TargetPendingBytes = 10 * 1024 * 1024 // 10MB | |
| } | |
| } |
|
Hi @abhishek10004, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you! |
3 similar comments
|
Hi @abhishek10004, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you! |
|
Hi @abhishek10004, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you! |
|
Hi @abhishek10004, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you! |
1 participant
Description
This PR introduces the new configuration parameters, validation logic, and storage layer interface fields for the Multi-Range Downloader (MRD) connection pool autoscaling feature.
Specifically, it adds:
mrd.target-pending-bytes(hidden): The target threshold for pending bytes to trigger autoscaling.mrd.target-pending-ranges(hidden): The target threshold for pending ranges to trigger autoscaling.Note: In this simplified approach, the
min-connectionsandmax-connectionsconfiguration options are not exposed to the user. Instead:min-connectionsdefaults to1internally.max-connectionsdefaults to the user-configuredmrd.pool-size.This ensures that the connection pool autoscaling behavior is controlled safely through the existing
pool-sizeconfig.Default Values
If the user does not explicitly override the settings, GCSFuse defaults to the following autoscaling parameters:
mrd.pool-size(acting as maximum connections):41mrd.target-pending-ranges:20mrd.target-pending-bytes:10MB(10,485,760 bytes)This PR is the first of a multi-part series splitting the overall connection pool autoscaling implementation. It is purely declarative and safe; because these fields are not yet wired up in the downloader wrapper or instance code, they have zero functional runtime impact on existing codepaths.
Link to the issue in case of a bug fix.
b/504489809
Testing details
go build ./...and basic layout validation).cfgpackage unit tests pass successfully, covering the new configuration boundaries and combinations.Any backward incompatible change? If so, please explain.
N/A