-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Add client-level excluded regions and make it mutable. #36616
Add client-level excluded regions and make it mutable. #36616
Conversation
API change check APIView has identified API level changes in this PR and created following API reviews. |
…namicReconfigurationOfProperties
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/CosmosClientBuilder.java
Outdated
Show resolved
Hide resolved
/azp run java - cosmos - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
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 with some questions, thanks @jeet1995 for the changes!
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/CosmosExcludedRegions.java
Show resolved
Hide resolved
...cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/routing/LocationCache.java
Outdated
Show resolved
Hide resolved
…namicReconfigurationOfProperties # Conflicts: # sdk/cosmos/azure-cosmos/CHANGELOG.md
/azp run java - cosmos - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run java - cosmos - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
* */ | ||
public Set<String> getExcludedRegions() { | ||
|
||
if (this.excludedRegions == null || this.excludedRegions.isEmpty()) { |
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.
I guess you clone the Set here to avoid allwoign modifications on it?
For perf reasons ou internal API shoudl avoid the cloning - either via internal property getter that returns the raw set or by creating an ImmutableSet in the ctor when you clone the Set passed into the struct.
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/CosmosAsyncClient.java
Show resolved
Hide resolved
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/CosmosAsyncClient.java
Show resolved
Hide resolved
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/CosmosClientBuilder.java
Show resolved
Hide resolved
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/ConnectionPolicy.java
Show resolved
Hide resolved
LGTM except for the comment around why we have to create new Sets all the time. |
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!
Description
This PR introduces a way to inject a set of excluded regions at the client-level through a custom
Supplier
type.Configuring the
Supplier
Configure the
Supplier
withCosmosClientBuilder
.excludedRegionsSupplier()
should implementSupplier<CosmosExcludedRegions>
.CosmosExcludedRegions
class structure