Databricks ClusteState & Clsuter GET API by Seokyun-Ha · Pull Request #34071 · apache/airflow

Seokyun-Ha · 2023-09-04T08:51:31Z

closes: #19490

Implement Databricks ClusterState class
Databricks cluster relating functions
- get_cluster()
- async_get_cluster()
- activate_cluster()
renaming a_get_... methods to async_get_...
Handle unexpected states of RunState, ClusterState on__init__() level.

Organization: @bagelcode-data
Co-workers: @Seokyun-Ha, @bskim45, @kyeonghoon-kim

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

boring-cyborg · 2023-09-04T08:51:35Z

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.
Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
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

Lee-W

I think we might need some test cases as well

airflow/providers/databricks/hooks/databricks.py

Seokyun-Ha · 2023-09-07T03:06:38Z

We are making mock testing code. After the task, we will let you know 😄 w/ @kyeonghoon-kim

Seokyun-Ha · 2023-09-07T10:09:46Z

Hello, @Lee-W @hussein-awala, We've applied all your feedbacks! 😄 Also, we implemented mock testing code as well. Please, take a look 🙏 Thanks!

* add TestClusterState * rename a_ to async_ * add get_cluster_state test * add GET_CLUSTER_RESPONSE * add async * add acitvate_cluster test * add TestClusterState * rename a_ to async_ * add get_cluster_state test * add GET_CLUSTER_RESPONSE * add async * add acitvate_cluster test * review * review * review * review

Seokyun-Ha · 2023-09-08T01:46:15Z

We checked some test are failed :(
We will take a look and fix them!

* fix lint * fix lint

* fix test * fix test * fix line too long

Seokyun-Ha · 2023-09-08T10:08:03Z

We checked our code works well using mock test and validation 😄 Please, take a look at it 🙏

potiuk · 2023-09-11T09:51:19Z

cc: @alexott ?

# Conflicts: # airflow/providers/databricks/hooks/databricks.py

Seokyun-Ha · 2023-09-14T02:48:57Z

Hello, @alexott . We wrote some code for get ClusterState and others. Please take a look at :)

Lee-W · 2023-09-19T10:13:09Z

airflow/providers/databricks/hooks/databricks.py

+        api_called = False
+        time_start = time.time()
+
+        while True:


Do we want to add an option for users to trigger the API simply but not wait for it?

I think just triggering the API is already implemented in start_cluster method on line 562

If that's the case, should we consolidate them into one method? For me, I can not understand the logic difference by reading the names start_cluster and activate_cluster. But this's not a huge issue though.

When start a Databricks Cluster, it tunrs on as PENDING state, so we cannot use the cluster until it's on ready state. That's the what start_cluster() method does.

Meanwhile, active_cluster() method guarantees the cluster is on Running State, so after the completed, we can utilize the cluster immediately.

Sometimes users want just starting the cluster, or want making the cluster into the usable status.

Yep, I know it's a desired behavior. Just wondering if it would be possible for us to merge the method and toggle the waiting behavior through a parameter

Yes, maybe. I now understand your suggestion. I think callingactive_cluster() method without polling and timeout is same as calling start_cluster(). So, if I change the code it looks like this

def start_cluster(self, json: dict, polling: int | None = None, timeout: int | None = None) -> None: cluster_id = json["cluster_id"] api_called = False time_start = time.time() while True: run_state = self.get_cluster_state(cluster_id) if run_state.is_running: return elif run_state.is_terminal: if api_called: raise AirflowException( f"Cluster {cluster_id} start failed with '{run_state.state}' " f"state: {run_state.state_message}" ) # This part changed # self.start_cluster(json) self._do_api_call(START_CLUSTER_ENDPOINT, json) api_called = True # This part changed if polling: # wait for cluster to start time.sleep(polling) else: return elapsed_time = time.time() - time_start if timeout and elapsed_time > timeout: raise AirflowException(f"Cluster {cluster_id} start timed out after {timeout} seconds")

Please, take a look and get some opinions 🙏

+ I think then we can apply this to restart_cluster() method too!

I would prefer to have one method only, with default to return immediately, but allow to wait until start/rester when specifying options.

tests/providers/databricks/hooks/test_databricks.py

alexott · 2023-09-24T11:59:08Z

airflow/providers/databricks/hooks/databricks.py

-    async def a_get_run_page_url(self, run_id: int) -> str:
+    async def async_get_run_page_url(self, run_id: int) -> str:


this & other similar changes could be considered as breaking. There is quite heavy direct use of the hook. I suggest that we rename, but leave functions with original names calling the new names, but adding deprecation warnings to them.

I agree, the best approach is by renaming it as you did, then create a new method deprecated method with the old name which call the new name method:

async def a_get_run_page_url(self, run_id: int) -> str: warnings.warn( "This method is deprecated. Please use `<path to the new method>` instead.", AirflowProviderDeprecationWarning, stacklevel=2, ) return await async_get_run_page_url(run_id= run_id)

Then we can remove it in the next major version

alexott · 2023-09-24T12:00:48Z

