Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Deferrable mode for S3ToGCSOperator #29462

Merged
merged 1 commit into from
Jul 11, 2023
Merged

Deferrable mode for S3ToGCSOperator #29462

merged 1 commit into from
Jul 11, 2023

Conversation

moiseenkov
Copy link
Contributor

^ 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.

@moiseenkov moiseenkov force-pushed the s3_to_gcs_deferrable_mode branch 4 times, most recently from 6925d4e to 3469f13 Compare February 13, 2023 10:37
@moiseenkov moiseenkov force-pushed the s3_to_gcs_deferrable_mode branch 9 times, most recently from c6b8ac5 to 74f43c0 Compare February 20, 2023 08:24
@moiseenkov
Copy link
Contributor Author

@kaxil , hi,
Could you review my fixes please?

kaxil
kaxil reviewed Feb 23, 2023
View reviewed changes
Copy link
Member

@kaxil kaxil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @pankajastro Could you review this too please

pankajastro
pankajastro reviewed Feb 24, 2023
View reviewed changes
airflow/providers/google/cloud/hooks/cloud_storage_transfer_service.py Outdated Show resolved Hide resolved
airflow/providers/google/cloud/transfers/s3_to_gcs.py Outdated Show resolved Hide resolved
airflow/providers/google/cloud/transfers/s3_to_gcs.py Outdated Show resolved Hide resolved
airflow/providers/google/cloud/triggers/cloud_storage_transfer_service.py Show resolved Hide resolved
tests/providers/google/cloud/hooks/test_cloud_storage_transfer_service_async.py Outdated Show resolved Hide resolved
tests/providers/google/cloud/hooks/test_cloud_storage_transfer_service_async.py Outdated Show resolved Hide resolved
airflow/providers/google/cloud/transfers/s3_to_gcs.py Show resolved Hide resolved
airflow/providers/google/cloud/transfers/s3_to_gcs.py Show resolved Hide resolved
airflow/providers/google/cloud/triggers/cloud_storage_transfer_service.py Show resolved Hide resolved
tests/providers/google/cloud/hooks/test_cloud_storage_transfer_service_async.py Outdated Show resolved Hide resolved
@moiseenkov moiseenkov force-pushed the s3_to_gcs_deferrable_mode branch 3 times, most recently from 955d134 to ef075b4 Compare February 24, 2023 15:07
@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label May 24, 2023
@moiseenkov moiseenkov force-pushed the s3_to_gcs_deferrable_mode branch 3 times, most recently from 24e653e to 323fc3b Compare July 5, 2023 07:17
@VladaZakharova
Copy link
Contributor

Hi @potiuk @pankajastro !
Could you please take a look on this PR? Thanks :)

potiuk
potiuk requested changes Jul 5, 2023
View reviewed changes
Copy link
Member

@potiuk potiuk left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had a recent change on how we treat default deferrable setting and we need to adjust this one too: #31712

One more case in point @Lee-W that we need to add a pre-commit checking the defferrable convention, otherwise we will end up will all kind of mess where some deferrable parameters will have the default values following the convention and some not.

@moiseenkov - can you please update the change to follow this patterm?
@Lee-W -> will it be possible that you add such pre-commit (#32355 (comment)) ?

@Lee-W
Copy link
Member

Lee-W commented Jul 5, 2023

@potiuk Sure! Sorry for the late reply. I'm already working on it.

@potiuk
Copy link
Member

potiuk commented Jul 5, 2023

@potiuk Sure! Sorry for the late reply. I'm already working on it.

Cool! :)

@moiseenkov
Copy link
Contributor Author

We had a recent change on how we treat default deferrable setting and we need to adjust this one too: #31712

One more case in point @Lee-W that we need to add a pre-commit checking the defferrable convention, otherwise we will end up will all kind of mess where some deferrable parameters will have the default values following the convention and some not.

