Reduce memory footprint of s3 key trigger #40473
Conversation
We can return True on the first positive. And we don't need to keep track of the files.
boring-cyborg
bot
added
area:providers
provider:amazon-aws
|
@eladkal fyi this would change this lets but it would be technically breaking so i guess require a major, which is why i mention to you. also the removal of the _async form of the decorator would also be technically "breaking" |
Please add 9.0.0 to versions list airflow/airflow/providers/amazon/provider.yaml Lines 27 to 28 in 26768d9 also please short description at the top of the change log |
I am not sure whether it's breaking. It changes the behaviour, for sure (like any other change) but the likelihood it actually breaks someone's workflow is very low. If you are iterating over the results (which is the intended usage of that method), things will continue to work. So the INTENTION of the API has not changed - some of the internal details of it and characteristics (less memory used, lazy evaluation) changed. Also - the decorators are by definition not part of the API - they are part of the method implementation and again - impact the implementation detail, but - unless you inspect details of the decorator - from the user perspective, nothing changed. IMHO this is literally just an optimization, and I would not qualify it as a breaking change. This is a great example of something that I advocate for quite some time - not every "method signature" change - even if technically breaking, is breaking. What reallly counts is the intentions of the one who wrote the method on how it should be used, matched with assesment of probabllity the changed thing was used properly - according to the intention. In this case, the intention has not changed, it should be used in the same way before and after. And IMHO probability that it was used not according to the intention and this change will break it, is very low. |
|
BTW. Yes it shoudl be mentioned in the changelog, but not as a breaking change IMHO. |
|
In that case all we need is just the info note I mentioned. |
Co-authored-by: Vincent <97131062+vincbeck@users.noreply.github.com>
This reverts commit cb2f31a.
|
ok @eladkal i went with the "it's a bugfix" approach and added note in changelog. PTAL |
|
Is there a way to tell the build steps that were done as part of PR that's already been merged? Doc builds are failing in main for the amazon provider, and I'm curious how they didn't fail in this PR. |
weird, maybe this https://github.com/apache/airflow/actions/runs/9716729092 |
Yeah, that's all I found as well, but that can't be the full set of tests since there is almost nothing there. Or maybe it was and that explains how it got through, I'm not sure. |
Click "View details" button at the last push : 
|
* Use generator to reduce memory footprint We can return True on the first positive. And we don't need to keep track of the files. * add tests * fixup * Update airflow/providers/amazon/aws/hooks/s3.py Co-authored-by: Vincent <97131062+vincbeck@users.noreply.github.com> * Revert "Update airflow/providers/amazon/aws/hooks/s3.py" This reverts commit cb2f31a. * reapply vincent's suggestion * add check for param * add changelog --------- Co-authored-by: Vincent <97131062+vincbeck@users.noreply.github.com>
Also had to make the provide_bucket_name decorator support async generators, and in the process i realized we didn't need a separate "async" version of the decorator.