Support multiple formats of Azure region names for CosmosClientOptions #3857

pradeep-chellappan · 2023-05-18T09:14:16Z

Description

This is a proposed fix for - #2330
We want to set CosmosClientOptions.ApplicationRegion and CosmosClientOptions.ApplicationPreferredRegions in the format that ARM exposes it. For e.g. if the region is West US 2, the CosmosClientOptions should accept both "West US 2" and "WestUs2" and "westus2". This gives maximum flexibility to the users of the SDK.
To enable this, I keep a mapping between the normalized region name and the one that CosmosDB SDK uses internally. When CosmosClientOptions.ApplicationRegion or CosmosClientOptions.ApplicationPreferredRegions is set, the region names are converted, if required.

Type of change

Please delete options that are not relevant.

[] New feature (non-breaking change which adds functionality)

Closing issues

To automatically close an issue: closes #2330

…ferredRegions in multiple region name formats. This is a proposed fix for - Azure#2330

pradeep-chellappan · 2023-05-18T09:17:41Z

@microsoft-github-policy-service agree company="DocuSign"

Microsoft.Azure.Cosmos/src/RegionNameMapping.cs

kirankumarkolli

Please check comments

…eep-chellappan/azure-cosmos-dotnet-v3 into pradeepc/regionname-mapping

sourabh1007 · 2023-05-22T15:25:30Z

Microsoft.Azure.Cosmos/src/RegionNameMapping.cs

