-
Notifications
You must be signed in to change notification settings - Fork 504
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
[Internal] Circuit Breaker: Adds Code to Implement Per Partition Circuit Breaker #5023
base: master
Are you sure you want to change the base?
[Internal] Circuit Breaker: Adds Code to Implement Per Partition Circuit Breaker #5023
Conversation
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.
All good!
…tition_circuit_breaker
…tition_circuit_breaker
[DataRow("30", DisplayName = "Scenario whtn the circuit breaker consecutive failure threshold is set to 30.")] | ||
[Owner("dkunda")] | ||
[Timeout(70000)] | ||
public async Task ReadItemAsync_WithCircuitBreakerEnabledAndSingleMasterAccountAndServiceUnavailableReceived_ShouldApplyPartitionLevelOverride( |
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.
It would be good to extend this to non-point operations such as queries and batch.
/// location for the partition key range. | ||
/// </summary> | ||
/// <returns>A task representing the asynchronous operation.</returns> | ||
private async Task TryOpenConnectionToUnhealthyEndpointsAndInitiateFailbackAsync() |
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.
One question I had about this - are these background tasks assigned to a thread part of a dedicated thread pool which is also bounded? We have been very conservative here in the Java SDK (at most 1 or 2 threads running this background recovery flow to not affect the hot path). How are we bounding / managing this?
Microsoft.Azure.Cosmos/src/Routing/GlobalPartitionEndpointManagerCore.cs
Show resolved
Hide resolved
…tition_circuit_breaker
…tition_circuit_breaker
…tition_circuit_breaker
Good Point. I think it's a good idea to keep separate threshold for both reads and writes. Exposed two separate environment variables for capturing read and write thresholds. |
…tition_circuit_breaker
…tition_circuit_breaker
/// <summary> | ||
/// Enable partition level circuit breaker | ||
/// </summary> | ||
internal bool EnablePartitionLevelCircuitBreaker { get; set; } = ConfigurationManager.IsPartitionLevelCircuitBreakerEnabled(defaultValue: false); |
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.
PPCB dependent on PPAF, argument check
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.
May not be always true right?
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.
@@ -939,8 +939,11 @@ internal virtual void Initialize(Uri serviceEndpoint, | |||
#endif | |||
|
|||
this.GlobalEndpointManager = new GlobalEndpointManager(this, this.ConnectionPolicy); | |||
this.PartitionKeyRangeLocation = this.ConnectionPolicy.EnablePartitionLevelFailover | |||
? new GlobalPartitionEndpointManagerCore(this.GlobalEndpointManager) | |||
this.PartitionKeyRangeLocation = this.ConnectionPolicy.EnablePartitionLevelFailover || this.ConnectionPolicy.EnablePartitionLevelCircuitBreaker |
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.
PPCB depdnent on PPAF, some PPAF check might suffice?
/// A read-only string containing the environment variable name for enabling per partition circuit breaker. The default value | ||
/// for this flag is false. | ||
/// </summary> | ||
internal static readonly string PartitionLevelCircuitBreakerEnabled = "AZURE_COSMOS_PARTITION_LEVEL_CIRCUIT_BREAKER_ENABLED"; |
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.
nit: Just for simplicity how about use PPCB? (Below configs do use it anyways)
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 updated the variable name as AZURE_COSMOS_CIRCUIT_BREAKER_ENABLED
, for simplicity.
} | ||
|
||
/// <summary> | ||
/// Determines if a read request is eligible for partition-level circuit breaker. |
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.
nit: Read request used in the doc, even writes in MM are also used.
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.
Updated the verbiage.
@@ -454,10 +457,14 @@ private ShouldRetryResult TryMarkEndpointUnavailableForPkRangeAndRetryOnServiceU | |||
|
|||
if (shouldMarkEndpointUnavailableForPkRange) |
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.
Some failures might be fatal and might need to by-pass circuit-breaker? (ex: 429/3092)
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.
Yes. Thanks for pointing this out. 429/3092
should trigger the partition failover irrespective of whether the PPCB counter reaches it's threshold. Updated the code to reflect the same.
/// <summary> | ||
/// Enable partition level circuit breaker | ||
/// </summary> | ||
internal bool EnablePartitionLevelCircuitBreaker { get; set; } = ConfigurationManager.IsPartitionLevelCircuitBreakerEnabled(defaultValue: false); |
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.
IMP: What's the guidance for compute gateway?
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.
For compute gateway, by default the PPAF will be disabled, so does the PPCB. If compute choses to enable PPAF, then we will enable PPCB by default. Are you thinking on the background validation task for the failed addresses ? If so - we can choose a model to enable PPCB by default, and opt-out if the customer doesn't want PPCB.
But IMO, given that the address validation is guarded by the channel dictionary at-least one healthy channel validation, we should be good IMO.
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.
Can we please include this in the property codedoc for reference?
/// </returns> | ||
private bool IsRequestEligibleForPartitionLevelCircuitBreaker() | ||
{ | ||
return (this.documentServiceRequest.IsReadOnlyRequest || this.isMultiMasterWriteRequest) |
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.
Is MM + PPAF in scope in near term?
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 felt, doing for both (SM + MM) will help to address at the same time, since the differences would not be too much for PPCB + Reads and PPCB + MM + Writes. Added MM tests to cover the changes.
/// <returns> | ||
/// True if the request is a write request; otherwise, false. | ||
/// </returns> | ||
private bool IsRequestEligibleForPerPartitionAutomaticFailover() |
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.
non-blocking: One option is move this logic also to inside partitionKeyRangeLocationCache
which is soley responsible for state maintenance (counting, resetting and deciding when to fail-over)
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.
Thanks for the suggestion. I feel keeping the logic in one single place is always a good idea. Refactored the code to keep the logic in partitionKeyRangeLocationCache
…tition_circuit_breaker
…tition_circuit_breaker
Pull Request Template
Description
The idea of having a per partition circuit breaker (aka PPCB) is to optimize a) read availability , in a single master account and b) read + write availability in a multi master account, during a time when a specific partition in one of the regions is experiencing an outage/ quorum loss. This feature is independent of the partition level failover triggered by the backend. The per partition circuit breaker is developed behind a feature flag
AZURE_COSMOS_PARTITION_LEVEL_CIRCUIT_BREAKER_ENABLED
.However, when the partition level failover is enabled, we will enable the PPCB by default so that the reads can benefits from it.
Scope
For single master, only the read requests will use the circuit breaker to add the pk-range to region override mapping, and use this mapping as a source of truth to route the read requests.
For multi master, both the read and write requests will use the circuit breaker to add the pk-range to region override mapping and use this mapping as a source of truth to route both read and write requests.
Understanding the Configurations exposed by the environment variables:
AZURE_COSMOS_CIRCUIT_BREAKER_ENABLED
: This environment variable is used to enable/ disable the partition level circuit breaker feature. The default value isfalse
.AZURE_COSMOS_PPCB_STALE_PARTITION_UNAVAILABILITY_REFRESH_INTERVAL_IN_SECONDS
: This environment variable is used to set the background periodic address refresh task interval. The default value for this interval is60 seconds
.AZURE_COSMOS_PPCB_ALLOWED_PARTITION_UNAVAILABILITY_DURATION_IN_SECONDS
: This environment variable is used to set the partition unavailability time duration in seconds. The unavailability time indicates how long a partition can remain unhealthy, before it can re-validate it's connection status. The default value for this property is5 seconds
.AZURE_COSMOS_PPCB_CONSECUTIVE_FAILURE_COUNT_FOR_READS
: This environment variable is used to set the consecutive failure count for reads, before triggering per partition level circuit breaker flow. The default value for this flag is10
consecutive failures within 1 min window.AZURE_COSMOS_PPCB_CONSECUTIVE_FAILURE_COUNT_FOR_WRITES
: This environment variable is used to set the consecutive failure count for writes, before triggering per partition level circuit breaker flow. The default value for this flag is5
consecutive failures within 1 min window.Understanding the Working Principle:
On a high level, there are three parts of the circuit breaker implementation:
Short Circuit and Failover detection: The failover detection logic will reside in the SDK ClientRetryPolicy, just like we have for PPAF. Ideally the detection logic is based on the below two principles:
Status Codes: The status codes that are indicative of partition level circuit breaker would be the following: a)
503
Service Unavailable, b)408
Request Timeout, c) cancellation token expired.Threshold: Once the failover condition is met, the SDK will look for some consecutive failures, until it hits a particular threshold. Once this threshold is met, the SDK will fail over the read requests to the next preferred region for that offending partition. For example, if the threshold value for read requests is
10
, then the SDK will look for10
consecutive failures. If the threshold is met/ exceeded, the SDK will add the region failover information for that partition.Failover a faulty partition to the next preferred region: Once the failover conditions are met, the
ClientRetryPolicy
will trigger a partition level override usingGlobalPartitionEndpointManagerCore.TryMarkEndpointUnavailableForPartitionKeyRange
to the next region in the preferred region list. This failover information will help the current, as well as the subsequent requests (reads in single master and both reads and writes in multi master) to route the request to the next region.Failback the faulty partition to it's original first preferred region: With PPAF enabled, ideally the write requests will rely on 403.3 (Write Forbidden) signal to fail the partition back to the primary write region. However, this is not true for reads. That means SDK doesn’t have a definitive signal to identify when to initiate a failback for read requests.
Hence, the idea is to create a background task during the time of read failover, which will keep track of the pk-range and region mapping. The task will periodically fetch the address from the gateway address cache for those pk ranges in the faulty region, and it will try to initiate Rntbd connection to all 4 replicas of that partition. The RNTBD open connection attempt will be made similar to that of the replica validation flow. The life cycle of the background task will get initiated during a failover and will remain until the SDK is disposed.
If the attempt to make the connection to all 4 replicas is successful, then the task will remove/ override the entry with the primary region, resulting the SDK to failback the read requests.
Type of change
Closing issues
To automatically close an issue: closes #4981