hussein-awala commented on PR #37058: URL: https://github.com/apache/airflow/pull/37058#issuecomment-1915398665
> > > If we chose 2 then it makes sense to move it into common.io (with all the extras that it requires cause it is a 'new' capability for a provider). > > > > > > I think I''d support @hussein-awala 's idea here. There is a great feature which is non-obvious when thing are added in provider, namely dependency independence and ability to upgrade/downgrde separately from Airflow. And it's not much different than what we already have with the other secret backends - which are also implemented in providers, so this one would just follow the suite, and turns `common.io` into actualy pretty useful provider. > > Binding such "auxiliary" functionality into Airflow core is also against our "Airflow-as-a-platform" approach. Without my suggestions applied (where I considered adding a DB change to make it clearer what is local and what remote), in my view this one simply builds on top of the Public API that we already have and there is no particular reason it should be in "core" airflow. > > Sure. I can relate to that. What about fully integrating this into BaseXCom? So not making it an auxiliary as I stated wasn't my intention. While working with BaseXCom I noticed that it would be good to refactor it and give it a clean-up. By expanding its use into a provider we make the API more set in stone (I'm aware some of it is). > > Why do I think it is better to have it in core? I think the use of custom xcom backends is limited. The bugs in de-serialization and non purging of values from custom backends should have let to more bug reports imho. Given that most patterns consider using a custom backend to store things on object storage we take that need away by implementing this. And yes, I understand that you can store things on redis or alike as well. What I mean with this is that it is a point to consider. > > Note that your suggestion of adding the extra field could create an issue for people that are currently using a custom xcom backend. They might already store it non-locally and the pattern is to store a reference in the value field. How to provide a proper migration path is then the question. I reviewed it again, I think you can just move the class `XComObjectStoreBackend` to the provider and keep the bug fix in the base XCom backend (you don't need to separate that in two PRs if I'm not mistaken), but you will need to add a check for the Airflow version in the module (>=2.8.2) similar to what we do for min Airflow version: https://github.com/apache/airflow/blob/b15d5578dac73c4c6a3ca94d90ab0dc9e9e74c9c/airflow/providers/common/sql/__init__.py#L32-L42 -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