airflow/providers/databricks/hooks/databricks.py

        """
        self._do_api_call(START_CLUSTER_ENDPOINT, json)

+    def activate_cluster(self, json: dict, polling: int, timeout: int | None = None) -> None:


Maybe call it start_cluster_and_wait ?

alexott · 2023-09-24T17:22:03Z

airflow/providers/databricks/hooks/databricks.py

+        api_called = False
+        time_start = time.time()
+
+        while True:


I would prefer to have one method only, with default to return immediately, but allow to wait until start/rester when specifying options.

Seokyun-Ha · 2023-09-27T07:06:52Z

Hello, guys, I and @kyeonghoon-kim discussed about this active_cluster and this PR. We noticed this PR has too many changes on DatabricksHook and other. So, we decided to separate this PR into smaller ones.

Kind of changes

RunState, ClusterState and get_cluster_state()
- We will open PR for this as soon as possible
active_cluster() method
- This method can be replaced into Airflow Sensor. We will implement a sensor for Databricks Cluster State in future PR. It will monitor cluster state until the desired state met.
async_get_xxx()
- This change are dropped, because they make breaking changes.

Thanks for your kind and sincere reviews 🙏 : We will close this PR after we make a new separated PR 😄
cc. @Lee-W @hussein-awala @alexott

github-actions · 2023-12-16T00:11:45Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

boring-cyborg bot added area:providers provider:databricks labels Sep 4, 2023

Seokyun-Ha force-pushed the databricks-hook-cluster-manage branch from a5c4beb to 77376fb Compare September 4, 2023 09:29

Seokyun-Ha mentioned this pull request Sep 4, 2023

provider Databricks : add cluster API #19490

Closed

2 tasks

Lee-W reviewed Sep 4, 2023

View reviewed changes

hussein-awala self-requested a review September 4, 2023 11:20

Seokyun-Ha requested a review from Lee-W September 5, 2023 05:12

hussein-awala reviewed Sep 5, 2023

View reviewed changes

Lee-W reviewed Sep 6, 2023

View reviewed changes

airflow/providers/databricks/hooks/databricks.py Show resolved Hide resolved

airflow/providers/databricks/hooks/databricks.py Outdated Show resolved Hide resolved

airflow/providers/databricks/hooks/databricks.py Outdated Show resolved Hide resolved

Seokyun-Ha changed the title ~~Databricks ClusteState & Clsuter get, activate~~ Databricks ClusteState & Clsuter GET API Sep 6, 2023

Seokyun-Ha and others added 17 commits September 7, 2023 20:07

implt cluster state & get/activate cluster

6a05017

quote -> dquote

4129d4c

use state_msg

65a2012

condition expr formatting

d58918b

fix new methods mistakes

31ebaa1

airflow ci failed resolves

8025efe

raise Exception after timeout

d1c9e99

oops mistake...

091e1e6

clearity: a_get -> async_get

a7c3274

quote -> dquote

92fbf7f

real elapsed time

ad0a561

update commenets

5ea2e08

handle exception raise first

3be8ef2

check unexpected state on constructure level

0f14140

formatting: move and front side

8e9f766

fix state validation logic

9a77a6e

fix to pass pre-commit test

1b30c4e

potiuk force-pushed the databricks-hook-cluster-manage branch from 1e921cc to 1b30c4e Compare September 7, 2023 18:07

fix mistake

1a5d678

kyeonghoon-kim and others added 3 commits September 8, 2023 12:05

fix lint (#3)

e9ddb98

* fix lint * fix lint

fix test (#5)

50684ca

* fix test * fix test * fix line too long

fix activate timeout mistake

6d0539f

Seokyun-Ha requested review from Lee-W and hussein-awala September 11, 2023 07:32

Merge branch 'main' into databricks-hook-cluster-manage

7ddc770

# Conflicts: # airflow/providers/databricks/hooks/databricks.py

Merge branch 'main' into databricks-hook-cluster-manage

24c003e

Lee-W reviewed Sep 19, 2023

View reviewed changes

use parameterize (#6)

3606e8f

alexott reviewed Sep 24, 2023

View reviewed changes

Merge branch 'apache:main' into databricks-hook-cluster-manage

b827967

github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Dec 16, 2023

github-actions bot closed this Dec 25, 2023

		async def a_get_run_page_url(self, run_id: int) -> str:
		async def async_get_run_page_url(self, run_id: int) -> str:

Comments

Conversation

Seokyun-Ha commented Sep 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

boring-cyborg bot commented Sep 4, 2023

Uh oh!

Lee-W left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Seokyun-Ha commented Sep 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Seokyun-Ha commented Sep 7, 2023

Uh oh!

Seokyun-Ha commented Sep 8, 2023

Uh oh!

Seokyun-Ha commented Sep 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

potiuk commented Sep 11, 2023

Uh oh!

Seokyun-Ha commented Sep 14, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyeonghoon-kim Sep 20, 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!

Seokyun-Ha Sep 22, 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!

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!

Seokyun-Ha commented Sep 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Dec 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Seokyun-Ha commented Sep 4, 2023 •

edited

Loading

Seokyun-Ha commented Sep 7, 2023 •

edited

Loading

Seokyun-Ha commented Sep 8, 2023 •

edited

Loading

kyeonghoon-kim Sep 20, 2023 •

edited

Loading

Seokyun-Ha Sep 22, 2023 •

edited

Loading

Seokyun-Ha commented Sep 27, 2023 •

edited

Loading