-
Notifications
You must be signed in to change notification settings - Fork 55
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
Added Domain support for stream mirroring and sourcing and KV full support for the same. #631
Conversation
Due to the difficulty of automating leaf node testing below are examples of tests/usage of mirroring and stream sourcing.
|
Object stores don't seem to support mirroring so this has not been included as previously discussed. |
Deleted the 'System.Diagnostics.Metrics' import as it was not being used anywhere in the file. This change helps in maintaining a clean codebase with only necessary dependencies.
Deleted an unnecessary directive for System.Xml.Schema in NatsJSContext.cs. This cleanup aids in maintaining clean and efficient code.
Clean up redundant initializations and streamline the handling of `subjects`, `mirror`, and `sources` to improve code clarity. Ensure default assignments are explicitly defined within conditional branches for better code maintainability.
Simplify the mirror object instantiation by using the ShallowCopy method, reducing code redundancy. This change improves readability and maintenance by encapsulating the cloning logic within the method.
Updated the conditional check for `config.Sources` to use pattern matching, improving readability and adhering to modern C# conventions. This change ensures cleaner and more maintainable code.
This commit updates the KeyValueStoreTest to skip the Test_CombinedSources test for NATS server versions earlier than 2.10 since some of the mirroring features are introduced with 2.10. It also removes unnecessary retry delay and timeout parameters from the test configuration.
|
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 thanks @darkwatchuk
@rickdotnet @Jarema I wouldn't mind an extra LGTM here please since I did make a few changes myself as well 😅 |
Your changes LGTM, @mtmk. The Could also get a shallow copy from records using |
hehe true. we can write more tests for it maybe?
ah that's a good point. do you think we should use that to keep models clean? @darkwatchuk and I had a quick discussion about keeping them clean as well. |
That'd be a by-product, but essentially. If we had a mixed bag of classes/records and wanted a consistent experience, I'd opt to keep the mirror = config.Mirror.ShallowCopy(); vs mirror = config.Mirror with { }; Both feel good to me. |
I think |
Nice, looks good to me! |
Looks like we're missing some of reference implementation details here: cc @Jarema |
Double checked the implementation against nats-io/nats.go#1112 (with latest changes in main) Looks good. Only, some of the mirror bucket mapping seems to be missing which is captured in #642 and can be handled in a follow up PR |
* Add placement configuration to key-value and object store (#650) * Bump System.Text.Json from 8.0.4 to 8.0.5 in /src/NATS.Client.Core (#649) * Add other client extensions (#637) * Add Domain support for stream mirroring and sourcing and KV full support for the same. (#631) * Add type-forwarders NET 5.0+ (#641) * Add JetStream NATS Client Extensions (#598)
* Add placement configuration to key-value and object store (#650) * Bump System.Text.Json from 8.0.4 to 8.0.5 in /src/NATS.Client.Core (#649) * Add other client extensions (#637) * Add Domain support for stream mirroring and sourcing and KV full support for the same. (#631) * Add type-forwarders NET 5.0+ (#641) * Add JetStream NATS Client Extensions (#598)
Added Sources and Mirror to KV Store Creation
fixes #494
PS nats-io/nats.go#1112