Skip to content
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

Microsoft.Extensions.Configuration support for Cosmos Clustering Provider #9204

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

OmnipotentOwl
Copy link

@OmnipotentOwl OmnipotentOwl commented Oct 26, 2024

PR Classification

New feature: Introduce support for Cosmos DB clustering provider for Orleans via Microsoft.Extensions.Configuration.

PR Summary

This pull request supports the Cosmos DB clustering provider for Orleans to be configured via Microsoft.Extensions.Configuration, enabling clustering configurations for both Silo and Client roles.

  • CosmosClusteringProviderBuilder.cs: Implemented the CosmosClusteringProviderBuilder class and registered it as a provider.
  • CosmosClusteringOptions.cs: Introduced CosmosClusteringOptions and a validator class.
  • AssemblyInfo.cs: Added [assembly: InternalsVisibleTo("Tester.AzureUtils")] attribute.
Microsoft Reviewers: Open in CodeFlow

Added a `using` directive for `System.Runtime.CompilerServices` and
an assembly attribute `[assembly: InternalsVisibleTo("Tester.AzureUtils")]`
to make internal types visible to the `Tester.AzureUtils` assembly.
Introduce CosmosClusteringProviderBuilder class implementing IProviderBuilder<ISiloBuilder> and IProviderBuilder<IClientBuilder> interfaces. Register the class as a provider for both "Silo" and "Client" roles. Implement Configure methods to set up Cosmos DB clustering options and client configuration. Import necessary namespaces to support the new functionality.

This adds support for using the Cosmos DB Clustering Provider from Orleans IConfiguration setup.
@OmnipotentOwl
Copy link
Author

@dotnet-policy-service agree

@ReubenBond
Copy link
Member

Is this ready for review?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

@OmnipotentOwl
Copy link
Author

@ReubenBond I believe it is, however I would want to run the pipelines again to ensure there aren't any hidden issues.

Added a `using` directive for `System.Runtime.CompilerServices` and
an assembly attribute `[assembly: InternalsVisibleTo("Tester.AzureUtils")]`
to make internal types visible to the `Tester.AzureUtils` assembly.
Introduce CosmosClusteringProviderBuilder class implementing IProviderBuilder<ISiloBuilder> and IProviderBuilder<IClientBuilder> interfaces. Register the class as a provider for both "Silo" and "Client" roles. Implement Configure methods to set up Cosmos DB clustering options and client configuration. Import necessary namespaces to support the new functionality.

This adds support for using the Cosmos DB Clustering Provider from Orleans IConfiguration setup.
@ReubenBond ReubenBond force-pushed the feature/cosmos-cluster-iconfiguration branch from bf98daf to ff3874c Compare February 13, 2025 17:28
@ReubenBond
Copy link
Member

@OmnipotentOwl I rebased and am re-running workflows. Ideally, we should update the tests to use this new configuration mechanism

Replaced "Cosmos" with "AzureCosmosDB" in the
assembly registrations for the `CosmosClusteringProviderBuilder`
for both "Silo" and "Client" configurations to better reflect the
configuration values used by Aspire.
@OmnipotentOwl OmnipotentOwl marked this pull request as ready for review February 23, 2025 04:27
@OmnipotentOwl
Copy link
Author

@ReubenBond, I did some local testing with Aspire with the clustering configuration, and it looks to work mostly for the Cosmos DB emulator (next). I did have to adjust the assembly: RegisterProvider name property based on what I saw in Aspire, but if you feel like we shouldn't rename that or name it just 'Cosmos,' then we might need to do a shim in Aspire to handle that difference.

I am going to see if I can run it tomorrow against a real Cosmos DB instance and if that looks good I may have another PR here shortly for the storage provider.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants