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

Overall timeout policy feature #37

Conversation

NYMEZIDE
Copy link
Contributor

HttpClientSettings: add new optional parameter TimeoutOverall
When HttpClientTimeout less than TimeoutOverall then HttpClientTimeout equal as TimeoutOverall.

tests: HttpClientWrapperBuilder using AddJsonClient instead of a direct call AddHttpClient
add 5 test cases of Overall Timeout parameter

Issue #13

When HttpClientTimeout less than TimeoutOverall then HttpClientTimeout equal as TimeoutOverall.

tests: HttpClientWrapperBuilder using AddJsonClient instead of a direct call AddHttpClient
add 5 test cases of Overall Timeout parameter
Copy link
Contributor

@Ceridan Ceridan left a comment

Choose a reason for hiding this comment

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

Great job actually, this issue get off the ground. I have some questions to discuss before approval. See review.

@Ceridan Ceridan added the hacktoberfest-accepted Hacktoberfest label Oct 16, 2020
timeoutOverall combines feature httpClientTimeout
timeoutOverall became required
tests: fixs for new changes
+ Should_httpClientTimeout_is_overallTimeout_with_delta_1000ms test
@NYMEZIDE NYMEZIDE changed the base branch from master to milestone/v2.0.0 October 17, 2020 17:46
@Ceridan Ceridan self-requested a review October 18, 2020 12:14
Copy link
Contributor

@Ceridan Ceridan left a comment

Choose a reason for hiding this comment

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

Great job, currently we have good consistent code with proper OverallTimeout integration. Could I ask you to make some minor changes to make this changes easier to understand for end-users.

- preserve properties order: TimeoutOverall before TimeoutPerTry
- TimeoutOverallInMilliseconds set 50000
- fix spelling error
Copy link
Contributor

@Ceridan Ceridan left a comment

Choose a reason for hiding this comment

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

Awesome job! Thank you a lot!

@Ceridan Ceridan merged commit 49e6e62 into dodopizza:milestone/v2.0.0 Oct 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants