-
Notifications
You must be signed in to change notification settings - Fork 36
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
Insitu query logging & request timeouts #207
base: master
Are you sure you want to change the base?
Conversation
I suspect there may be an issue with this though...
# Conflicts: # CHANGELOG.md
|
||
try: | ||
if session is not None: | ||
response = session.get(next_page_url, params=params, timeout=(15.05, 331)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the justification of setting these values separately? Also, why 331 and not 300 (the gateway timeout we have configured)? Also curious why you chose 15.05
Also, we should probably avoid magic numbers and assign these values to variables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean by the first question.
I chose the numbers (primarily 15.05) since the documentation for setting timeouts recommends setting it to a little bit higher than a multiple of 3, so I picked one admittedly arbitrarily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant why set timeout=(15.05, 331)
rather than timeout=331
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First one is connect timeout, second is read timeout.
I suppose it would be beneficial to fail quickly if we can't even connect rather than wait a whole 5 minutes
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
Improved the logging for insitu queries to show when the query starts + its URL and then when the query ends, its URL (for correlation), status code, and elapsed time.