Skip to content
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

Transient HTTP exceptions: Adds retry logic to all http requests #1788

Merged
merged 26 commits into from
Sep 2, 2020

Conversation

j82w
Copy link
Contributor

@j82w j82w commented Aug 19, 2020

Pull Request Template

Description

This PR adds an abstraction on top of HttpClient called CosmosHttpClient. All the caches and gateway calls have been refactored to use the new abstraction. This adds retry logic for transient exceptions that should be retried on.

Type of change

Please delete options that are not relevant.

  • [] Bug fix (non-breaking change which fixes an issue)
  • [] New feature (non-breaking change which adds functionality)
  • [] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [] This change requires a documentation update

Closing issues

closes #1769

@j82w j82w self-assigned this Aug 19, 2020
@j82w j82w changed the title Transient HTTP exceptions: Adds retry logic to all http requests Draft - Transient HTTP exceptions: Adds retry logic to all http requests Aug 19, 2020
@j82w j82w changed the title Draft - Transient HTTP exceptions: Adds retry logic to all http requests Transient HTTP exceptions: Adds retry logic to all http requests Aug 21, 2020
@YohanSciubukgian
Copy link

As you have recently added HttpClientFactory (https://devblogs.microsoft.com/cosmosdb/httpclientfactory-cosmos-db-net-sdk/), why don't you use Polly which is now added as an extension to the framework (https://docs.microsoft.com/en-us/dotnet/architecture/microservices/implement-resilient-applications/use-httpclientfactory-to-implement-resilient-http-requests) ?

It will allow us to take full advantages of MessageHandlers registration from the HttpClientFactory and have full option control over Polly directly (retry policy, circuit breaker,...).

@j82w
Copy link
Contributor Author

j82w commented Aug 24, 2020

As you have recently added HttpClientFactory (https://devblogs.microsoft.com/cosmosdb/httpclientfactory-cosmos-db-net-sdk/), why don't you use Polly which is now added as an extension to the framework (https://docs.microsoft.com/en-us/dotnet/architecture/microservices/implement-resilient-applications/use-httpclientfactory-to-implement-resilient-http-requests) ?

It will allow us to take full advantages of MessageHandlers registration from the HttpClientFactory and have full option control over Polly directly (retry policy, circuit breaker,...).

@YohanSciubukgian

  1. General guideline is to avoid dependencies because it causes conflicts with other projects. https://azure.github.io/azure-sdk/dotnet_introduction.html#dotnet-dependencies
  2. This model guarantees that the retries always occur. If the user specifies a custom http factory then it's not possible for us to add the retry handler. Depending on every one that uses a custom http client factory to implement the correct retry logic doesn't seem realistic. The better model seems to be to wrap the http client and always retry.

@YohanSciubukgian
Copy link

YohanSciubukgian commented Aug 25, 2020

I didn't have the compatibility with .Net Standard in mind neither the need to avoid dependencies as much as possible (even with Microsoft.* packages)
Thanks for your explanations 😄

ealsur
ealsur previously approved these changes Sep 1, 2020
@@ -748,18 +713,6 @@ internal virtual async Task<PartitionKeyRangeCache> GetPartitionKeyRangeCacheAsy

internal GlobalAddressResolver AddressResolver { get; private set; }

internal event EventHandler<SendingRequestEventArgs> SendingRequest
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it impact compute?

Copy link
Contributor Author

@j82w j82w Sep 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, they only pass in the sending request. I didn't find any references to the properties in Cassandra or Mongo projects. It's also really dangerous because if they set a custom httpclient then this field no longer works because it's not possible to add a http client handler after that the http client is created.

kirankumarkolli
kirankumarkolli previously approved these changes Sep 2, 2020
Copy link
Member

@kirankumarkolli kirankumarkolli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@j82w j82w dismissed stale reviews from kirankumarkolli and ealsur via 25774a9 September 2, 2020 11:44
@ghost
Copy link

ghost commented Dec 15, 2021

Closing due to in-activity, pease feel free to re-open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TaskCancelled exception thrown by HttpClient during GetMasterAddressesViaGatewayAsync
5 participants