kaxil commented on code in PR #69985:
URL: https://github.com/apache/airflow/pull/69985#discussion_r3681519605
##########
airflow-core/src/airflow/migrations/versions/0094_3_2_0_replace_deadline_inline_callback_with_fkey.py:
##########
@@ -54,12 +54,102 @@
_ASYNC_CALLBACK_CLASSNAME = "airflow.sdk.definitions.deadline.AsyncCallback"
+def _serialize_extended(value):
Review Comment:
0094 is in the released `3.2.0` tag and main is already 3.4.0, so patching
it in place only helps deployments upgrading straight from <3.2. Everyone who
went through 3.2.x or 3.3.x has already run the broken version, has 100% of
their callback rows unreadable, and will never re-run 0094 to pick this up.
Is a repair revision planned as a follow-up? A new migration could reuse
these two helpers to re-encode existing rows, and it would need to handle both
corrupt shapes: Postgres rows have the outer `__var` envelope with raw `kwargs`
underneath, MySQL/SQLite rows have no envelope at all. That second shape is why
"repair out of band" is not actionable as written -- the `jsonb_path_exists`
query in the description matches on `$.**.__var.*`, which finds nothing in a
MySQL/SQLite row and so reports it clean.
##########
airflow-core/newsfragments/69985.bugfix.rst:
##########
@@ -0,0 +1 @@
+Fix scheduler crash from deadline callbacks whose kwargs contain a nested
dict. Migration ``0094`` (``replace_deadline_inline_callback_with_fkey``) built
``callback.data`` with nested kwargs left unencoded, so
``BaseSerialization.deserialize`` raised ``KeyError`` on read and the scheduler
entered CrashLoopBackOff. Both the Postgres and MySQL/SQLite upgrade paths now
extended-serialize nested kwargs (and the downgrade path decodes them back).
Deployments already upgraded across ``0094`` must repair existing
``callback.data`` rows out of band. (#69980)
Review Comment:
Scoping this to nested dicts undersells it. `BaseSerialization.deserialize`
fetches `encoded_var[Encoding.VAR]` on any non-primitive, non-list value, so a
flat `kwargs={"k": "v"}` and even `kwargs={}` raise `KeyError('__var')` exactly
like the nested case, and on MySQL/SQLite the pre-fix path wrote no top-level
envelope at all so the outer dict raises immediately. Every row 0094 migrated
is unreadable, on both backends, not just the ones with nested tags.
Your own diff shows it: `test_null_callback_does_not_crash` covers a row
whose `kwargs` is `{}`, and swapping its `json.loads` for
`BaseSerialization.deserialize` only passes because of this fix. Worth
rewording to something like "every deadline callback row migrated by ``0094``",
otherwise an operator whose tags are flat strings reads this release note and
concludes they are in the clear.
--
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]
