-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Source TikTok-Marketing: added more clear message for 504 error #23091
Source TikTok-Marketing: added more clear message for 504 error #23091
Conversation
/test connector=connectors/source-tiktok-marketing
Build FailedTest summary info:
|
/test connector=connectors/source-tiktok-marketing
Build FailedTest summary info:
|
/test connector=connectors/source-tiktok-marketing
Build FailedTest summary info:
|
/test connector=connectors/source-tiktok-marketing
Build FailedTest summary info:
|
/test connector=connectors/source-tiktok-marketing
Build PassedTest summary info:
|
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.
Please add allowedHosts.
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.
The requested changes are trivial or more of an open discussion therefore I'll approve the PR
@@ -427,7 +431,7 @@ def stream_slices(self, **kwargs) -> Iterable[Optional[Mapping[str, Any]]]: | |||
ids = self.get_advertiser_ids() | |||
start, end, step = 0, len(ids), 100 | |||
for i in range(start, end, step): | |||
yield {"advertiser_ids": ids[i: min(end, i + step)]} | |||
yield {"advertiser_ids": ids[i : min(end, i + step)]} |
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.
nit: this does not align with PEP 8: E203 whitespace before ':' even though our black formatter does not pick it up
""" | ||
try: | ||
data = response.json() | ||
except Exception: | ||
if response.status_code == 504: |
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 have two questions:
- Do we expect Tiktok to maybe someday have the response body as JSON as well? If so, this code will not work anymore
- Why don't we add this as part of the HttpStream? I seems like whenever there will be a 504, this will be an error that we will want to have clear logging on. @brianjlai is there a reason why we would not put this in the CDK itself?
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.
Answering for first question, docs, tik tok has declaretaled status codes in json response, there aren't any status for 504, which is html.
/test connector=connectors/source-tiktok-marketing
Build PassedTest summary info:
|
/publish connector=connectors/source-tiktok-marketing
if you have connectors that successfully published but failed definition generation, follow step 4 here |
…ytehq#23091) * added handling of 504 error * added changelog * fix log message * updated expected records * added allowedHosts and fix formatting * auto-bump connector version --------- Co-authored-by: Serhii Lazebnyi <53845333+lazebnyi@users.noreply.github.com> Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>
added more clear message for 504 error
on-call: https://github.com/airbytehq/oncall/issues/980