Re: [PR] Databricks: Fail fast for non-serializable retry_args in deferrable operators and triggers [airflow]

["moomindani (via GitHub)" <gi...@apache.org>]

moomindani commented on PR #64960:
URL: https://github.com/apache/airflow/pull/64960#issuecomment-4591066978

   Re-reviewed after the latest update — all three points I raised earlier are resolved:
   
   1. **Rebase / accidental feature removal** — the branch is now rebased; the diff is back to just the retry-args validation (6 Databricks files). `on_kill` and `self.caller` (+ its `serialize()` entry) are present again on both triggers, so the #65672 / #66965 features are no longer regressed.
   2. **Real-env evidence** — the `airflow dags test`-equivalent repro you posted shows the Databricks-specific `ValueError` firing before any Databricks API call, exactly as the PR promises.
   3. **Validator matches the boundary serializer** — switching from `json.dumps` back to `airflow.sdk.serde.serialize` (with the `airflow.serialization.serde` fallback) means the validator now mirrors what the trigger boundary actually does. The added `datetime` case confirms serde-compatible values pass while Tenacity strategy objects still fail fast.
   
   I ran the added tests locally (`test_retry.py` plus the operator/sensor/trigger cases) and they pass — serde rejects `wait_incrementing` / `stop_after_attempt` and accepts `{"deadline": datetime(...)}`.
   
   The `time.time()` → fixed `STATEMENT_END_TIME` change I'd earlier flagged as drive-by is actually fine — it makes the test deterministic, in line with the no-`datetime.now()` testing standard. The `UNSUPPORTED_RETRY_ARGS` duplication across test files is also fine given the earlier preference to keep retry test data local.
   
   LGTM from the Databricks side — thanks for working through the iterations.


-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org