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

TDL-16323: Add request timeout #47

Merged
merged 6 commits into from Dec 1, 2021
Merged

TDL-16323: Add request timeout #47

merged 6 commits into from Dec 1, 2021

Conversation

hpatel41
Copy link
Contributor

@hpatel41 hpatel41 commented Nov 12, 2021

Description of change

TDL-16323: Add request timeout

  • Use default timeout of 300 seconds if the user does not enter the preferred timeout value

Manual QA steps

Risks

Rollback steps

  • revert this branch

@hpatel41 hpatel41 marked this pull request as ready for review November 15, 2021 08:23
@hpatel41 hpatel41 changed the title Add request timeout TDL-16323: Add request timeout Nov 15, 2021
Copy link

@umangagrawal-crest umangagrawal-crest left a comment

Choose a reason for hiding this comment

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

@harshpatel4crest Please update the readme and sample config file

@hpatel41
Copy link
Contributor Author

@harshpatel4crest Please update the readme and sample config file

Updated readme file and sample config file.

# get the value of request timeout from config
config_request_timeout = args.config.get('request_timeout')

if config_request_timeout and float(config_request_timeout):
Copy link
Contributor

Choose a reason for hiding this comment

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

Add detailed comment over if condition.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't see the meaning of using the request_timeout variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the code and returned the value directly.

Comment on lines 202 to 204
request_timeout = float(config_request_timeout)
# return the timeout from config
return request_timeout
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
request_timeout = float(config_request_timeout)
# return the timeout from config
return request_timeout
return float(config_request_timeout)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

except requests.Timeout:
pass

# verfiy that we backoff for 5 times
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# verfiy that we backoff for 5 times
# verify that we backoff for 5 times

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

except requests.ConnectionError:
pass

# verfiy that we backoff for 5 times
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# verfiy that we backoff for 5 times
# verify that we backoff for 5 times

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@KrisPersonal KrisPersonal merged commit 94966d7 into master Dec 1, 2021
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.

None yet

8 participants