Use AirflowJsonEncoder in BaseXCom.serialize_value

[olivermeyer]

closes: #16386

Use the custom AirflowJsonEncoder when serializing a value for XCom.

[potiuk]

LGTM but I would love to have someone more familiar with the encoder to verify if there are no side effects ? Things like performance and some potential serialize/deserialize compatibility come to mind immmediately.

[github-actions]

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

[olivermeyer]

LGTM but I would love to have someone more familiar with the encoder to verify if there are no side effects ? Things like performance and some potential serialize/deserialize compatibility come to mind immmediately.

Thanks for reviewing. Do you know who is familiar with the encoder and could take a look?

Also, CI says The PR likely needs to run all tests! - is that the case? If yes, I'll amend and force-push. If not, is there a way to trigger CI without force-pushing? One of the tests failed but it passes locally so could be a transient error.

[potiuk]

Thanks for reviewing. Do you know who is familiar with the encoder and could take a look?

Let's see who will chime in :). There are a few CODEOWNERS already added automatically so I guess one or few of those nice people will.

Also, CI says The PR likely needs to run all tests! - is that the case? If yes, I'll amend and force-push. If not, is there a way to trigger CI without force-pushing? One of the tests failed but it passes locally so could be a transient error.

Please rebase and push. This is pretty standard and you will likely expect to do couple of those while you iterate on your PRs in Airflow.

[olivermeyer]

Bumping this PR - any chance one of the reviewers could take a look?

[potiuk]

Hey Everyone - any comments here? @kaxil :) ? I somehow have a feeling that using the custom encoder is not the best idea.

[kaxil]

Yeah please hold off on this one, as a part of #8703 we were hoping/planning to use Serialization code in https://github.com/apache/airflow/blob/19ed074e9c696eb4aff25f3a833e7f359f2b1c38/airflow/serialization/serialized_objects.py to achieve the same and more.

And at some point remove the JsonEncode in airflow.utils in favor of it too.

[olivermeyer]

@kaxil thanks for taking a look. I'll close this PR for now. One thing isn't clear to me: will there be a need for a similar PR in the future to use some other custom serialization when setting an Xcom, or will that somehow be handled as part of #8703?

[kaxil]

@kaxil thanks for taking a look. I'll close this PR for now. One thing isn't clear to me: will there be a need for a similar PR in the future to use some other custom serialization when setting an Xcom, or will that somehow be handled as part of #8703?

Yup, that is correct, there was an attempt in #9847 however the PR was abandoned