-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
[AIRFLOW-5768] GCP cloud sql don't store ephemeral connection in db #6440
[AIRFLOW-5768] GCP cloud sql don't store ephemeral connection in db #6440
Conversation
Something like this gets my vote. WDYT @mik-laj? (It might be worth moving some of that logic down in to the base DbApiHook? Not sure.) |
Yeah I thought about this too and was also unsure. Just "connection" is a term that has great potential for conflict / confusion / ambiguity, to add it to all hooks could be trouble. Maybe if we renamed An alternative approach would be to alter the hook to accept every param the mysql connections (actual connection) would need, so that the GCP Cloud SQL hook does not actually need to create a connection object -- it can just pass the appropriate params to postgres / mysql hook. On this latter idea, some have discouraged adding params like |
88668ba
to
a521089
Compare
OK Just want to offer one more idea. If we don't like adding So we define something like this:
And then in
|
Codecov Report
@@ Coverage Diff @@
## master #6440 +/- ##
==========================================
- Coverage 83.99% 83.63% -0.36%
==========================================
Files 627 627
Lines 36537 36516 -21
==========================================
- Hits 30690 30541 -149
- Misses 5847 5975 +128
Continue to review full report at Codecov.
|
Oooh interesting idea, Connection usually implies and active connection/open socket, but as you say, our Connection objects are just credentials and connection information. Might be a thing to rename pre 2.0, at least in the code an models, if maybe not in the UI? |
Yep. Was on my list . Will do it latest tomorrow. |
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.
Really nice simplification! Love it.
Just a comment about target package in the light of recent AIP-21 clarifications and rebase is needed.
Ans sorry for so late review.
* add optional param `connection` to postgres and mysql hooks * instead of storing ephemeral connection to db, just pass directly to hook * remove obsoleted tests
e6a4382
to
8f31a8c
Compare
thanks @potiuk i have rebased |
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 really like it. Sorry it took so long!
Jira
Description
GCP cloud sql operator creates dynamically an ephemeral Connection object. It persists to metastore during execution and deletes afterward.
This behavior has negative impact on our ability to refactor creds management.
By not persisting to database, we can also remove some complexity re ensuring connection is deleted in event of failure, and the tests that go along with that.
This change requires that we add optional param
connection
to both MySqlHook and PostgresHook.Incidentally, this PR incorporates test simplifications originally in unmerged PR #6390 and will render that PR obsolete.
Tests
Commits
Documentation