-
Notifications
You must be signed in to change notification settings - Fork 14.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix gcs listing - ensure blobs are loaded #34919
Conversation
78676a8
to
a36625a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I'd like to note that it happens only with specifying prefix
and delimiter
.
With match_glob
it works as expected without this patch.
@@ -829,10 +829,12 @@ def _list( | |||
versions=versions, | |||
) | |||
|
|||
all_blobs = list(blobs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this issue is specific to using prefix
and delimiter
, it would be better for future debugging to convert it into blobs = list(blobs)
within the else
block above (and revert the later reference to blobs
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shahar1 thanks for taking a look at this, much appreciated. I've moved the list(blobs)
into the above else
as requested. I tested this locally using prefix
with match_glob
and then, prefix
with delimiter
and both code paths appear to work fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed this was also broken for list_by_timespan
, I've fixed that as well and squashed the commits.
56e512f
to
10b0f3e
Compare
LGTM :) |
@atrbgithub can you rebase? (You disabled allowing mantainers to do changes on your branch so I can't do it for you) |
10b0f3e
to
7319319
Compare
@eladkal My apologies that was not intentional. I've rebased, I will look to get that setting changed. Apparently this is a known issue when creating a PR from a repo which is under an organisation - https://github.com/orgs/community/discussions/5634 |
I've raised #35884 as an alternative, which is outside the org and allows |
No need :) |
Great thanks @eladkal 👍 |
Hey @atrbgithub looks like Sensor
Error
|
@pankajastro thanks for raising, I have mentioned this here and asked for the change not to be merged in. |
@pankajastro I've raised a PR to fix this #36130 Would you be able to retest? Edit - A new PR has been raised to address this #36202 |
This fixes #34909
Performing the list of the blobs appears to force the blobs to be loaded rather than lazily loaded.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rst
or{issue_number}.significant.rst
, in newsfragments.