moomindani commented on PR #64960:
URL: https://github.com/apache/airflow/pull/64960#issuecomment-4561143677
Thanks for the repro — the user-visible error is exactly what was promised.
One substantive concern on the validator itself: the PR description says it
"uses Airflow serde (`airflow.sdk.serde.serialize`) to validate compatibility",
but the implementation switched to `json.dumps` in a00dcfce ("Refactor retry.py
to use stdlib JSON serialization"). The actual trigger-kwargs boundary uses
Airflow's serde layer, which accepts more than JSON (datetimes,
`__serialize__`-bearing classes, etc.). Validating with `json.dumps` can reject
configurations that would actually serialize fine — e.g.
`retry_args={"deadline": some_datetime}` — which would be a regression on valid
configs.
Two options:
1. Switch back to `serde_serialize()` from `airflow.sdk.serde` so the
validator and the boundary use the same serializer. This is what the PR
description still claims.
2. Keep `json.dumps` but document the contract as "must be
JSON-serializable" (stricter than what Airflow actually requires).
I'd prefer option 1 — the whole point is to mirror what the serializer at
the boundary will do.
Minor things I'd skip for this PR: the `_retry_test_utils.py` consolidation
that got reverted in the last commit was nicer than the 4× duplicated
`UNSUPPORTED_RETRY_ARGS` we ended up with, and the `time.time()` → fixed-value
change in the sensor test is unrelated drive-by. Both are non-blocking.
--
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]
