search

astronomer / astro-provider-databricks

Orchestrate your Databricks notebooks in Airflow and execute them as Databricks Workflows
Apache License 2.0
20 stars 10 forks source link

Pass api version to jobs API calls across operator implementations #66

Closed pankajkoti closed 3 months ago

pankajkoti commented 3 months ago

Based on a user feedback, they would like to use Databricks API version 2.1 for the various Databricks SDK calls from the operator implementations. Hence, use the existing configurable API version constant fetched from an environment variable and pass it in places where Databricks API calls are made from the Databricks SDK across the operators implementations.

closes: https://github.com/astronomer/issues-airflow/issues/614

pankajkoti commented 3 months ago

dev/logs/scheduler/latest -- is this intentionally removed?

yes, looks like the log file was pushed to git erroneously earlier. Hence, removed it from Git tracking.

pankajkoti commented 3 months ago

@pankajkoti it looks good overall!

Some feedback:

  1. It seems we don't have any tests that confirm if users set an environment variable, it will be forwarded to the API calls - which I understood is the goal with this ticket

The constant JOBS_API_VERSION that fetches this environment variable takes a default value if an env variable is not set. And thus we have a set value that gets passed to the API calls. The ticket was that although we had this constant, it was not getting passed across all the API calls and this is what we are trying to support in the PR.

  1. Were we exposing this environment variable before? It feels that JOBS_API_VERSION can be misleading - if we could expose it as DATABRICKS_JOBS_API_VERSION

Happy to rename it to so. Will create an immediate follow up PR.

  1. This was there before, but since we're using this as a setting, and not really a constant, it would be great if we had setttings.py as opposed to constants.py

Yes, makes sense. Will create an immediate follow up PR.

Edit: Follow up PR to address the comments https://github.com/astronomer/astro-provider-databricks/pull/68