-
Notifications
You must be signed in to change notification settings - Fork 533
[Per Partition Automatic Failover] Remove Environment Variable to Set PPAF at the SDK Layer and Add Support for Internal Client Options #5287
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
Changes from 2 commits
2026d8b
c5b40ca
98fb329
6a1cfb9
f8ce129
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -602,10 +602,16 @@ public virtual void InitializeAccountPropertiesAndStartBackgroundRefresh(Account | |
| return; | ||
| } | ||
|
|
||
| bool isPPafEnabled = ConfigurationManager.IsPartitionLevelFailoverEnabled(defaultValue: false); | ||
| if (databaseAccount.EnablePartitionLevelFailover.HasValue) | ||
| { | ||
| isPPafEnabled = databaseAccount.EnablePartitionLevelFailover.Value; | ||
| bool isPPafEnabled = false; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be simplified as:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Simplified the PPAF logic as suggested by removing the local variable and using direct assignment. f8ce129 |
||
| if (databaseAccount.EnablePartitionLevelFailover.HasValue) | ||
| { | ||
| isPPafEnabled = databaseAccount.EnablePartitionLevelFailover.Value; | ||
| } | ||
|
|
||
| // Apply the DisablePartitionLevelFailover setting to override PPAF if explicitly disabled | ||
| if (this.connectionPolicy.DisablePartitionLevelFailover) | ||
| { | ||
| isPPafEnabled = false; | ||
| } | ||
|
|
||
| this.connectionPolicy.EnablePartitionLevelFailover = isPPafEnabled; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| //------------------------------------------------------------ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| //------------------------------------------------------------ | ||
|
|
||
| namespace Microsoft.Azure.Cosmos.Tests | ||
| { | ||
| using System; | ||
| using Microsoft.VisualStudio.TestTools.UnitTesting; | ||
|
|
||
| [TestClass] | ||
| public class ConfigurationManagerTests | ||
| { | ||
| [TestMethod] | ||
| public void GetCircuitBreakerTimeoutCounterResetWindowInMinutes_DefaultValue() | ||
| { | ||
| // Test that the default value is returned when environment variable is not set | ||
| int result = ConfigurationManager.GetCircuitBreakerTimeoutCounterResetWindowInMinutes(5); | ||
| Assert.AreEqual(5, result); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| public void GetCircuitBreakerTimeoutCounterResetWindowInMinutes_CustomDefaultValue() | ||
| { | ||
| // Test that custom default values are respected | ||
| int result = ConfigurationManager.GetCircuitBreakerTimeoutCounterResetWindowInMinutes(10); | ||
| Assert.AreEqual(10, result); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| public void GetCircuitBreakerTimeoutCounterResetWindowInMinutes_EnvironmentVariableOverride() | ||
| { | ||
| // Test that environment variable overrides the default value | ||
| const string envVarName = "AZURE_COSMOS_PPCB_TIMEOUT_COUNTER_RESET_WINDOW_IN_MINUTES"; | ||
| const string testValue = "15"; | ||
|
|
||
| try | ||
| { | ||
| Environment.SetEnvironmentVariable(envVarName, testValue); | ||
| int result = ConfigurationManager.GetCircuitBreakerTimeoutCounterResetWindowInMinutes(5); | ||
| Assert.AreEqual(15, result); | ||
| } | ||
| finally | ||
| { | ||
| // Clean up environment variable | ||
| Environment.SetEnvironmentVariable(envVarName, null); | ||
| } | ||
| } | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unnecessary new lines
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed unnecessary new lines as requested. f8ce129 |
||
| [TestMethod] | ||
| public void PartitionLevelFailoverEnvironmentVariableRemoved() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test is not needed.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed the unnecessary test method as requested. 98fb329 |
||
| { | ||
| // Verify that the deprecated environment variable constant is no longer available | ||
| // This test ensures the AZURE_COSMOS_PARTITION_LEVEL_FAILOVER_ENABLED constant was removed | ||
| Type configManagerType = typeof(ConfigurationManager); | ||
| var field = configManagerType.GetField("PartitionLevelFailoverEnabled", | ||
| System.Reflection.BindingFlags.Static | System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Public); | ||
|
|
||
| Assert.IsNull(field, "PartitionLevelFailoverEnabled constant should have been removed"); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| public void IsPartitionLevelFailoverEnabledMethodRemoved() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test is not needed
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed the unnecessary test method as requested. 98fb329 |
||
| { | ||
| // Verify that the deprecated IsPartitionLevelFailoverEnabled method is no longer available | ||
| Type configManagerType = typeof(ConfigurationManager); | ||
| var method = configManagerType.GetMethod("IsPartitionLevelFailoverEnabled", | ||
| System.Reflection.BindingFlags.Static | System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Public); | ||
|
|
||
| Assert.IsNull(method, "IsPartitionLevelFailoverEnabled method should have been removed"); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -225,17 +225,17 @@ public void VerifyCosmosConfigurationPropertiesGetUpdated() | |
| CollectionAssert.AreEqual(regionalEndpoints.ToArray(), policy.AccountInitializationCustomEndpoints.ToArray()); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Test to validate that when the partition level failover is enabled with the preferred regions list is missing, then the client | ||
| /// initialization should succeed. This should hold true for both environment variable and CosmosClientOptions. | ||
| /// </summary> | ||
| [TestMethod] | ||
| [Owner("dkunda")] | ||
| public void CosmosClientOptions_WhenPartitionLevelFailoverEnabledAndPreferredRegionsNotSet_ShouldInitializeCosmosClientSuccessfully() | ||
| { | ||
| try | ||
| { | ||
| Environment.SetEnvironmentVariable(ConfigurationManager.PartitionLevelFailoverEnabled, "True"); | ||
| /// <summary> | ||
| /// Test to validate that when the partition level failover is enabled, client | ||
| /// initialization should succeed. Environment variable support was removed. | ||
| /// </summary> | ||
| [TestMethod] | ||
| [Owner("dkunda")] | ||
| public void CosmosClientOptions_WhenPartitionLevelFailoverEnabledAndPreferredRegionsNotSet_ShouldInitializeCosmosClientSuccessfully() | ||
| { | ||
| try | ||
| { | ||
| // Note: Environment variable for PPAF was removed. This test now verifies the new approach. | ||
|
|
||
| string endpoint = AccountEndpoint; | ||
| string key = MockCosmosUtil.RandomInvalidCorrectlyFormatedAuthKey; | ||
|
|
@@ -273,26 +273,26 @@ public void CosmosClientOptions_WhenPartitionLevelFailoverEnabledAndPreferredReg | |
|
|
||
| CosmosClient cosmosClient = cosmosClientBuilder.Build(); | ||
|
|
||
| Assert.IsNotNull(cosmosClient, | ||
| message: "ApplicationPreferredRegions or ApplicationRegion is no longer mandatory fields, hence the client initialization should succeed."); | ||
| } | ||
| finally | ||
| { | ||
| Environment.SetEnvironmentVariable(ConfigurationManager.PartitionLevelFailoverEnabled, null); | ||
| Assert.IsNotNull(cosmosClient, | ||
| message: "ApplicationPreferredRegions or ApplicationRegion is no longer mandatory fields, hence the client initialization should succeed."); | ||
| } | ||
| finally | ||
| { | ||
| // Cleanup - no environment variable to reset since PPAF env var was removed | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Test to validate that when the partition level failover is enabled with the preferred regions list is provided, then the client | ||
| /// initialization should be successful. This holds true for both environment variable and CosmosClientOptions. | ||
| /// </summary> | ||
| [TestMethod] | ||
| [Owner("dkunda")] | ||
| public void CosmosClientOptions_WhenPartitionLevelFailoverEnabledAndPreferredRegionsSet_ShouldInitializeSuccessfully() | ||
| { | ||
| try | ||
| { | ||
| Environment.SetEnvironmentVariable(ConfigurationManager.PartitionLevelFailoverEnabled, "True"); | ||
| /// <summary> | ||
| /// Test to validate that when the partition level failover is enabled with the preferred regions list is provided, then the client | ||
| /// initialization should be successful. Environment variable support was removed. | ||
| /// </summary> | ||
| [TestMethod] | ||
| [Owner("dkunda")] | ||
| public void CosmosClientOptions_WhenPartitionLevelFailoverEnabledAndPreferredRegionsSet_ShouldInitializeSuccessfully() | ||
| { | ||
| try | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove try-finally
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also similar places wherever its applicable in this PR
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed unnecessary try-finally blocks in CosmosClientOptionsUnitTests.cs for the two test methods that only had cleanup comments with no actual environment variable cleanup. f8ce129 |
||
| { | ||
| // Note: Environment variable for PPAF was removed. This test now verifies the new approach. | ||
|
|
||
| string endpoint = AccountEndpoint; | ||
| string key = MockCosmosUtil.RandomInvalidCorrectlyFormatedAuthKey; | ||
|
|
@@ -355,12 +355,12 @@ public void CosmosClientOptions_WhenPartitionLevelFailoverEnabledAndPreferredReg | |
| Assert.AreEqual(cosmosSerializerOptions.Indented, clientOptions.SerializerOptions.Indented); | ||
| Assert.IsFalse(clientOptions.AllowBulkExecution); | ||
| Assert.AreEqual(consistencyLevel, clientOptions.ConsistencyLevel); | ||
| Assert.IsNotNull(clientOptions.ApplicationPreferredRegions); | ||
| Assert.IsNotNull(clientOptions.AccountInitializationCustomEndpoints); | ||
| } | ||
| finally | ||
| { | ||
| Environment.SetEnvironmentVariable(ConfigurationManager.PartitionLevelFailoverEnabled, null); | ||
| Assert.IsNotNull(clientOptions.ApplicationPreferredRegions); | ||
| Assert.IsNotNull(clientOptions.AccountInitializationCustomEndpoints); | ||
| } | ||
| finally | ||
| { | ||
| // Cleanup - no environment variable to reset since PPAF env var was removed | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -1212,8 +1212,26 @@ public void TestServerCertificatesValidationWithDisableSSLFlagTrue(string connSt | |
| RemoteCertificateValidationCallback? httpClientRemoreCertValidationCallback = socketsHttpHandler.SslOptions.RemoteCertificateValidationCallback; | ||
| Assert.IsNotNull(httpClientRemoreCertValidationCallback); | ||
| #nullable disable | ||
| } | ||
|
|
||
| } | ||
|
|
||
| [TestMethod] | ||
| public void DisablePartitionLevelFailoverOptionTest() | ||
| { | ||
| // Test that the DisablePartitionLevelFailover option defaults to false | ||
| CosmosClientOptions clientOptions = new CosmosClientOptions(); | ||
| Assert.IsFalse(clientOptions.DisablePartitionLevelFailover); | ||
|
|
||
| // Test that setting DisablePartitionLevelFailover to true is reflected in the connection policy | ||
| clientOptions.DisablePartitionLevelFailover = true; | ||
| ConnectionPolicy policy = clientOptions.GetConnectionPolicy(clientId: 0); | ||
| Assert.IsTrue(policy.DisablePartitionLevelFailover); | ||
|
|
||
| // Test that setting DisablePartitionLevelFailover to false is reflected in the connection policy | ||
| clientOptions.DisablePartitionLevelFailover = false; | ||
| policy = clientOptions.GetConnectionPolicy(clientId: 0); | ||
| Assert.IsFalse(policy.DisablePartitionLevelFailover); | ||
| } | ||
|
|
||
| private class TestWebProxy : IWebProxy | ||
| { | ||
| public ICredentials Credentials { get; set; } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -398,10 +398,8 @@ public async Task TestPPAFClientAndServerEnablementCombinationScenariosAsync( | |
| bool ppafEnabledFromClient, | ||
| bool? ppafEnabledFromService) | ||
| { | ||
| if (ppafEnabledFromClient) | ||
| { | ||
| Environment.SetEnvironmentVariable(ConfigurationManager.PartitionLevelFailoverEnabled, "True"); | ||
| } | ||
| // Note: Environment variable for PPAF was removed as per acceptance criteria. | ||
| // PPAF is now controlled exclusively by account metadata and client options. | ||
|
|
||
| try | ||
| { | ||
|
|
@@ -495,8 +493,7 @@ public async Task TestPPAFClientAndServerEnablementCombinationScenariosAsync( | |
| } | ||
| finally | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unnecessary try-finally
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed the unnecessary try-finally block as requested. 98fb329 |
||
| { | ||
| // Reset the environment variable to avoid affecting other tests. | ||
| Environment.SetEnvironmentVariable(ConfigurationManager.PartitionLevelFailoverEnabled, null); | ||
| // Cleanup - no environment variable to reset since PPAF env var was removed | ||
| } | ||
| } | ||
|
|
||
|
|
||
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.
This can be interpreted as
!this.ConnectionPolicy.DisablePartitionLevelFailoverOverride && this.accountServiceConfiguration != null && this.accountServiceConfiguration.AccountProperties.EnablePartitionLevelFailover.HasValueWe can get rid of the local variable in that case:
isPPafEnabledThere 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.
Simplified the PPAF logic by removing the local variable
isPPafEnabledand using direct assignment as suggested. 6a1cfb9