kaxil commented on code in PR #76718:
URL: https://github.com/apache/airflow/pull/76718#discussion_r4515404089
##########
providers/databricks/src/airflow/providers/databricks/operators/databricks.py:
##########
@@ -1024,9 +1038,8 @@ def get_job_status(self, external_id: JsonValue, context:
Context) -> str:
try:
run_info = self._hook.get_run(external_id)
except DatabricksApiError as e:
- # A stored run whose history expired (or was deleted) returns 404.
Report a sentinel that
- # is neither active nor succeeded so the mixin resubmits fresh
instead of failing the task.
- if e.http_status_code == 404:
+ # Neither active nor succeeded, so the mixin resubmits instead of
failing every retry.
+ if is_run_missing_error(e):
Review Comment:
When this matches, `e` is dropped and the only trace of the resubmit is the
mixin's "Prior job in terminal state, resubmitting fresh" with
`status=NOT_FOUND:`. Could we log the Databricks message here before returning?
Now that a workspace switch lands on this path too, the old run may still be
going in the other workspace, and that log line is all an on-call reader gets
to tell why a second run started. Same at L1537.
##########
providers/databricks/src/airflow/providers/databricks/operators/databricks.py:
##########
@@ -82,6 +83,19 @@
XCOM_RUN_PAGE_URL_KEY = "run_page_url"
XCOM_STATEMENT_ID_KEY = "statement_id"
+# jobs/runs/get answers 400 INVALID_PARAMETER_VALUE for a run it no longer
knows about, never 404.
+# These are the two message forms the Databricks SDK overrides to
ResourceDoesNotExist; the code
+# alone also covers malformed input, which must stay an error rather than
trigger a resubmit.
+_RUN_MISSING_MESSAGE_PATTERN = re.compile(r"Run .* does not exist|Run: .* in
job: .* doesn't exist")
+
+
+def is_run_missing_error(exc: DatabricksApiError) -> bool:
+ """Whether ``exc`` means Databricks has no record of the run (expired,
deleted, or foreign)."""
+ if exc.http_status_code == 404 or exc.error_code ==
"RESOURCE_DOES_NOT_EXIST":
Review Comment:
The comment above says runs/get never answers 404, but this still checks for
it. Is the 404 branch still needed? Nothing pins it either way now: the 404 row
in `TestIsRunMissingError` returns True through the `RESOURCE_DOES_NOT_EXIST`
check, and both operator tests moved to 400.
##########
providers/databricks/tests/unit/databricks/operators/test_databricks.py:
##########
@@ -2229,7 +2262,7 @@ def test_resubmits_when_stored_run_no_longer_exists(self,
db_mock_class):
db_mock = db_mock_class.return_value
db_mock.submit_run.return_value = RUN_ID
db_mock.get_run.side_effect = [
- DatabricksApiError("Response: RESOURCE_DOES_NOT_EXIST, Status
Code: 404", http_status_code=404),
+ make_api_error(400, "INVALID_PARAMETER_VALUE", "Run 999 does not
exist."),
Review Comment:
Can we also test a malformed-input 400 from `get_run` here, asserting it
raises and `submit_run` is never called? If `get_job_status` resubmitted on
every `DatabricksApiError`, these tests would still pass. Same for the run-now
test at L3622.
##########
providers/databricks/src/airflow/providers/databricks/operators/databricks.py:
##########
@@ -82,6 +83,19 @@
XCOM_RUN_PAGE_URL_KEY = "run_page_url"
XCOM_STATEMENT_ID_KEY = "statement_id"
+# jobs/runs/get answers 400 INVALID_PARAMETER_VALUE for a run it no longer
knows about, never 404.
+# These are the two message forms the Databricks SDK overrides to
ResourceDoesNotExist; the code
+# alone also covers malformed input, which must stay an error rather than
trigger a resubmit.
+_RUN_MISSING_MESSAGE_PATTERN = re.compile(r"Run .* does not exist|Run: .* in
job: .* doesn't exist")
+
+
+def is_run_missing_error(exc: DatabricksApiError) -> bool:
Review Comment:
Should this be `_is_run_missing_error`? Its only callers are the two
`get_job_status` methods and the regex it reads is already private. It also
only works for errors raised by the sync `_do_api_call`, since `_a_do_api_call`
never sets `error_code`, which is easy to trip over once it's public.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
