-
Notifications
You must be signed in to change notification settings - Fork 494
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
CosmosClientBuilder: Add preferred regions and public internal func WithConnectionModeDirect() #1397
CosmosClientBuilder: Add preferred regions and public internal func WithConnectionModeDirect() #1397
Conversation
2. Make the function to build CosmosClient by customize connection configs from internal to public
/// <seealso cref="CosmosClientOptions.ApplicationPreferredRegions"/> | ||
public CosmosClientBuilder WithApplicationPreferredRegions(IReadOnlyList<string> applicationPreferredRegions) | ||
{ | ||
this.clientOptions.ApplicationPreferredRegions = applicationPreferredRegions; |
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.
We need to make a deep copy of the list, since the list itself is mutable.
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.
Either that or use ImmutableList.
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.
No need, the list is copied when the ConnectionPolicy is built. https://github.com/Azure/azure-cosmos-dotnet-v3/blob/master/Microsoft.Azure.Cosmos/src/ConnectionPolicy.cs#L73
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.
Since @bchong95 brought this topic, do you guys think it's necessary to force the input type as ImmutableList? Since it will ensure the thread safe. I might over thinking but it's possible the list will change during add process?
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.
Internally the list that is passed is iterated when you build the Client instance and not referenced, so modifying the variable after bulding the client has no effect.
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.
What's the contract on ClientOptions. If its IReadOnly then won't it also have the same issue?
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.
CosmosClientOptions's contract is public IReadOnlyList<string> ApplicationPreferredRegions { get; set; }
. Like I mentioned, once the Client is built, we don't maintain a reference to the variable, so modifying the variable does not alter the Client 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.
@ealsur lets please track this feedback for V4.
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.
You need to run the unit tests locally and update the public API file.
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
…dPrefferedRegionsConfig
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
Description
When we migrate our app from sdk v2 to sdk v3, we need a way to set the EnableTcpConnectionEndpointRediscovery and ApplicationPreferredRegions on CosmosClientOptions by CosmosClientBuilder.
The reason we need create CosmosClientBuilder is due to we need utilize the internal encryption feature.
Type of change
1395
Put closes #1395 .
lingdaLi