+
+            foreach (FieldInfo field in fields)
+            {
+                normalizedToCosmosDBRegionNameMapping[field.Name.ToLowerInvariant()] = field.GetValue(null).ToString();


how did we make sure that it is applicable for all the regions? Is other SDKs are following this logic? or it is just observation and relying on adding condition if we found/add any new region which is not following this pattern (even in future)?

It is applicable to all regions where CosmosDB currently runs (based on the Regions class).

@sourabh1007 its possible that some other azure RP's returning/expecting in different form.
One example that was mentioned with "-" by @FabianMeiswinkel

We will learn more as we uncover and fill the gaps forward

kirankumarkolli · 2023-06-09T14:51:47Z

Microsoft.Azure.Cosmos/src/RegionNameMapping.cs

+    /// </summary>
+    internal class RegionNameMapping
+    {
+        private static readonly IDictionary<string, string> normalizedToCosmosDBRegionNameMapping;


Usage is one at the client initialization level. Caching is unnecessary.

Sorry, but what is the caching that you are referring to?

@kirankumarkolli - I did not understand your comment. Could you please clarify?

Static dict<> lifetime is process lifetime. Where as the usage of the dict<> is just one method call at the ClientInitialization time only.

So static instance/cache is unnecessary, it can be created once as local state.

kirankumarkolli · 2023-06-14T04:08:22Z

@pradeep-chellappan can you please take a look at comments?

kirankumarkolli · 2023-07-11T16:57:12Z

@pradeep-chellappan Can you please look into the comments.

We want to ship this feature ASAP. We will wait for your update till 07/14 and after that we will move forward with a new PR replicating the changes.

pradeep-chellappan · 2023-07-12T00:16:41Z

@pradeep-chellappan Can you please look into the comments.

We want to ship this feature ASAP. We will wait for your update till 07/14 and after that we will move forward with a new PR replicating the changes.

Hey Kiran, could you please reply to my prior comment regarding the caching?

kirankumarkolli · 2023-07-12T23:29:40Z

Microsoft.Azure.Cosmos/src/RegionNameMapping.cs

+
+        static RegionNameMapping()
+        {
+            FieldInfo[] fields = typeof(Regions).GetFields(BindingFlags.Public | BindingFlags.Static);


@ealsur how about include all (non-public) also?

Its more about future proofing where the region might be added as internal before going public. And string/text based API's will still be functional right?

We don't have cases where "Regions" contains internal names. We have cases where "LocationNames" (which is internal itself) contains regions that are not ready though. When they are not ready, there is no mapping in RegionProximityUtil either.
So adding also Internals here would not enable any scenario.

kirankumarkolli · 2023-07-12T23:33:14Z

Microsoft.Azure.Cosmos/src/CosmosClientOptions.cs

+        public string ApplicationRegion
+        {
+            get => this.applicationRegion;
+            set => this.applicationRegion = RegionNameMapping.GetCosmosDBRegionName(value);


Application input is updated right on setter. Functionally works.
One other alternative is to keep original values as Application configured and then later during CosmosClient initialization do conversation.

@ealsur, @FabianMeiswinkel any preferences?

I would prefer also having the original UserAgent input in the diagnostics - so, both the normalized and original values. Keeping the original value in ApplicationRegion and then client on initialization normalizing it is porbably the easiest way to get above.

No preference. On the Diagnostics we log the value of the public getter:

azure-cosmos-dotnet-v3/Microsoft.Azure.Cosmos/src/Tracing/TraceData/ClientConfigurationTraceDatum.cs

Lines 28 to 29 in d7fc282

this.ConsistencyConfig = new ConsistencyConfig(cosmosClientContext.ClientOptions.ConsistencyLevel,

cosmosClientContext.ClientOptions.ApplicationPreferredRegions, cosmosClientContext.ClientOptions.ApplicationRegion);

The only thing with this approach is that the getter does not return the same value that was set, so if anyone is validating that if I set West US, I get West US, the test will fail.

Making the conversion internally on:

azure-cosmos-dotnet-v3/Microsoft.Azure.Cosmos/src/CosmosClientOptions.cs

Lines 782 to 790 in d7fc282

if (this.ApplicationRegion != null)

{

connectionPolicy.SetCurrentLocation(this.ApplicationRegion);

}

if (this.ApplicationPreferredRegions != null)

{

connectionPolicy.SetPreferredLocations(this.ApplicationPreferredRegions);

}

Would achieve the same thing while keeping the getter value matching the user input.

kirankumarkolli · 2023-07-12T23:35:16Z

Microsoft.Azure.Cosmos/src/RegionNameMapping.cs

+
+            foreach (FieldInfo field in fields)
+            {
+                normalizedToCosmosDBRegionNameMapping[field.Name.ToLowerInvariant()] = field.GetValue(null).ToString();


Coverage for spaces" Ref: #2330 (comment)

I.e. if the incoming name doesn't have spaces also it should match.

kirankumarkolli · 2023-07-12T23:36:28Z

@pradeep-chellappan Can you please look into the comments.
We want to ship this feature ASAP. We will wait for your update till 07/14 and after that we will move forward with a new PR replicating the changes.

Hey Kiran, could you please reply to my prior comment regarding the caching?

Thanks you @pradeep-chellappan, please check updated comments.

ealsur · 2023-10-11T18:53:30Z

Closed through #4016

Allow ComosClientOptions to take ApplicationRegion and ApplicationPre…

d2bc95d

…ferredRegions in multiple region name formats. This is a proposed fix for - Azure#2330

pradeep-chellappan requested review from khdang, sboshra, adityasa, neildsh, kirankumarkolli, ealsur, FabianMeiswinkel and kirillg as code owners May 18, 2023 09:14

kirankumarkolli reviewed May 18, 2023

View reviewed changes

Microsoft.Azure.Cosmos/src/RegionNameMapping.cs Outdated Show resolved Hide resolved

kirankumarkolli reviewed May 18, 2023

View reviewed changes

Microsoft.Azure.Cosmos/src/RegionNameMapping.cs Show resolved Hide resolved

kirankumarkolli requested changes May 18, 2023

View reviewed changes

pradeep-chellappan and others added 3 commits May 18, 2023 10:13

Merge branch 'master' into pradeepc/regionname-mapping

855e1ad

Address PR comment to avoid duplicating list of names.

7abed77

Merge branch 'pradeepc/regionname-mapping' of https://github.com/prad…

91c5a62

…eep-chellappan/azure-cosmos-dotnet-v3 into pradeepc/regionname-mapping

sourabh1007 reviewed May 22, 2023

View reviewed changes

kirankumarkolli reviewed Jun 9, 2023

View reviewed changes

kirankumarkolli reviewed Jul 12, 2023

View reviewed changes

ealsur mentioned this pull request Jul 13, 2023

Adding GetNormalizedRegionName to fix #2330 #3746

Closed

4 tasks

iainx self-assigned this Jul 31, 2023

iainx mentioned this pull request Aug 4, 2023

CosmosClientOptions: Adds support for multiple formats of Azure region names #4016

Merged

ealsur closed this Oct 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support multiple formats of Azure region names for CosmosClientOptions #3857

Support multiple formats of Azure region names for CosmosClientOptions #3857

pradeep-chellappan commented May 18, 2023

pradeep-chellappan commented May 18, 2023

kirankumarkolli left a comment

sourabh1007 May 22, 2023 •

edited

Loading

pradeep-chellappan May 22, 2023

kirankumarkolli Jun 9, 2023

kirankumarkolli Jun 9, 2023

pradeep-chellappan Jun 14, 2023

pradeep-chellappan Jul 12, 2023

kirankumarkolli Jul 12, 2023

kirankumarkolli commented Jun 14, 2023

kirankumarkolli commented Jul 11, 2023

pradeep-chellappan commented Jul 12, 2023

kirankumarkolli Jul 12, 2023

ealsur Jul 13, 2023

kirankumarkolli Jul 12, 2023 •

edited

Loading

FabianMeiswinkel Jul 13, 2023

ealsur Jul 13, 2023

kirankumarkolli Jul 12, 2023

kirankumarkolli commented Jul 12, 2023

ealsur commented Oct 11, 2023

	this.ConsistencyConfig = new ConsistencyConfig(cosmosClientContext.ClientOptions.ConsistencyLevel,
	cosmosClientContext.ClientOptions.ApplicationPreferredRegions, cosmosClientContext.ClientOptions.ApplicationRegion);

	if (this.ApplicationRegion != null)
	{
	connectionPolicy.SetCurrentLocation(this.ApplicationRegion);
	}

	if (this.ApplicationPreferredRegions != null)
	{
	connectionPolicy.SetPreferredLocations(this.ApplicationPreferredRegions);
	}

Support multiple formats of Azure region names for CosmosClientOptions #3857

Support multiple formats of Azure region names for CosmosClientOptions #3857

Conversation

pradeep-chellappan commented May 18, 2023

Description

Type of change

Closing issues

pradeep-chellappan commented May 18, 2023

kirankumarkolli left a comment

Choose a reason for hiding this comment

sourabh1007 May 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirankumarkolli commented Jun 14, 2023

kirankumarkolli commented Jul 11, 2023

pradeep-chellappan commented Jul 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirankumarkolli Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirankumarkolli commented Jul 12, 2023

ealsur commented Oct 11, 2023

sourabh1007 May 22, 2023 •

edited

Loading

kirankumarkolli Jul 12, 2023 •

edited

Loading