Keep polling DataFusion pipeline state when the run is not visible yet - #72406
Conversation
apache#60688 changed `_check_response_status_and_data` to raise `requests.exceptions.HTTPError` on a 404 instead of `AirflowNotFoundException`, but the polling loop in `DataFusionHook.wait_for_pipeline_state` was changed to catch `KeyError` instead of `AirflowException`. The two no longer line up, so the 404 that CDAP returns while a run is still being registered escapes the loop and fails `CloudDataFusionStartPipelineOperator` immediately. That 404 tolerance was added in apache#10031 for exactly this reason: right after a pipeline is started, the run is not yet visible in the system. Catch `HTTPError` alongside `KeyError` so the loop keeps polling. Only the 404 branch of `_check_response_status_and_data` raises `HTTPError` (any other non-200 raises `RequestException`), so this stays narrow: real failures still propagate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015kAvbZ6SeKgp6jbGcvpXSh
|
I think it (kinda) makes sense! But additionally, I would do some AI-review of PR #60688 because I'm guessing it's only the tip of the iceberg if so much exception were changed in 1 PR, probably a bunch of try/except are going to fail in one situation or another. Moreover I would challenge if KeyError is possible at all and if one should retry on KeyError at all... |
|
Yeah, fair point — let me split this into two things. The HTTPError catch is the actual fix here. That's the regression the issue is The KeyError catch was already there on main before this PR (also came in with Auditing all of #60688 for similar issues is a good shout, but honestly feels |
💯Asbolutely, focus should definitely remain on fixing the issue, thanks a lot for the PR! 👌 |
apache#72406) apache#60688 changed `_check_response_status_and_data` to raise `requests.exceptions.HTTPError` on a 404 instead of `AirflowNotFoundException`, but the polling loop in `DataFusionHook.wait_for_pipeline_state` was changed to catch `KeyError` instead of `AirflowException`. The two no longer line up, so the 404 that CDAP returns while a run is still being registered escapes the loop and fails `CloudDataFusionStartPipelineOperator` immediately. That 404 tolerance was added in apache#10031 for exactly this reason: right after a pipeline is started, the run is not yet visible in the system. Catch `HTTPError` alongside `KeyError` so the loop keeps polling. Only the 404 branch of `_check_response_status_and_data` raises `HTTPError` (any other non-200 raises `RequestException`), so this stays narrow: real failures still propagate.
closes: #72385
What's the problem
CloudDataFusionStartPipelineOperatorstarts a pipeline and then pollsDataFusionHook.wait_for_pipeline_state()for the run's status. Right after the start call, CDAP hasn't registered the run yet and returns404for the first poll or two.#60688 changed the exception handling, causing the 404 to no longer be caught:
_check_response_status_and_datawait_for_pipeline_stateAirflowNotFoundExceptionAirflowExceptionHTTPErrorKeyErrorHTTPErrorisn't aKeyError, so the 404 escapes the polling loop and fails the task immediately.The fix
Changed the exception handling to: