You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/06/14 07:26:06 UTC
[GitHub] [airflow] olivermeyer opened a new pull request #16395: Use AirflowJsonEncoder in BaseXCom.serialize_value
olivermeyer opened a new pull request #16395:
URL: https://github.com/apache/airflow/pull/16395
closes: #16386
Use the custom `AirflowJsonEncoder` when serializing a value for XCom.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16395: Use AirflowJsonEncoder in BaseXCom.serialize_value
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16395:
URL: https://github.com/apache/airflow/pull/16395#issuecomment-860228621
> 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.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16395: Use AirflowJsonEncoder in BaseXCom.serialize_value
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16395:
URL: https://github.com/apache/airflow/pull/16395#issuecomment-865339764
Hey Everyone - any comments here? @kaxil :) ? I somehow have a feeling that using the custom encoder is not the best idea.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #16395: Use AirflowJsonEncoder in BaseXCom.serialize_value
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #16395:
URL: https://github.com/apache/airflow/pull/16395#issuecomment-870095007
> @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 https://github.com/apache/airflow/pull/9847 however the PR was abandoned
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] olivermeyer commented on pull request #16395: Use AirflowJsonEncoder in BaseXCom.serialize_value
Posted by GitBox <gi...@apache.org>.
olivermeyer commented on pull request #16395:
URL: https://github.com/apache/airflow/pull/16395#issuecomment-860226103
> 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.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] olivermeyer closed pull request #16395: Use AirflowJsonEncoder in BaseXCom.serialize_value
Posted by GitBox <gi...@apache.org>.
olivermeyer closed pull request #16395:
URL: https://github.com/apache/airflow/pull/16395
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16395: Use AirflowJsonEncoder in BaseXCom.serialize_value
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16395:
URL: https://github.com/apache/airflow/pull/16395#issuecomment-865339764
Hey Everyone - any comments here? @kaxil :) ? I somehow have a feeling that using the custom encoder is not the best idea.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] olivermeyer commented on pull request #16395: Use AirflowJsonEncoder in BaseXCom.serialize_value
Posted by GitBox <gi...@apache.org>.
olivermeyer commented on pull request #16395:
URL: https://github.com/apache/airflow/pull/16395#issuecomment-866574037
@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?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] olivermeyer closed pull request #16395: Use AirflowJsonEncoder in BaseXCom.serialize_value
Posted by GitBox <gi...@apache.org>.
olivermeyer closed pull request #16395:
URL: https://github.com/apache/airflow/pull/16395
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #16395: Use AirflowJsonEncoder in BaseXCom.serialize_value
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #16395:
URL: https://github.com/apache/airflow/pull/16395#issuecomment-860132039
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.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #16395: Use AirflowJsonEncoder in BaseXCom.serialize_value
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #16395:
URL: https://github.com/apache/airflow/pull/16395#issuecomment-870095007
> @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 https://github.com/apache/airflow/pull/9847 however the PR was abandoned
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] olivermeyer commented on pull request #16395: Use AirflowJsonEncoder in BaseXCom.serialize_value
Posted by GitBox <gi...@apache.org>.
olivermeyer commented on pull request #16395:
URL: https://github.com/apache/airflow/pull/16395#issuecomment-865063120
Bumping this PR - any chance one of the reviewers could take a look?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org