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 documentation, handle 504 #1336

Merged
merged 2 commits into from
Jul 27, 2017
Merged

add documentation, handle 504 #1336

merged 2 commits into from
Jul 27, 2017

Conversation

matkoniecz
Copy link
Contributor

  • Update tests
  • Update data migrations (not relevant)
  • Update docs (in comments of code)

documents 429 handling, add handling to 504.

504 also may happen (as overpass is overloaded) and also typically is solvable by retrying.

Today it was happening with some (low) frequency and without that change test relying on public overpass were sometimes failing.

Copy link
Member

@rmarianski rmarianski left a comment

Choose a reason for hiding this comment

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

Added a minor comment about possibly spelling the logic differently. LGTM either way.

@@ -503,9 +503,17 @@ def query_result(self, query):
params=dict(data=query))
if r.status_code == 200:
break
if r.status_code != 429:
if r.status_code != 429 and r.status_code != 504:
Copy link
Member

Choose a reason for hiding this comment

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

Do you think it's clearer spelling it like:

if r.status_code == 200 or r.status_code not in (429, 504):
    break

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 like r.status_code not in (429, 504) - much better than testing 429, 504 separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

210227d is inspired by this comment. I kept 200 and 429/504 separately.

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.

2 participants