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

Add ImpalaHook #26970

Merged
merged 2 commits into from Jan 6, 2023
Merged

Add ImpalaHook #26970

merged 2 commits into from Jan 6, 2023

Conversation

blcksrx
Copy link
Contributor

@blcksrx blcksrx commented Oct 10, 2022 •
edited

Description

Impala Hook

Use case / motivation

Related Issues
closes: #8916

@blcksrx blcksrx changed the title Impala Impala Hook Oct 10, 2022
uranusjr
uranusjr reviewed Oct 11, 2022
View reviewed changes
setup.py Outdated Show resolved Hide resolved
eladkal
eladkal requested changes Oct 12, 2022
View reviewed changes
Copy link
Contributor

@eladkal eladkal left a comment

Choose a reason for hiding this comment

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

We need a doc about how to setup connection. Check other peoviders for connection.rst examples

airflow/providers/apache/impala/provider.yaml Outdated Show resolved Hide resolved
airflow/providers/apache/impala/README.md Outdated Show resolved Hide resolved
@blcksrx
Copy link
Contributor Author

blcksrx commented Oct 12, 2022

Hwy guys thanks for the comments.
This is my old pr that I opened it.
Im going to work on it on this Friday

@blcksrx blcksrx force-pushed the impala branch 3 times, most recently from b6c7313 to 50bc2c5 Compare October 19, 2022 14:55
@bdsoha
Copy link
Contributor

bdsoha commented Oct 19, 2022 •
edited

@blcksrx You might want to consider allowing additional parameters to be passed to impala.dbapi.connect for more advanced configurations such as LDAP (auth_mechanism = "LDAP") etc.

The most flexible way would be to allow passing **kwargs.

@blcksrx
Copy link
Contributor Author

blcksrx commented Oct 19, 2022

@blcksrx You might want to consider allowing additional parameters to be passed to impala.dbapi.connect for more advanced configurations such as LDAP (auth_mechanism = "LDAP") etc.

The most flexible way would be to allow passing **kwargs.

It's already implemented by passing extra_dejson to the connect method. does it need to be added also in constructor?

@bdsoha
Copy link
Contributor

bdsoha commented Oct 19, 2022

@blcksrx You might want to consider allowing additional parameters to be passed to impala.dbapi.connect for more advanced configurations such as LDAP (auth_mechanism = "LDAP") etc.
The most flexible way would be to allow passing **kwargs.

It's already implemented by passing extra_dejson to the connect method. does it need to be added also in constructor?

Seems like that will do it.

@blcksrx blcksrx marked this pull request as draft October 19, 2022 21:19
@blcksrx blcksrx force-pushed the impala branch 2 times, most recently from 8606673 to 1a43b80 Compare October 21, 2022 09:27
@blcksrx blcksrx marked this pull request as ready for review October 21, 2022 09:48
@blcksrx blcksrx requested review from eladkal and removed request for potiuk, kaxil and mik-laj October 21, 2022 09:56
@blcksrx blcksrx force-pushed the impala branch 2 times, most recently from f27ed89 to 38761ea Compare October 23, 2022 09:49
@blcksrx blcksrx requested review from uranusjr and removed request for eladkal October 25, 2022 09:09
@blcksrx blcksrx force-pushed the impala branch 2 times, most recently from e52fb3c to d39e7cc Compare December 15, 2022 15:00
@eladkal
Copy link
Contributor

eladkal commented Dec 16, 2022 •
edited

@potiuk i took a look at CI. The wait for CI images steps takes 2 hours before timeout. Retry doesnt solve it.
I know something is missing/not right in the PR but I wasnt able to find exactly what we are missing here. Will you have time to give us some clues?

@blcksrx
Copy link
Contributor Author

blcksrx commented Dec 17, 2022

@eladkal Idk what was the problem, I just rebased it and it's done!

@eladkal
Copy link
Contributor

eladkal commented Dec 17, 2022

@blcksrx for some reason the thread on the airflow dependency was resolved but it wasn't added.
we need minimum apache-airflow>=2.3.0 as dependency in the provider yaml.

@eladkal
Copy link
Contributor

eladkal commented Dec 19, 2022

error to address:

Checking dist/apache-airflow-providers-apache-impala-2.4.1.dev0.tar.gz: FAILED
ERROR    `long_description` has syntax errors in markup and would not be        
         rendered on PyPI.

@blcksrx blcksrx force-pushed the impala branch 4 times, most recently from ef8b712 to d680ed0 Compare January 4, 2023 09:22
@blcksrx
impala hook
1636cc8 
Update airflow/providers/apache/impala/hooks/impala.py

Co-authored-by: Dov Benyomin Sohacheski <b@kloud.email>

Update tests/providers/apache/impala/hooks/test_impala.py

Co-authored-by: Dov Benyomin Sohacheski <b@kloud.email>

Update tests/providers/apache/impala/hooks/test_impala.py

Co-authored-by: Dov Benyomin Sohacheski <b@kloud.email>

Update tests/providers/apache/impala/hooks/test_impala.py

Co-authored-by: Dov Benyomin Sohacheski <b@kloud.email>

impala connection docs and pytest
@eladkal
Copy link
Contributor

eladkal commented Jan 5, 2023

Latest error was already fixed in #28725
I rebased the PR. lets see

@eladkal eladkal changed the title Impala Hook Add ImpalaHook Jan 6, 2023
@eladkal eladkal merged commit 6d09fc7 into apache:main Jan 6, 2023
@eladkal
Copy link
Contributor

eladkal commented Jan 6, 2023

🎉🎉🎉🎉

@potiuk
Copy link
Member

potiuk commented Jan 6, 2023

🎉

@blcksrx
Copy link
Contributor Author

blcksrx commented Jan 6, 2023

🥳

@pierrejeambrun pierrejeambrun added this to the Airflow 2.6.0 milestone Jan 9, 2023
@pierrejeambrun pierrejeambrun added the type:new-feature Changelog: New Features label Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uranusjr uranusjr uranusjr approved these changes

@Taragolis Taragolis Taragolis left review comments

@bdsoha bdsoha bdsoha approved these changes

@eladkal eladkal eladkal approved these changes

@potiuk potiuk Awaiting requested review from potiuk potiuk is a code owner

@dstandish dstandish Awaiting requested review from dstandish

@jedcunningham jedcunningham Awaiting requested review from jedcunningham

Assignees
No one assigned
Labels
area:dev-tools area:providers kind:documentation type:new-feature Changelog: New Features
Projects
None yet
Milestone
Airflow 2.6.0
Development

Successfully merging this pull request may close these issues.

Impala Hook
7 participants
@blcksrx @bdsoha @potiuk @eladkal @uranusjr @Taragolis @pierrejeambrun