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

fix(tableau): retry on InternalServerError 504 #12213

Merged
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,10 @@
SiteItem,
TableauAuth,
)
from tableauserverclient.server.endpoint.exceptions import NonXMLResponseError
from tableauserverclient.server.endpoint.exceptions import (
InternalServerError,
NonXMLResponseError,
)
from urllib3 import Retry

import datahub.emitter.mce_builder as builder
Expand Down Expand Up @@ -1196,6 +1199,24 @@
retry_on_auth_error=False,
retries_remaining=retries_remaining - 1,
)

except InternalServerError as ise:

Check warning on line 1203 in metadata-ingestion/src/datahub/ingestion/source/tableau/tableau.py

View check run for this annotation

Codecov / codecov/patch

metadata-ingestion/src/datahub/ingestion/source/tableau/tableau.py#L1203

Added line #L1203 was not covered by tests
# In some cases Tableau Server returns 504 error, which is a timeout error, so it worths to retry.
if ise.code == 504:
if retries_remaining <= 0:
raise ise
return self.get_connection_object_page(

Check warning on line 1208 in metadata-ingestion/src/datahub/ingestion/source/tableau/tableau.py

View check run for this annotation

Codecov / codecov/patch

metadata-ingestion/src/datahub/ingestion/source/tableau/tableau.py#L1205-L1208

Added lines #L1205 - L1208 were not covered by tests
query=query,
connection_type=connection_type,
query_filter=query_filter,
fetch_size=fetch_size,
current_cursor=current_cursor,
retry_on_auth_error=False,
retries_remaining=retries_remaining - 1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

not a huge fan of this entire method - it feels way more complicated than it needs to be and has a ton of mostly duplicated code

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 agree
If this works, we should invest some time on a refactor.

)
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need an else: raise

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed

else:
raise ise

Check warning on line 1218 in metadata-ingestion/src/datahub/ingestion/source/tableau/tableau.py

View check run for this annotation

Codecov / codecov/patch

metadata-ingestion/src/datahub/ingestion/source/tableau/tableau.py#L1218

Added line #L1218 was not covered by tests

except OSError:
# In tableauseverclient 0.26 (which was yanked and released in 0.28 on 2023-10-04),
# the request logic was changed to use threads.
Expand Down
Loading