-
Notifications
You must be signed in to change notification settings - Fork 2
feat(pubsub): implement core PubSub framework infrastructure #103
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
18
commits into
main
Choose a base branch
from
jbrinkman/pubsub-core
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
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
9dfa51d
feat(pubsub): implement core PubSub infrastructure and FFI integration
jbrinkman 4cbd22a
feat(rust): add PubSub FFI functions for callback registration and me…
jbrinkman 73a842c
feat(rust): implement proper PubSub callback storage and management
jbrinkman 4697c40
feat: Add comprehensive integration tests for FFI PubSub callback flow
jbrinkman 0a75eea
test(pubsub): Refactor PubSub FFI Callback Integration Tests
jbrinkman 03dc000
refactor(pubsub): Implement instance-based PubSub callback architecture
jbrinkman 0e84419
fix: resolve critical memory leak in PubSub FFI message processing
jbrinkman dd0c85d
feat(pubsub): add thread safety to PubSub handler access in BaseClient
jbrinkman 426e1a9
feat(pubsub): replace Task.Run with channel-based message processing
jbrinkman 069af2d
feat(pubsub): implement graceful shutdown coordination between Rust a…
jbrinkman da86a39
feat(pubsub): Add queue-based message retrieval and comprehensive int…
jbrinkman 13ba6c4
refactor(pubsub): Remove unused PubSubConfigurationExtensions class
jbrinkman 5868588
style: Apply code formatting to PubSub files
jbrinkman fa8bff7
chore(reports): Remove legacy reporting artifacts and unused files
jbrinkman 600b48e
refactor(pubsub): Simplify synchronization primitives in PubSub messa…
jbrinkman d59edfd
fix: Address Lint configuration errors
jbrinkman 60c2c30
fix: enable pattern subscriptions in cluster mode
jbrinkman d2eec0e
test: remove redundant and inaccurate PubSub tests
jbrinkman File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -151,3 +151,7 @@ $RECYCLE.BIN/ | |
| _NCrunch* | ||
|
|
||
| glide-logs/ | ||
|
|
||
| # Test results and coverage reports | ||
| testresults/ | ||
| reports/ | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,170 @@ | ||
| # Configuration Architecture Analysis | ||
|
|
||
| ## Overview | ||
|
|
||
| This document analyzes the configuration architecture in the Valkey.Glide C# client, focusing on the relationship between `ConnectionConfiguration` and `ConfigurationOptions`, and how configuration changes can be made through the `ConnectionMultiplexer`. | ||
|
|
||
| ## Configuration Classes Relationship | ||
|
|
||
| ### ConfigurationOptions | ||
| - **Purpose**: External API configuration class that follows StackExchange.Redis compatibility patterns | ||
| - **Location**: `sources/Valkey.Glide/Abstract/ConfigurationOptions.cs` | ||
| - **Role**: User-facing configuration interface | ||
|
|
||
| ### ConnectionConfiguration | ||
| - **Purpose**: Internal configuration classes that map to the underlying FFI layer | ||
| - **Location**: `sources/Valkey.Glide/ConnectionConfiguration.cs` | ||
| - **Role**: Internal configuration representation and builder pattern implementation | ||
|
|
||
| ## Configuration Flow | ||
|
|
||
| ``` | ||
| ConfigurationOptions → ClientConfigurationBuilder → ConnectionConfig → FFI.ConnectionConfig | ||
| ``` | ||
|
|
||
| 1. **User Input**: `ConfigurationOptions` (external API) | ||
| 2. **Translation**: `ConnectionMultiplexer.CreateClientConfigBuilder<T>()` method | ||
| 3. **Building**: `ClientConfigurationBuilder<T>` (internal) | ||
| 4. **Internal Config**: `ConnectionConfig` record | ||
| 5. **FFI Layer**: `FFI.ConnectionConfig` | ||
|
|
||
| ## Key Components Analysis | ||
|
|
||
| ### ConnectionMultiplexer Configuration Mapping | ||
|
|
||
| The `ConnectionMultiplexer.CreateClientConfigBuilder<T>()` method at line 174 performs the critical translation: | ||
|
|
||
| ```csharp | ||
| internal static T CreateClientConfigBuilder<T>(ConfigurationOptions configuration) | ||
| where T : ClientConfigurationBuilder<T>, new() | ||
| { | ||
| T config = new(); | ||
| foreach (EndPoint ep in configuration.EndPoints) | ||
| { | ||
| config.Addresses += Utils.SplitEndpoint(ep); | ||
| } | ||
| config.UseTls = configuration.Ssl; | ||
| // ... other mappings | ||
| _ = configuration.ReadFrom.HasValue ? config.ReadFrom = configuration.ReadFrom.Value : new(); | ||
| return config; | ||
| } | ||
| ``` | ||
|
|
||
| ### Configuration Builders | ||
|
|
||
| The builder pattern is implemented through: | ||
| - `StandaloneClientConfigurationBuilder` (line 525) | ||
| - `ClusterClientConfigurationBuilder` (line 550) | ||
|
|
||
| Both inherit from `ClientConfigurationBuilder<T>` which provides: | ||
| - Fluent API methods (`WithXxx()`) | ||
| - Property setters | ||
| - Internal `ConnectionConfig Build()` method | ||
|
|
||
| ## Configuration Mutability Analysis | ||
|
|
||
| ### Current State: Immutable After Connection | ||
|
|
||
| **Connection Creation**: Configuration is set once during `ConnectionMultiplexer.ConnectAsync()`: | ||
|
|
||
| ```csharp | ||
| public static async Task<ConnectionMultiplexer> ConnectAsync(ConfigurationOptions configuration, TextWriter? log = null) | ||
| { | ||
| // Configuration is translated and used to create the client | ||
| StandaloneClientConfiguration standaloneConfig = CreateClientConfigBuilder<StandaloneClientConfigurationBuilder>(configuration).Build(); | ||
| // ... connection establishment | ||
| return new(configuration, await Database.Create(config)); | ||
| } | ||
| ``` | ||
|
|
||
| **Storage**: The original `ConfigurationOptions` is stored in `RawConfig` property (line 156): | ||
|
|
||
| ```csharp | ||
| internal ConfigurationOptions RawConfig { private set; get; } | ||
| ``` | ||
|
|
||
| ### Limitations for Runtime Configuration Changes | ||
|
|
||
| 1. **No Reconfiguration API**: `ConnectionMultiplexer` doesn't expose methods to change configuration after connection | ||
| 2. **Immutable Builder Chain**: Once built, the configuration flows to FFI layer and cannot be modified | ||
| 3. **Connection Recreation Required**: Any configuration change requires creating a new `ConnectionMultiplexer` instance | ||
|
|
||
| ## Potential Configuration Change Approaches | ||
|
|
||
| ### 1. Connection Recreation (Current Pattern) | ||
| ```csharp | ||
| // Current approach - requires new connection | ||
| var newConfig = oldConfig.Clone(); | ||
| newConfig.ReadFrom = new ReadFrom(ReadFromStrategy.AzAffinity, "us-west-2"); | ||
| var newMultiplexer = await ConnectionMultiplexer.ConnectAsync(newConfig); | ||
| ``` | ||
|
|
||
| ### 2. Potential Runtime Reconfiguration (Not Currently Implemented) | ||
|
|
||
| To enable runtime configuration changes, the following would need to be implemented: | ||
|
|
||
| ```csharp | ||
| // Hypothetical API | ||
| public async Task ReconfigureAsync(Action<ConfigurationOptions> configure) | ||
| { | ||
| var newConfig = RawConfig.Clone(); | ||
| configure(newConfig); | ||
|
|
||
| // Would need to: | ||
| // 1. Validate configuration changes | ||
| // 2. Update underlying client configuration | ||
| // 3. Potentially recreate connections | ||
| // 4. Update RawConfig | ||
| } | ||
| ``` | ||
|
|
||
| ### 3. Builder Pattern Extension | ||
|
|
||
| A potential approach could extend the builder pattern to support updates: | ||
|
|
||
| ```csharp | ||
| // Hypothetical API | ||
| public async Task<bool> TryUpdateConfigurationAsync<T>(Action<T> configure) | ||
| where T : ClientConfigurationBuilder<T>, new() | ||
| { | ||
| // Create new builder from current configuration | ||
| // Apply changes | ||
| // Validate and apply if possible | ||
| } | ||
| ``` | ||
|
|
||
| ## ReadFrom Configuration Specifics | ||
|
|
||
| ### Current Implementation | ||
| - `ReadFrom` is a struct (line 74) with `ReadFromStrategy` enum and optional AZ string | ||
| - Mapped in `CreateClientConfigBuilder()` at line 199 | ||
| - Flows through to FFI layer via `ConnectionConfig.ToFfi()` method | ||
|
|
||
| ### ReadFrom Change Requirements | ||
| To change `ReadFrom` configuration at runtime would require: | ||
| 1. **API Design**: Method to accept new `ReadFrom` configuration | ||
| 2. **Validation**: Ensure new configuration is compatible with current connection type | ||
| 3. **FFI Updates**: Update the underlying client configuration | ||
| 4. **Connection Management**: Handle any required connection reestablishment | ||
|
|
||
| ## Recommendations | ||
|
|
||
| ### Short Term | ||
| 1. **Document Current Limitations**: Clearly document that configuration changes require connection recreation | ||
| 2. **Helper Methods**: Provide utility methods for common reconfiguration scenarios: | ||
| ```csharp | ||
| public static async Task<ConnectionMultiplexer> RecreateWithReadFromAsync( | ||
| ConnectionMultiplexer current, | ||
| ReadFrom newReadFrom) | ||
| ``` | ||
|
|
||
| ### Long Term | ||
| 1. **Runtime Reconfiguration API**: Implement selective runtime configuration updates for non-disruptive changes | ||
| 2. **Configuration Validation**: Add validation to determine which changes require reconnection vs. runtime updates | ||
| 3. **Connection Pool Management**: Consider connection pooling to minimize disruption during reconfiguration | ||
|
|
||
| ## Conclusion | ||
|
|
||
| Currently, the `ConnectionMultiplexer` does not support runtime configuration changes. The architecture is designed around immutable configuration set at connection time. Any configuration changes, including `ReadFrom` strategy modifications, require creating a new `ConnectionMultiplexer` instance. | ||
|
|
||
| The relationship between `ConfigurationOptions` and `ConnectionConfiguration` is a translation layer where the external API (`ConfigurationOptions`) is converted to internal configuration structures (`ConnectionConfiguration`) that interface with the FFI layer. | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| #!/bin/zsh | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be moved to a scripts directory? We already have a |
||
| # Streams all commands from Valkey and logs them to a file with timestamps in ./log directory. | ||
|
|
||
| LOGDIR="./log" | ||
| LOGFILE="$LOGDIR/valkey-monitor.log" | ||
|
|
||
| # Ensure directory exists | ||
| if [ ! -d "$LOGDIR" ]; then | ||
| mkdir -p "$LOGDIR" | ||
| fi | ||
| # Ensure log file exists | ||
| if [ ! -f "$LOGFILE" ]; then | ||
| touch "$LOGFILE" | ||
| fi | ||
|
|
||
| # Run MONITOR and prepend timestamps using date | ||
| valkey-cli MONITOR | while read -r line; do | ||
| echo "$(date '+[%Y-%m-%d %H:%M:%S]') $line" | ||
| done >> "$LOGFILE" | ||
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
Oops, something went wrong.
Oops, something went wrong.
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.
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.
Nit. Probably best not to reference line number in here, as this is likely to change? Similar elsewhere in this file. (Know you probably didn't write this; should be easy to remove).