-
Notifications
You must be signed in to change notification settings - Fork 2k
[ENH] Multi-region, multi-cloud configuration. #6092
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
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
|
It introduces a dedicated topology module in Key Changes• Add Affected Areas• This summary was automatically generated by @propel-code-bot |
9bdda62 to
5ba5807
Compare
26bc4bf to
adfd6a8
Compare
| println!( | ||
| "region_name_empty error: {:?}", | ||
| result.as_ref().unwrap_err() | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Maintainability] Consider removing the println! statements from the tests that check for validation errors. They were likely added for debugging and can clutter the CI logs.
This applies to this test and several others that follow the same pattern (e.g., region_name_too_long, region_name_non_ascii, topology_name_empty, etc.).
Context for Agents
Consider removing the `println!` statements from the tests that check for validation errors. They were likely added for debugging and can clutter the CI logs.
This applies to this test and several others that follow the same pattern (e.g., `region_name_too_long`, `region_name_non_ascii`, `topology_name_empty`, etc.).
File: rust/types/src/topology.rs
Line: 1305038ac2e to
ded3a45
Compare
d244e1d to
f6b3b95
Compare
ded3a45 to
82b5763
Compare
| /// The name of the preferred region for operations with region affinity. | ||
| preferred: RegionName, | ||
| /// The set of provider regions available in this configuration. | ||
| regions: Vec<ProviderRegion<T>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't topologies below already contain the regions? what is this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Topologies contains the listing of regions. This is per-region state. So, e.g. we can bind T to a Spanner+Storage config and specify it once per region.
f6b3b95 to
a1380cf
Compare
82b5763 to
b90737d
Compare
| use thiserror::Error; | ||
|
|
||
| /// Maximum length for region and topology names. | ||
| const MAX_NAME_LENGTH: usize = 32; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Requirements] The MAX_NAME_LENGTH of 32 seems potentially restrictive. While it covers standard cloud provider region names, it might be too short for custom or on-premise deployments where more descriptive names are common. For example, a name like "corporate-datacenter-building-a-rack-5" would exceed this limit.
To provide more flexibility, consider increasing this limit. A value like 64 would align better with common identifier length limits in various systems (e.g., DNS labels are up to 63 characters) and would be less likely to cause issues for users with custom naming schemes.
Context for Agents
The `MAX_NAME_LENGTH` of 32 seems potentially restrictive. While it covers standard cloud provider region names, it might be too short for custom or on-premise deployments where more descriptive names are common. For example, a name like "corporate-datacenter-building-a-rack-5" would exceed this limit.
To provide more flexibility, consider increasing this limit. A value like 64 would align better with common identifier length limits in various systems (e.g., DNS labels are up to 63 characters) and would be less likely to cause issues for users with custom naming schemes.
File: rust/types/src/topology.rs
Line: 87There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm keeping 32 for now as we can always up it.
b90737d to
4052be6
Compare
Description of changes
This PR introduces the types for multi-region, multi-cloud support.
Test plan
CI
Migration plan
Not applicable yet.
Observability plan
N/A
Documentation Changes
N/A