Add failover request #584
Add failover request #584
Conversation
zhiyuanliang-ms
requested review from
avanigupta,
jimmyca15,
amerjusupovic and
RichardChen820
| @@ -1077,6 +1080,8 @@ private void UpdateCacheExpirationTime(KeyValueWatcher changeWatcher) | |||
| } | |||
|
|
|||
| previousEndpoint = currentEndpoint; | |||
|
|
|||
| _requestTracingOptions.IsFailoverRequest = true; | |||
There was a problem hiding this comment.
Let's consider this concrete scenario, customer passes an endpoint (static) and we discover a replica endpoint (dynamic)
- At a time point, the static one starts to fail, failover to the dynamic one, current code can handle the dynamic client as a failover, add failover tag in correlation context.
- The static client keeps failing and refresh trigger before its backoff time due, I think the dynamic client still need to have the failover tag in correlation context, but the current code will treat it as not a failover
There was a problem hiding this comment.
And we need to consider the loadbalance feature, the load balance requests should not be considered as failover
There was a problem hiding this comment.
If we don't consider loadbalancing, then we can have a very simple way to decide whether the request is from a failover: we can just check whether the current client is the first client from the list returned by ConfigurationClientManager.GetClients. But this way will break when load balancing is enabled.
There was a problem hiding this comment.
@RichardChen820 I sent an email to discuss this.
zhiyuanliang-ms
force-pushed
the
zhiyuanliang/failover-telemetry
branch
from
4ea24d5 to
f71d226
Compare
| /// <summary> | ||
| /// Flag to indicate whether replica discovery is enabled. | ||
| /// </summary> | ||
| public bool IsReplicaDiscoveryEnabled { get; set; } = false; |
There was a problem hiding this comment.
It's quite easy to know the count of replica discovered by the srv dns lookup, how about set DiscoveredReplicaCount=x rather than using a boolean to indicate enabled or not.
| @@ -223,6 +230,11 @@ public async Task RefreshAsync(CancellationToken cancellationToken) | |||
| } | |||
| ); | |||
|
|
|||
| if (clients.Count() < totalClientCount && _requestTracingEnabled && _requestTracingOptions != null) | |||
There was a problem hiding this comment.
Comparing the client count can't tell failover happen or not, it just tell some client's backoff time is not due.
There was a problem hiding this comment.
I planned to move this PR on preview branch where we have load balancing implementated.
|
Move this PR on preview branch so that we can think more clearly about how we should consider failover when load balancing is introduced. |
|
Move to #586 |
Why this PR?
#583
Visible change
Correlation context will have two more tags:
UsesReplicaDiscoveryandFailoverRequest