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

[Feature] Experimental: Make retry strategy configurable #363

Merged
merged 1 commit into from
Oct 21, 2024

Conversation

avijeet95
Copy link
Contributor

Adds support to configure Retry Strategy in HttpClient. Currently only the default retry strategy is used. This strategy retries 3 times and does not have any sleep interval in between. For our use case, we would prefer using the ExponentialBackOffStrategy.

This does not affect the default behavior but gives additional options to the users.

@avijeet95 avijeet95 changed the title Make retry strategy configurable [FEATURE] Make retry strategy configurable Oct 17, 2024
@avijeet95 avijeet95 changed the title [FEATURE] Make retry strategy configurable [Feature] Make retry strategy configurable Oct 17, 2024
Copy link
Contributor

@mgyucht mgyucht left a comment

Choose a reason for hiding this comment

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

Let's label this as experimental, but otherwise this is fine for me.

Long-term, I think we will want to implement retries at a higher layer, after the response is deserialized, so that the retry implementation can review a deserialized error response before making a decision.

@avijeet95 avijeet95 changed the title [Feature] Make retry strategy configurable [Experimental] Make retry strategy configurable Oct 18, 2024
@avijeet95 avijeet95 changed the title [Experimental] Make retry strategy configurable [Feature] Make retry strategy configurable Oct 18, 2024
@avijeet95
Copy link
Contributor Author

Let's label this as experimental, but otherwise this is fine for me.

Long-term, I think we will want to implement retries at a higher layer, after the response is deserialized, so that the retry implementation can review a deserialized error response before making a decision.

Yes right now there are retries from the SDK level and then further in HttpClient. But making that decision is a bit more involved so this PR unblocks us till we get to the point you suggested. Thanks!

@avijeet95 avijeet95 changed the title [Feature] Make retry strategy configurable [Experiment] Make retry strategy configurable Oct 18, 2024
@avijeet95 avijeet95 changed the title [Experiment] Make retry strategy configurable [Experimental] Make retry strategy configurable Oct 18, 2024
@avijeet95
Copy link
Contributor Author

Added experimental to list of valid tags

@mgyucht
Copy link
Contributor

mgyucht commented Oct 18, 2024

Ah sorry, I meant to label withRequestRetryHandler as experimental in a doccomment. The PR tags need to be consistent across all SDKs to minimize unnecessary variance. Let's set the PR title to[Feature] Experimental: Make retry strategy configurable.

@avijeet95 avijeet95 changed the title [Experimental] Make retry strategy configurable [Feature] Experimental : Make retry strategy configurable Oct 18, 2024
@mgyucht mgyucht changed the title [Feature] Experimental : Make retry strategy configurable [Feature] Experimental: Make retry strategy configurable Oct 21, 2024
@mgyucht mgyucht added this pull request to the merge queue Oct 21, 2024
Merged via the queue into databricks:main with commit a33907e Oct 21, 2024
11 checks passed
rauchy added a commit that referenced this pull request Oct 22, 2024
### New Features and Improvements

 * Experimental: Make retry strategy configurable ([#363](#363)).

### Bug Fixes

 * CommonHttpsClient Builder - set timeout correctly ([#362](#362)).
@rauchy rauchy mentioned this pull request Oct 22, 2024
github-merge-queue bot pushed a commit that referenced this pull request Oct 22, 2024
### New Features and Improvements

* Experimental: Make retry strategy configurable
([#363](#363)).


### Bug Fixes

* CommonHttpsClient Builder - set timeout correctly
([#362](#362)).

Co-authored-by: Omer Lachish <[email protected]>
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.

3 participants