Adding DatabricksSQLStatementSensor Sensor with Deferrability
#49516
Conversation
|
cc: @pankajkoti, I've created a draft PR. Do you mind take a look at this? |
jroachgolf84
changed the title
DatabricksSQLStatementSensor Operator with DeferrabilityDatabricksSQLStatementSensor Sensor with Deferrability
|
Team, can I please request a review on this issue? |
|
LGTM. The code looks good but I'd appreciate another review @alexott maybe ? |
alexott
left a comment
alexott
left a comment
There was a problem hiding this comment.
We need to fix some small things, but it's good in general
providers/databricks/tests/system/databricks/example_databricks_sensors.py
Outdated
Show resolved
Hide resolved
providers/databricks/tests/system/databricks/example_databricks_sensors.py
Outdated
Show resolved
Hide resolved
providers/databricks/src/airflow/providers/databricks/sensors/databricks.py
Outdated
Show resolved
Hide resolved
|
I'll go ahead and make those changes. Thanks! |
|
Thanks for the feedback - I've implemented those changes. |
providers/databricks/src/airflow/providers/databricks/sensors/databricks.py
Show resolved
Hide resolved
providers/databricks/tests/system/databricks/example_databricks_sensors.py
Outdated
Show resolved
Hide resolved
pankajkoti
left a comment
pankajkoti
left a comment
There was a problem hiding this comment.
LGTM. Have some inline comments about docs. I was also able to do a quick sanity run on example DAGs using the operator and sensor. Thanks for contributing this. And apologies for the delay in the review here, was caught up with getting a release ready at work 🙇🏽
I am good to merge once the comments from @alexott are considered/resolved.
providers/databricks/tests/system/databricks/example_databricks_sensors.py
Outdated
Show resolved
Hide resolved
|
Changes made and implemented! |
|
Are we good to go ahead and merge this? |
|
@eladkal, changes implemented, should be all set to go! |
…he#49516) * Adding mixin, Sensor, and updated Operator to create a draft PR * Prepping to update branch * Adding tests for DatabricksSQLStatementSensor * Added unit tests and examples * Added documentation, pointed ignore tests on mixin * Implementing changes requested in PR * Tweaking example DAG * Adding validation, updating docs * Updated example DAG * Adding test for Databricks mixins * Updated test_project_structure.py
6 participants
Pull request to add
DatabricksSQLStatementsSensorSensor with deferrability.Currently, there is an existing
DatabricksSqlSensor. However, this Sensor has faulty implementation, and actually executes a query on eachpoke. Recently, #48507 was merged by @pankajkoti to create aDatabricksSQLStatementsOperator. This Sensor quite closely mirrors that implementation, but with a few distinct capabilities.statement_id, rather than a SQLstatement.This PR also introduces the
DatabricksSQLStatementsMixinto take much of the overlapping logic betweenDatabricksSQLStatementsSensorandDatabricksSQLStatementsOperatorand make it reusable.These changes both the Operator and Sensor were fully tested, and an example Task was added.
Closes: #48368