-
Notifications
You must be signed in to change notification settings - Fork 1
feat: Add ReadFrom parsing support to ConfigurationOptions for AZ Affinity #28
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
Open
jbrinkman
wants to merge
20
commits into
main
Choose a base branch
from
jbrinkman/az-affinity-configurationoptions-support
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
feat: Add ReadFrom parsing support to ConfigurationOptions for AZ Affinity #28
jbrinkman
wants to merge
20
commits into
main
from
jbrinkman/az-affinity-configurationoptions-support
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
e0d1212
to
ae81ad5
Compare
tests/Valkey.Glide.IntegrationTests/ConnectionMultiplexerReadFromMappingTests.cs
Outdated
Show resolved
Hide resolved
tests/Valkey.Glide.IntegrationTests/ReadFromEndToEndIntegrationTests.cs
Outdated
Show resolved
Hide resolved
tests/Valkey.Glide.IntegrationTests/ReadFromEndToEndIntegrationTests.cs
Outdated
Show resolved
Hide resolved
tests/Valkey.Glide.IntegrationTests/ReadFromEndToEndIntegrationTests.cs
Outdated
Show resolved
Hide resolved
tests/Valkey.Glide.UnitTests/ConfigurationOptionsReadFromTests.cs
Outdated
Show resolved
Hide resolved
tests/Valkey.Glide.UnitTests/ConfigurationOptionsReadFromTests.cs
Outdated
Show resolved
Hide resolved
ae81ad5
to
353265e
Compare
- Implement ParseReadFromStrategy method to convert string values to ReadFromStrategy enum - Add validation for ReadFrom strategy and AZ parameter combinations - Extend DoParse method to handle readFrom and az parameters from connection strings - Add comprehensive error handling with descriptive messages - Support case-insensitive parsing for all ReadFrom strategies - Extend ToString method to include ReadFrom and AZ in connection strings - Add comprehensive unit tests covering all parsing scenarios - Reorganize private fields to follow C# analyzer rules Addresses task 1 of GitHub issue #26 Satisfies requirements: 1.1, 1.2, 1.5, 1.6, 4.1, 4.2, 4.3, 4.4, 4.5 Signed-off-by: jbrinkman <[email protected]>
- Add FormatReadFrom method to convert ReadFrom struct to string representation - Extract FormatReadFromStrategy method as proper private method - Refactor ToString method to use new FormatReadFrom method - Ensure proper formatting for all ReadFromStrategy values - Maintain backward compatibility with existing ToString format Addresses task 2 of AZ affinity support implementation Signed-off-by: jbrinkman <[email protected]>
…parameters - Add readFrom case to the switch statement in DoParse method - Add az case to the switch statement in DoParse method - Implement proper parsing logic that creates ReadFrom struct from parsed values - Add validation during parsing to catch invalid combinations early - Support case-insensitive parsing of ReadFrom strategy values - Validate AZ parameter combinations with ReadFrom strategies Addresses task 4 from AZ affinity support specification Signed-off-by: jbrinkman <[email protected]>
- Add ConnectionMultiplexerReadFromMappingTests.cs with 12 test cases - Verify ReadFrom configuration mapping from ConfigurationOptions to ClientConfigurationBuilder - Test all ReadFromStrategy values (Primary, PreferReplica, AzAffinity, AzAffinityReplicasAndPrimary) - Verify null ReadFrom handling for backward compatibility - Test both standalone and cluster client configurations - Confirm end-to-end flow from ConfigurationOptions to ConnectionConfig Addresses task 5: Verify ConnectionMultiplexer ReadFrom mapping Requirements: 3.1, 3.2, 3.4, 6.1 Signed-off-by: jbrinkman <[email protected]>
…serialization - Add ToString output tests for each ReadFromStrategy value - Add round-trip parsing tests (Parse → ToString → Parse) - Add backward compatibility tests with existing configuration strings - Add proper AZ formatting tests in ToString output - Covers requirements 5.1-5.5 and 6.2-6.3 from AZ affinity support spec All 57 tests pass, ensuring proper serialization and deserialization of ReadFrom configurations while maintaining backward compatibility. Signed-off-by: jbrinkman <[email protected]>
…property validation - Add ReadFrom property setter validation tests for all strategies - Add Clone method ReadFrom preservation tests with comprehensive scenarios - Add null ReadFrom handling and default behavior tests - Add cross-validation tests between ReadFromStrategy and AZ parameters - Ensure proper error handling for invalid configurations - Verify independence of cloned instances - Test complex configuration scenarios with ReadFrom Addresses requirements 2.1, 2.2, 2.3, 2.4, 2.5, and 6.4 from AZ affinity support spec Signed-off-by: jbrinkman <[email protected]>
…g functionality Signed-off-by: jbrinkman <[email protected]>
…rom configuration - Add ReadFromEndToEndIntegrationTests with 48 test cases covering complete pipeline - Test connection string to FFI layer flow for all ReadFromStrategy values - Test ConfigurationOptions programmatic configuration flow - Test error handling throughout the complete configuration pipeline - Test round-trip serialization (Parse → ToString → Parse) integrity - Test backward compatibility with legacy configurations - Test performance scenarios with concurrent connections - Test FFI layer integration to verify configuration reaches Rust core - Verify AZ affinity settings are properly passed to the Rust core - Cover both standalone and cluster client scenarios - Fix test case for invalid ReadFrom strategy to avoid parsing ambiguity Implements task 12 from AZ Affinity support implementation plan. Requirements covered: 3.1, 3.2, 3.3, 3.4, 3.5 Signed-off-by: jbrinkman <[email protected]>
Signed-off-by: jbrinkman <[email protected]>
…iplexer - Updated exception message assertion in ConfigurationOptionsReadFromTests to be case insensitive. - Replaced reflection-based method invocation with direct calls to CreateClientConfigBuilder in ConnectionMultiplexerReadFromMappingTests for better readability and performance. - Added new tests to verify ReadFrom configuration flows correctly to ConnectionConfig for both standalone and cluster configurations. - Removed unnecessary reflection helper method to streamline the test code. Signed-off-by: jbrinkman <[email protected]>
Signed-off-by: jbrinkman <[email protected]>
5f74a52
to
c32220b
Compare
…onMultiplexerReadFromMappingTests to use explicit type declarations for variable assignments, enhancing code clarity and consistency. Signed-off-by: jbrinkman <[email protected]>
…readability Signed-off-by: jbrinkman <[email protected]>
29 tasks
…figurationOptions Signed-off-by: jbrinkman <[email protected]>
…rategy type Signed-off-by: jbrinkman <[email protected]>
…figurationOptions Signed-off-by: jbrinkman <[email protected]>
…FromTests for clarity Signed-off-by: jbrinkman <[email protected]>
edlng
approved these changes
Aug 16, 2025
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.
LGTM! I do agree with Yury's comment on cutting down the tests^
tests/Valkey.Glide.IntegrationTests/ReadFromEndToEndIntegrationTests.cs
Outdated
Show resolved
Hide resolved
tests/Valkey.Glide.UnitTests/ConfigurationOptionsReadFromTests.cs
Outdated
Show resolved
Hide resolved
tests/Valkey.Glide.UnitTests/ConfigurationOptionsReadFromTests.cs
Outdated
Show resolved
Hide resolved
tests/Valkey.Glide.UnitTests/ConfigurationOptionsReadFromTests.cs
Outdated
Show resolved
Hide resolved
jduo
reviewed
Aug 19, 2025
jduo
reviewed
Aug 19, 2025
jduo
approved these changes
Aug 19, 2025
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
I added true e2e tests, few tests were simplified and moved to UT. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR implements comprehensive ReadFrom parsing support in ConfigurationOptions to enable AZ (Availability Zone) affinity configuration through connection strings and programmatic configuration. This addresses issue #26 by providing a complete implementation of ReadFrom strategy parsing, validation, and serialization.
Changes Made
Core Implementation
readFrom
andaz
parameters from connection stringsSupported ReadFrom Strategies
Primary
- Always read from primary nodesPreferReplica
- Prefer replica nodes, fallback to primaryAzAffinity
- Prefer replicas in the same AZ, with fallbackAzAffinityReplicasAndPrimary
- Prefer nodes in the same AZ (replicas first, then primary), with fallbackConnection String Examples
Test Coverage
Technical Details
Implementation Approach
DoParse
method to handlereadFrom
andaz
parametersValidateAndSetReadFrom
with comprehensive strategy/AZ validationFormatReadFrom
method for ToString output with proper formattingValidation Rules
Primary
andPreferReplica
strategies cannot have AZ parametersAzAffinity
andAzAffinityReplicasAndPrimary
strategies require non-empty AZ parametersBackward Compatibility
Testing
All tests pass including:
Closes
Closes #26
Checklist