Refactor and move backoff logic to AzureAppConfigurationProvider #508
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ger for dynamic clients
jimmyca15
reviewed
Jan 8, 2024
| clientBackoffStatus = _configClientBackoffs[endpoint]; | ||
| } | ||
|
|
||
| return clientBackoffStatus?.BackoffEndTime <= DateTimeOffset.UtcNow; |
There was a problem hiding this comment.
Regarding the null check (?). I see that we either get existing backoff status or create a new one. How could it be null?
There was a problem hiding this comment.
DateTimeOffset.UtcNow, should calculate once outside of the predicate.
There was a problem hiding this comment.
Right it shouldn't be null, leftover from old change
jimmyca15
reviewed
Jan 8, 2024
|
|
||
| if (!_configClientBackoffs.TryGetValue(endpoint, out ConfigurationClientBackoffStatus clientBackoffStatus)) | ||
| { | ||
| UpdateClientBackoffStatus(endpoint, true); |
There was a problem hiding this comment.
There's no definition of success/fail here. I think it might be a bit better to just initialize it inline here and then add it into the dictionary.
jimmyca15
reviewed
Jan 8, 2024
| @@ -319,12 +282,11 @@ private async Task RefreshFallbackClients(CancellationToken cancellationToken) | |||
| _lastFallbackClientRefresh = DateTime.UtcNow; | |||
| } | |||
|
|
|||
| private bool IsFallbackClientDiscoveryDue(DateTimeOffset dateTime) | |||
| private bool IsFallbackClientDiscoveryDue(DateTimeOffset now) | |||
jimmyca15
approved these changes
Jan 8, 2024
Eskibear
reviewed
Jan 10, 2024
| { | ||
| clientBackoffStatus.FailedAttempts++; | ||
|
|
||
| TimeSpan backoffDuration = _options.MinBackoffDuration.CalculateBackoffDuration(FailOverConstants.MaxBackoffDuration, clientBackoffStatus.FailedAttempts); |
There was a problem hiding this comment.
Here basically the backoff time = minDuration * (1<<attemps) when attempts >=2.
attemps: 1, 2, 3, 4...
backoffTime( x minDuration): 1x, 4x, 8x, 16x...
Should we change it to 1<<(attemps -1), so the backoff time becomes 1x,2x,4x,8x, ...?
There was a problem hiding this comment.
That is a good idea. I think this happens because we optimized to return when attempts=1, so we never get the 2x backoff duration.
There was a problem hiding this comment.
Makes sense, just gonna subtract one from attempts in that code then.
jimmyca15
reviewed
Jan 10, 2024
| /// <summary> | ||
| /// An optional timespan value to set the minimum backoff duration to a value other than the default. | ||
| /// </summary> | ||
| /// <remarks>This property is used only for unit testing.</remarks> |
There was a problem hiding this comment.
This property is not only used for unit testing. It's referenced in this file for backoff. It would be appropriate to say this property is internal for availability in unit tests. But even that seems unnecessary.
There was a problem hiding this comment.
That makes sense, I'll remove the remarks.
RichardChen820
requested changes
Jan 16, 2024
src/Microsoft.Extensions.Configuration.AzureAppConfiguration/IConfigurationClientManager.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationProvider.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Extensions.Configuration.AzureAppConfiguration/ConfigurationClientManager.cs
Outdated
Show resolved
Hide resolved
RichardChen820
approved these changes
Jan 17, 2024
amerjusupovic
added a commit
that referenced
this pull request
Feb 21, 2024
* move backoff logic to provider, can't access backoffendtime from manager for dynamic clients * add condition before calling getclients in refreshasync, add refreshclients method * fix logic to check if backoff ended for client * fix indents * check client backoff before refresh, fix tests in progress * remove tolist * comment changes, fix refresh tests * reduce minimum backoff when testing * fix tests, fix typo with failover logic * update comments in tests * resolve PR comments * resolve small comments
amerjusupovic
added a commit
that referenced
this pull request
Feb 22, 2024
* Automatic http failover base on Srv Dns (#442) * Automatic http failover base on Srv Dns * update failover condition * Logging messages for auto failover * Not use generic method * Remove useless constructor * Failover to TCP when response is truncated * Error handling of TCP failover * Resolve comments and add thrid party notice * Fix the tests * Fix the tess * Fix the test * Update to use DnsClient.NET * Remove unecessary code * Resolve conflict * Stop once it has been auto failovered * Update GetAvaliableClient method * Resolve comments * Resolve comments * Internalize getting dynamic client in ConfigurationClientManager * Fix format * Fix format * Resolve comments * Resolve comments * Resolve comments * Resolve comments * Resolve comments * Resolve comments * Resolve comments * Not await RefreshFallbackClients when startup * Revert non-await RefreshFallbackClients * Validate domain of Srv endpoints (#489) * Validate domain of Srv endpoints * Resolve comments * Resolve comments * Create object in using * Add the origin host to the srv lookup result * Validate endpoint by domain name (#499) * Validate endpoint by domain name * Resolve comments * Remove *-test * Fix formatResolve conflict * Use await using for IAsyncDisposible * Replace IAsyncEumerable with ValueTask<IEnumerable> * Resolve comments * Remove cancallation token * Resolve comments * Refactor and move backoff logic to AzureAppConfigurationProvider (#508) * move backoff logic to provider, can't access backoffendtime from manager for dynamic clients * add condition before calling getclients in refreshasync, add refreshclients method * fix logic to check if backoff ended for client * fix indents * check client backoff before refresh, fix tests in progress * remove tolist * comment changes, fix refresh tests * reduce minimum backoff when testing * fix tests, fix typo with failover logic * update comments in tests * resolve PR comments * resolve small comments * fix tests backoff duration * fix tests backoff duration * fix error from merging main branch in * add back fix for attempts backoff calculation --------- Co-authored-by: Richard chen <99175581+RichardChen820@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.