@VladaZakharova - can you please update the change to follow this patterm? @Lee-W -> will it be possible that you add such pre-commit (#32355 (comment)) ?

Thanks for the update! I will update PR and ping you afterwards

@moiseenkov moiseenkov force-pushed the s3_to_gcs_deferrable_mode branch 3 times, most recently from 9967ad6 to 26f5de7 Compare July 5, 2023 09:44
@moiseenkov
Copy link
Contributor Author

Hi, @potiuk , @Lee-W ,

I updated the PR:
https://github.com/apache/airflow/pull/29462/files#diff-3e4eedf131305f589cec193af2b406b57a5eab1a3db181272996c7e4d03e6cc6R156

@Lee-W Lee-W mentioned this pull request Jul 5, 2023
Merged
@potiuk potiuk force-pushed the s3_to_gcs_deferrable_mode branch from 26f5de7 to 02cb3ac Compare July 5, 2023 18:29
Lee-W
Lee-W reviewed Jul 6, 2023
View reviewed changes
airflow/providers/google/cloud/hooks/cloud_storage_transfer_service.py Outdated Show resolved Hide resolved
airflow/providers/google/cloud/hooks/cloud_storage_transfer_service.py Outdated Show resolved Hide resolved
airflow/providers/google/cloud/hooks/cloud_storage_transfer_service.py Outdated Show resolved Hide resolved
airflow/providers/google/cloud/hooks/cloud_storage_transfer_service.py Outdated Show resolved Hide resolved
airflow/providers/google/cloud/hooks/cloud_storage_transfer_service.py Show resolved Hide resolved
airflow/providers/google/cloud/transfers/s3_to_gcs.py Outdated Show resolved Hide resolved
airflow/providers/google/cloud/transfers/s3_to_gcs.py Outdated Show resolved Hide resolved
airflow/providers/google/cloud/transfers/s3_to_gcs.py Outdated Show resolved Hide resolved
airflow/providers/google/cloud/triggers/cloud_storage_transfer_service.py Outdated Show resolved Hide resolved
tests/system/providers/google/cloud/gcs/example_s3_to_gcs_async.py Show resolved Hide resolved
@moiseenkov moiseenkov force-pushed the s3_to_gcs_deferrable_mode branch 3 times, most recently from ee22f81 to 4a23c8f Compare July 6, 2023 12:44
@moiseenkov moiseenkov requested a review from Lee-W July 6, 2023 12:46
@moiseenkov moiseenkov force-pushed the s3_to_gcs_deferrable_mode branch 2 times, most recently from a2e9949 to 51f7f61 Compare July 6, 2023 14:44
@moiseenkov
Combine 8 into 1
a2e4755 
Implement deferrable mode for S3ToGCSOperator

Update airflow/providers/google/cloud/hooks/cloud_storage_transfer_service.py

Co-authored-by: Wei Lee <weilee.rx@gmail.com>

Update airflow/providers/google/cloud/hooks/cloud_storage_transfer_service.py

Co-authored-by: Wei Lee <weilee.rx@gmail.com>

Update airflow/providers/google/cloud/transfers/s3_to_gcs.py

Co-authored-by: Wei Lee <weilee.rx@gmail.com>

Update airflow/providers/google/cloud/transfers/s3_to_gcs.py

Co-authored-by: Wei Lee <weilee.rx@gmail.com>

Update airflow/providers/google/cloud/transfers/s3_to_gcs.py

Co-authored-by: Wei Lee <weilee.rx@gmail.com>

Update airflow/providers/google/cloud/triggers/cloud_storage_transfer_service.py

Co-authored-by: Wei Lee <weilee.rx@gmail.com>

Address code review comments
@VladaZakharova
Copy link
Contributor

@potiuk
Hi there!
Could we recheck this PR? Thanks!

@potiuk potiuk merged commit 86c6cc9 into apache:main Jul 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pankajastro pankajastro pankajastro approved these changes

@potiuk potiuk potiuk approved these changes

@kaxil kaxil Awaiting requested review from kaxil

@ahidalgob ahidalgob Awaiting requested review from ahidalgob

@Lee-W Lee-W Awaiting requested review from Lee-W

Assignees
No one assigned
Labels
area:providers area:system-tests kind:documentation provider:google Google (including GCP) related issues
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@moiseenkov @potiuk @VladaZakharova @Lee-W @kaxil @ahidalgob @pankajastro