Skip to content

Added Hook for Amazon RDS #20642

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
Merged
potiuk merged 1 commit into from
Feb 25, 2022
Merged

Added Hook for Amazon RDS #20642

potiuk merged 1 commit into from
Feb 25, 2022

Conversation

@kazanzhy
Copy link
Contributor

@kazanzhy kazanzhy commented Jan 3, 2022 •
edited
Loading

I had two different implementations of RDSHook.

dstandish
dstandish reviewed Jan 4, 2022
View reviewed changes
@kazanzhy kazanzhy force-pushed the add_aws_rds_hooks branch 2 times, most recently from aec5a1c to ae53b0d Compare January 5, 2022 22:14
eladkal
eladkal reviewed Jan 6, 2022
View reviewed changes
@kazanzhy kazanzhy force-pushed the add_aws_rds_hooks branch 3 times, most recently from 03a38e2 to d093539 Compare January 10, 2022 23:21
@kazanzhy kazanzhy marked this pull request as ready for review January 11, 2022 13:40
dstandish
dstandish requested changes Jan 11, 2022
View reviewed changes
dstandish
dstandish reviewed Jan 12, 2022
View reviewed changes
Copy link
Contributor

@dstandish dstandish left a comment

Choose a reason for hiding this comment

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

looks good to me apart from the one file i have question about

@github-actions
Copy link

github-actions bot commented Jan 12, 2022

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.

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Jan 12, 2022
@kazanzhy kazanzhy force-pushed the add_aws_rds_hooks branch 4 times, most recently from bdd5e00 to fb2567e Compare January 17, 2022 14:48
@kazanzhy kazanzhy mentioned this pull request Jan 17, 2022
Merged
eladkal
eladkal requested changes Jan 20, 2022
View reviewed changes
@kazanzhy kazanzhy force-pushed the add_aws_rds_hooks branch 2 times, most recently from 69157ae to c5451fd Compare January 24, 2022 15:04
@kazanzhy kazanzhy force-pushed the add_aws_rds_hooks branch 2 times, most recently from 254c8c2 to 36a6a4f Compare January 29, 2022 15:03
@kazanzhy kazanzhy force-pushed the add_aws_rds_hooks branch 3 times, most recently from 2308501 to 2432fe2 Compare February 18, 2022 18:40
@dstandish
Copy link
Contributor

dstandish commented Feb 18, 2022

@potiuk i'm a bit confused why we are seeing failures like this in this pr... https://github.com/apache/airflow/runs/5252964277?check_suite_focus=true#step:11:1533
anything pop out to you re why?

@kazanzhy
Copy link
Contributor Author

kazanzhy commented Feb 18, 2022 •
edited
Loading

So there are three ways to install stubs for RDS.

  • boto3-stubs[rds] - ERRORS
  • boto3-stubs-lite[rds] - ERRORS
  • mypy-boto3-rds - OK

Seems boto3-stubs overload something in botocore.
I'll try to find the solution because it's more convenient to put all necessary libs inside boto3-stubs than list them.

@kazanzhy kazanzhy force-pushed the add_aws_rds_hooks branch 2 times, most recently from 2192a54 to 645365c Compare February 20, 2022 15:15
@kazanzhy kazanzhy closed this Feb 20, 2022
@kazanzhy kazanzhy reopened this Feb 20, 2022
@dstandish
Copy link
Contributor

dstandish commented Feb 23, 2022

So there are three ways to install stubs for RDS.
boto3-stubs[rds] - ERRORS
boto3-stubs-lite[rds] - ERRORS
mypy-boto3-rds - OK
Seems boto3-stubs overload something in botocore.
I'll try to find the solution because it's more convenient to put all necessary libs inside boto3-stubs than list them.

Nice! glad you figured it out.

@dstandish dstandish force-pushed the add_aws_rds_hooks branch 4 times, most recently from 168cb4c to f0ef165 Compare February 25, 2022 06:23
@potiuk
Copy link
Member

potiuk commented Feb 25, 2022

@kazanzhy @dstandish - I would really love if you could upvote and comment here: pypa/pip#10258 (comment).

The problem is that we have no way currently to determine what is the root cause of the problem. The problem is that pip is not able to solve dependencies in a reasonable time and runs backtracking for hours - but it does not really give us the possibilty to get the root cause message that could allow us to determine how we could help pip.

I proposed to add optional timeout on pip install execution producing a "conflict" message if the timeout is reached. That could give us at least a chance to understand what is the problem (someone released a new dependency that makes pip fail installing airflow from the scratch and we need to help pip to determine the right set of dependencies because it is unable to figure it out on its own).

@potiuk
Copy link
Member

potiuk commented Feb 25, 2022

@kazanzhy @dstandish I think I managed to workaround the pip resolver issue with #21824 - please rebase to latest main.

@dstandish
Copy link
Contributor

dstandish commented Feb 25, 2022

I would really love if you could upvote and comment here:

done

@kazanzhy @dstandish I think I managed to workaround the pip resolver issue with #21824 - please rebase to latest main.

thanks a bunch!

@dstandish
Copy link
Contributor

dstandish commented Feb 25, 2022

finally we can merge. thanks again to @potiuk for resolving the pip problem, and thank you @kazanzhy for your persistence and patience.

@potiuk potiuk merged commit 0378659 into apache:main Feb 25, 2022
@kazanzhy kazanzhy deleted the add_aws_rds_hooks branch February 28, 2022 14:38
@jedcunningham jedcunningham added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Feb 28, 2022
@potiuk potiuk mentioned this pull request Mar 7, 2022
Closed
84 tasks
@kazanzhy kazanzhy mentioned this pull request Mar 9, 2022
Merged
@potiuk potiuk mentioned this pull request Mar 15, 2022
Closed
41 tasks
@kazanzhy kazanzhy mentioned this pull request Aug 21, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dstandish dstandish dstandish approved these changes

@potiuk potiuk potiuk approved these changes

@eladkal eladkal eladkal approved these changes

@mik-laj mik-laj Awaiting requested review from mik-laj

@vikramkoka vikramkoka Awaiting requested review from vikramkoka

Assignees

No one assigned

Labels

area:providers changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) full tests needed We need to run full set of tests for this PR to merge kind:documentation provider:amazon AWS/Amazon - related issues

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@kazanzhy @potiuk @dstandish @eladkal @jedcunningham