Introduced retries to the TableauSensor - #52770
Conversation
|
@eladkal what is your thought on the adjusted name of the retries parameter? ( |
|
The sensor currently doesn't have deferrable capability. Do we expect this functionality to work well with defer mode when implemented? |
In that case, if In this case the trigger probably should retry on any error though, to not make it overly complex, as other Tableau Operators / Sensors will also pass in retries. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
@eladkal what is your take? I would like to finish this PR. |
|
I will reopen it, as it seems close to be complete. |
b0f814a to
525f7ac
Compare
|
@eladkal - any comments? |
My comments were more of questions. I don't have time to look into it but regardless this is something we can always change in the future so feel free to proceed |
Thanks a lot for the reply, will test my code again and proceed |
|
@dominikhei This PR has a few issues that need to be addressed before it can be reviewed — please see our Pull Request quality criteria. Issues found:
What to do next:
There is no rush — take your time and work at your own pace. We appreciate your contribution and are happy to wait for updates. If you have questions, feel free to ask on the Airflow Slack. Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you. |
* Introduced retries to the TableauSensor to account for transient errors * Renamed param retries_on_failure to max_status_retries to ensure distinction to airflow retries
closes: #32799
I opened this as a draft since I’m unsure about the pattern used and made some compromises —> would appreciate feedback.
@hussein-awala , I like your idea of retrying on certain error codes (e.g., 408, 5xx). However, requests to Tableau’s get_by_id endpoint don’t expose status codes, and many exceptions don’t either. Error messages have them in plain text like:
Parsing strings for status codes feels fragile. We could catch exceptions that have
response.status_codeand retry on likely transient codes, but many don’t.My suggestion: add an optional retries_on_failure parameter to the sensor. It retries a set number of times on any failure, then raises an AirflowException. This keeps logic simple. The only other sensor that allows for configuring retries by itself is the BashSensor.
Handling job cancellations on failure, as described in the issue, seems better suited for
on_failure_callback.Additionally the TableauOperator comes with a param
blocking_refreshwhich could lead to the same problem, I wanted to address the sensor first and get some feedback.