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

Add request timeout #71

Merged
merged 4 commits into from
Feb 15, 2022
Merged

Add request timeout #71

merged 4 commits into from
Feb 15, 2022

Conversation

loeakaodas
Copy link
Contributor

@loeakaodas loeakaodas commented Feb 11, 2022

Description of change

Added request_timeout to tap http client as per TDL-16581

Manual QA steps

  • ran unit tests
  • circle tests passing

Risks

  • minimal

Rollback steps

  • revert this branch

Comment on lines +153 to +156
request_timeout = float(config_request_timeout)
else:
# If value is 0, "0", "", or not passed then it set default to 300 seconds
request_timeout = 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.

Is there a reason we use float() but the default value is an integer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used the reference implementation from tap-harvest as listed in the jira issue. I would agree that it is a bit weird to cast to float by default, my guess would be this would avoid potential precision loss if we cast to an integer instead.

However the differences in precision would be negligible for this use case anyway. I can change to int() instead to make it consistent.

Copy link
Contributor

Choose a reason for hiding this comment

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

When we tested the code with integer datatype and passed the value of request_timeout as "120.0" in the config file, then the below error message is thrown.
CRITICAL invalid literal for int() with base 10: '120.0'.
Also, we were not sure about the data type returned from the UI. So, to handle all the scenarios we used float datatype

@KrisPersonal KrisPersonal merged commit 30733f1 into master Feb 15, 2022
@luandy64 luandy64 deleted the add-request-timeout branch March 22, 2022 13:45
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