Conversation
There was a problem hiding this comment.
Please follow the required format: "[Internal] Category: (Adds|Fixes|Refactors|Removes) Description"
Internal should be used for PRs that have no customer impact. This flag is used to help generate the changelog to know which PRs should be included. Examples:
Diagnostics: Adds GetElapsedClientLatency to CosmosDiagnostics
PartitionKey: Fixes null reference when using default(PartitionKey)
[v4] Client Encryption: Refactors code to external project
[Internal] Query: Adds code generator for CosmosNumbers for easy additions in the future.
…counter reset window configuration Co-authored-by: kirankumarkolli <6880899+kirankumarkolli@users.noreply.github.com>
| } | ||
|
|
||
| [TestMethod] | ||
| public void PartitionLevelFailoverEnvironmentVariableRemoved() |
There was a problem hiding this comment.
This test is not needed.
There was a problem hiding this comment.
Removed the unnecessary test method as requested. 98fb329
| } | ||
|
|
||
| [TestMethod] | ||
| public void IsPartitionLevelFailoverEnabledMethodRemoved() |
There was a problem hiding this comment.
This test is not needed
There was a problem hiding this comment.
Removed the unnecessary test method as requested. 98fb329
| @@ -495,8 +493,7 @@ public async Task TestPPAFClientAndServerEnablementCombinationScenariosAsync( | |||
| } | |||
| finally | |||
There was a problem hiding this comment.
Unnecessary try-finally
There was a problem hiding this comment.
Removed the unnecessary try-finally block as requested. 98fb329
@copilot fix the title |
| { | ||
| isPPafEnabled = this.accountServiceConfiguration.AccountProperties.EnablePartitionLevelFailover.Value; | ||
| bool isPPafEnabled = false; | ||
| if (this.accountServiceConfiguration != null && this.accountServiceConfiguration.AccountProperties.EnablePartitionLevelFailover.HasValue) |
There was a problem hiding this comment.
This can be interpreted as !this.ConnectionPolicy.DisablePartitionLevelFailoverOverride && this.accountServiceConfiguration != null && this.accountServiceConfiguration.AccountProperties.EnablePartitionLevelFailover.HasValue
We can get rid of the local variable in that case: isPPafEnabled
There was a problem hiding this comment.
Simplified the PPAF logic by removing the local variable isPPafEnabled and using direct assignment as suggested. 6a1cfb9
…y block Co-authored-by: kirankumarkolli <6880899+kirankumarkolli@users.noreply.github.com>
| Environment.SetEnvironmentVariable(envVarName, null); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Removed unnecessary new lines as requested. f8ce129
| if (databaseAccount.EnablePartitionLevelFailover.HasValue) | ||
| { | ||
| isPPafEnabled = databaseAccount.EnablePartitionLevelFailover.Value; | ||
| bool isPPafEnabled = false; |
There was a problem hiding this comment.
This can be simplified as:
if (!this.connectionPolicy.DisablePartitionLevelFailover && databaseAccount.EnablePartitionLevelFailover.HasValue)
{
this.connectionPolicy.EnablePartitionLevelFailover = databaseAccount.EnablePartitionLevelFailover.Value;
}
There was a problem hiding this comment.
Simplified the PPAF logic as suggested by removing the local variable and using direct assignment. f8ce129
| [Owner("dkunda")] | ||
| public void CosmosClientOptions_WhenPartitionLevelFailoverEnabledAndPreferredRegionsSet_ShouldInitializeSuccessfully() | ||
| { | ||
| try |
There was a problem hiding this comment.
Also similar places wherever its applicable in this PR
There was a problem hiding this comment.
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
…eview Co-authored-by: kundadebdatta <87335885+kundadebdatta@users.noreply.github.com>
I cannot directly modify the PR title through the available tools, but I understand it should follow the format "[Internal] Category: (Adds|Fixes|Refactors|Removes) Description". The title should be updated by someone with repository admin permissions. |
…nally blocks, simplify PPAF logic Co-authored-by: kirankumarkolli <6880899+kirankumarkolli@users.noreply.github.com>
|
This has been taken care by this PR-5284. |
This PR implements the requirements to modernize Per Partition Automatic Failover (PPAF) configuration by removing deprecated environment variable support and adding new internal client options.
Changes Made
1. Remove Dependency on Environment Variable ❌
AZURE_COSMOS_PARTITION_LEVEL_FAILOVER_ENABLEDenvironment variable supportConfigurationManager.PartitionLevelFailoverEnabledconstantConfigurationManager.IsPartitionLevelFailoverEnabled()methodDocumentClient.csandGlobalEndpointManager.csto rely exclusively on account metadata2. Create New Environment Variable for Circuit Breaker ✅
AZURE_COSMOS_PPCB_TIMEOUT_COUNTER_RESET_WINDOW_IN_MINUTESenvironment variableConfigurationManager.GetCircuitBreakerTimeoutCounterResetWindowInMinutes()methodGlobalPartitionEndpointManagerCoreto use configurable timeout counter reset window:3. Add New Internal Client Options ✅
CosmosClientOptions.DisablePartitionLevelFailoverinternal propertyConnectionPolicy.DisablePartitionLevelFailoverpropertytrue, this option disables PPAF irrespective of account settingsTesting
Backward Compatibility
This is a breaking change for any code that relied on the
AZURE_COSMOS_PARTITION_LEVEL_FAILOVER_ENABLEDenvironment variable. However, this environment variable was marked as deprecated and the preferred approach has always been to use account-level configuration.Migration Path
AZURE_COSMOS_PARTITION_LEVEL_FAILOVER_ENABLEDenvironment variableDisablePartitionLevelFailoverclient option if explicit override is neededFixes #5277.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.