Skip to content

Handle json encoding of V1Pod in task callback#27609

Merged
potiuk merged 9 commits into
apache:mainfrom
astronomer:fix-bad-v1pod-ser-in-callback
Nov 16, 2022
Merged

Handle json encoding of V1Pod in task callback#27609
potiuk merged 9 commits into
apache:mainfrom
astronomer:fix-bad-v1pod-ser-in-callback

Conversation

@dstandish

Copy link
Copy Markdown
Contributor

Resolves #27593

@dstandish dstandish force-pushed the fix-bad-v1pod-ser-in-callback branch from 0d48933 to bd0521c Compare November 10, 2022 22:54
@dstandish dstandish changed the title Handle json encoding of V1Pod in task callbace Handle json encoding of V1Pod in task callback Nov 10, 2022
@dstandish dstandish force-pushed the fix-bad-v1pod-ser-in-callback branch from bd0521c to e18366b Compare November 10, 2022 22:54
Comment thread airflow/models/taskinstance.py Outdated
@dstandish

Copy link
Copy Markdown
Contributor Author

chopped one more line here @uranusjr if you want to doublecheck 35839ea

Comment thread airflow/models/taskinstance.py Outdated
@akizminet

akizminet commented Nov 11, 2022

Copy link
Copy Markdown

Why don't we use AirflowJsonEncoder for to_json method like this PR: #11952 ? It's more cleaner

@dstandish dstandish force-pushed the fix-bad-v1pod-ser-in-callback branch from 35839ea to 1598796 Compare November 15, 2022 16:54
@dstandish

Copy link
Copy Markdown
Contributor Author

Why don't we use AirflowJsonEncoder for to_json method like this PR: #11952 ? It's more cleaner

Seemed like a good idea so I explored it just now. The problem is that AirflowJsonEncoder is only one way -- there's no AirflowJsonDecoder that will get you back the same object. For example, when encoding a pod, it doesn't store anywhere the information that the json came from a v1pod so when decoding the json you would just get back dict of plain json-compatible objects. So, we cannot use this approach.

HOWEVER... with some adjustments, we can use BaseSerialization. Not sure if this is any better but, i've taken a crack at it.

@uranusjr what do you think is better? Currently PR is converting to use BaseSerialization. The original approach remains visible at https://github.com/apache/airflow/compare/main..15987961421ea5aa3c05b429c068cec85c55b774

@dstandish dstandish requested a review from uranusjr November 15, 2022 22:51
Comment thread airflow/models/taskinstance.py Outdated
return NotImplemented

def as_dict(self):
warnings.warn("This method is deprecated. Use BaseSerialization.serialize.", DeprecationWarning)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably should have stacklevel=2 to make the warning show the previous stack?

@potiuk potiuk merged commit 92389cf into apache:main Nov 16, 2022
@dstandish dstandish added this to the Airflow 2.4.4 milestone Nov 18, 2022
@ephraimbuddy ephraimbuddy added the type:bug-fix Changelog: Bug Fixes label Nov 22, 2022
@ephraimbuddy ephraimbuddy modified the milestones: Airflow 2.4.4, Airflow 2.5.0 Nov 23, 2022
@kaxil kaxil deleted the fix-bad-v1pod-ser-in-callback branch December 5, 2022 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:bug-fix Changelog: Bug Fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Object of type V1Pod is not JSON serializable after detecting zombie jobs cause Scheduler CrashLoopBack

5 participants