-
Notifications
You must be signed in to change notification settings - Fork 3k
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
fix(ingest/sac): handle descriptions which are None correctly #11572
fix(ingest/sac): handle descriptions which are None correctly #11572
Conversation
edde82a
to
07aaa15
Compare
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.
mostly looks good, just a few minor nits
@@ -559,7 +561,7 @@ def get_sac_connection( | |||
|
|||
retries = 3 | |||
backoff_factor = 10 | |||
status_forcelist = (500,) | |||
status_forcelist = (400, 500, 503) |
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.
let's add a comment around this - similar to the PR description
description=( | ||
column["descriptionName"].strip() | ||
if "descriptionName" in column | ||
and column["descriptionName"] is not None |
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.
if column.get("descriptionName") is not None
@hsheth2 I have added a comment on why we have to retry requests with these HTTP status codes. 😊 |
It is technically possible that the descriptions for Stories, Models and columns of Import Data models are None...this PR fixes the handling of such cases.
Additionally some more HTTP statuses will be retried (400 and 503) - unfortunately the Import Data API is a bit unstable and returns these HTTP status codes from time to time, although everything else is working.
FYI @hsheth2
Checklist