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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
