Databricks SQL Sensor #30428

harishkesavarao · 2023-04-02T08:49:46Z

Databricks SQL Sensor.

boring-cyborg · 2023-04-02T08:49:51Z

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:

Pay attention to the quality of your code (ruff, mypy and type annotations). Our pre-commits will help you with that.
In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
Be sure to read the Airflow Coding style.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack

harishkesavarao · 2023-04-02T08:55:46Z

@alexott @bilalaslamseattle @janvandervegt @o-nikolas @josh-fell - thank you for taking the time to review my PR. I have made all the changes requested for PR #30204 here. I had to open a new one because of GitHub handles. Can you please review once more and confirm if the changes look good? Thank you!

airflow/providers/databricks/sensors/sql.py

docs/apache-airflow-providers-databricks/operators/sql.rst

alexott · 2023-04-02T15:06:38Z

@potiuk @josh-fell it looks like there are problems with Docker auth - is this error critical?

alexott

small changes are necessary + comments from Bilal

docs/apache-airflow-providers-databricks/operators/sql.rst

airflow/providers/databricks/sensors/sql.py

tests/system/providers/databricks/example_databricks_sensor.py

potiuk · 2023-04-02T15:54:01Z

@potiuk @josh-fell it looks like there are problems with Docker auth - is this error critical?

Not sure if critical but for sure intermittent (and likely a failure on GitHub size). Rebase should attempt to re-run the job.

harishkesavarao · 2023-04-03T02:38:29Z

small changes are necessary + comments from Bilal

@alexott @bilalaslamseattle I have made all the changes requested. Can you please review again?

bilalaslamseattle

LGTM

hussein-awala · 2023-04-04T00:09:06Z

airflow/providers/databricks/sensors/sql.py

I think for the naming convention, it's better if we use _get_hook without @cached_property or if we rename the method to hook which is the name of the property. WDYT? (I prefer renaming it to hook)

+1 for

@cached_property def hook(self) -> DatabricksSqlHook: ...

Changed it.

josh-fell · 2023-04-04T01:52:23Z

airflow/providers/amazon/aws/transfers/sftp_to_s3.py

Was this file change intended as part of this PR?

This isn't part of the change. It got added when I rebased the PR with main.

@josh-fell not sure why it keeps happening, every time I rebase my branch with main, it keeps adding new files to my PR. Any suggestions to remove them and keep only my files?

Probably you do some mistake when rebasing. But what it is, hard to say. You can follow the "--rebase" pattern - it should not add files. You can use the GitHub UI to rebase (eaasier) or command line (more powerful and more arcane for those who are not used to git cli): see "How to Rebase" section https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#id14

I recommend to learn and master the CLI version, once you get used to doing it and do it often, it gets into a habit. Also things like liquidprompt or oh-my-zsh and friends help a lot with seeing the status of your git repo in the command prompt are immensely helpful to see if you are doing it right. Usually rebase should result with only your commits that should be pushed on top of the current master.

Thank you @potiuk, I rebased from the UI a few times and I am not sure what went wrong. I can clean things up and open a new PR if needed.

@potiuk I had the same problem 4 days ago (#29913), I had to clean it locally then force push.

Yeah seems liek a recurring issue last few days https://apache-airflow.slack.com/archives/CCPRP7943/p1680639118757579 - created an issue in GitHub Support for that.

Thank you for letting me know @potiuk. Is there a remediation path to resolve the extra files in the PR?

I will follow what @hussein-awala did to clean this up.

There are still leftovers of unrelated changes that needs to be cleaned

josh-fell · 2023-04-04T02:01:53Z

airflow/providers/databricks/sensors/sql.py

Since both the DatabrickSqlHook and DatabricksSqlOperator are both in "databricks_sql.py" files, probably makes sense to keep the file name convention for this new sensor file as well. Even though it's borderline non-compliant with AIP-21, consistency is probably worth having given they all deal with the same service.

Renamed it to databricks_sql.py to keep it consistent with the hook and operator files.

josh-fell · 2023-04-04T02:04:47Z

airflow/providers/databricks/sensors/sql.py

Suggested change

raise AirflowException("Both HTTP Path and SQL warehouse name are not specified.")

raise AirflowException("Databricks SQL warehouse/cluster configuration missing. Please specify either http_path or sql_warehouse_name.")

The current exception might be misleading to some users. It reads as though both http_path and sql_warehouse_name need to be specified. WDYT?

Agree, added the change.

…fied

harishkesavarao · 2023-04-04T03:43:18Z

@hussein-awala @josh-fell pushed changes based on PR comments, can you please review again? Thank you for your review!

boring-cyborg bot added area:providers area:system-tests kind:documentation provider:databricks labels Apr 2, 2023

bilalaslamseattle reviewed Apr 2, 2023

View reviewed changes

alexott suggested changes Apr 2, 2023

View reviewed changes

docs/apache-airflow-providers-databricks/operators/sql.rst Outdated Show resolved Hide resolved

airflow/providers/databricks/sensors/sql.py Outdated Show resolved Hide resolved

tests/system/providers/databricks/example_databricks_sensor.py Outdated Show resolved Hide resolved

harishkesavarao force-pushed the db-sql-sensor branch from 78dcf62 to e8217dd Compare April 3, 2023 02:04

bilalaslamseattle approved these changes Apr 3, 2023

View reviewed changes

harishkesavarao force-pushed the db-sql-sensor branch 2 times, most recently from d58fd51 to f297270 Compare April 3, 2023 21:11

harishkesavarao requested review from eladkal and o-nikolas as code owners April 3, 2023 21:59

hussein-awala reviewed Apr 4, 2023

View reviewed changes

josh-fell reviewed Apr 4, 2023

View reviewed changes

harishkrao and others added 10 commits April 3, 2023 20:22

Databricks SQL sensor

a8702d6

Added unit test to check if sql endpoint or http path have been speci…

918a8d0

…fied

Removed unused imports, variables

1f3f352

Renamed SQL endpoint to SQL warehouse

c07a222

PR changes

8d105ec

Rearranged import after static checks

039d646

sql warehouse or http path

f804b5f

Removed blank lines

122fc1c

End before

c923c41

PR changes

4620f35

harishkesavarao and others added 2 commits April 3, 2023 21:31

Changed provider entry

1390a10

Merge branch 'apache:main' into db-sql-sensor

d23287a

harishkesavarao closed this Apr 5, 2023

harishkesavarao deleted the db-sql-sensor branch April 5, 2023 03:59

harishkesavarao mentioned this pull request Apr 5, 2023

Databricks SQL sensor #30477

Merged

	raise AirflowException("Both HTTP Path and SQL warehouse name are not specified.")
	raise AirflowException("Databricks SQL warehouse/cluster configuration missing. Please specify either http_path or sql_warehouse_name.")

Databricks SQL Sensor #30428

Databricks SQL Sensor #30428

Uh oh!

Conversation

harishkesavarao commented Apr 2, 2023

Uh oh!

boring-cyborg bot commented Apr 2, 2023

Uh oh!

harishkesavarao commented Apr 2, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alexott commented Apr 2, 2023

Uh oh!

alexott left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

potiuk commented Apr 2, 2023

Uh oh!

harishkesavarao commented Apr 3, 2023

Uh oh!

bilalaslamseattle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

potiuk Apr 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harishkesavarao commented Apr 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

potiuk Apr 4, 2023 •

edited

Loading