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.

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

Added timeout to w.clusters.ensure_cluster_running() #227

Merged
merged 7 commits into from
Aug 2, 2023
Merged
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 27 additions & 12 deletions databricks/sdk/mixins/compute.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
import datetime
import logging
import re
import time
from dataclasses import dataclass
from typing import Optional

from databricks.sdk.errors import OperationFailed
from databricks.sdk.service import compute

_LOG = logging.getLogger('databricks.sdk')


@dataclass
class SemVer:
Expand Down Expand Up @@ -203,16 +209,25 @@ def select_node_type(self,
return nt.node_type_id
raise ValueError("cannot determine smallest node type")

def ensure_cluster_is_running(self, cluster_id: str):
def ensure_cluster_is_running(self, cluster_id: str) -> compute.ClusterDetails:
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you need cluster details to be returned? this will have to be adjusted across all SDKs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't have to 🤷‍♂️ Just seemed more useful given that we have it than returning None.

Do you want me to revert that commit?

Copy link
Contributor

Choose a reason for hiding this comment

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

For now, yes.

Copy link
Contributor

Choose a reason for hiding this comment

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

@judahrand could you address this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry! I'd forgotten to follow up on this. Done now.

"""Ensures that given cluster is running, regardless of the current state"""
state = compute.State
info = self.get(cluster_id)
if info.state == state.TERMINATED:
self.start(cluster_id).result()
elif info.state == state.TERMINATING:
self.wait_get_cluster_terminated(cluster_id)
self.start(cluster_id).result()
elif info.state in (state.PENDING, state.RESIZING, state.RESTARTING):
self.wait_get_cluster_running(cluster_id)
elif info.state in (state.ERROR, state.UNKNOWN):
raise RuntimeError(f'Cluster {info.cluster_name} is {info.state}: {info.state_message}')
timeout = datetime.timedelta(minutes=20)
deadline = time.time() + timeout.total_seconds()
while time.time() < deadline:
try:
state = compute.State
info = self.get(cluster_id)
if info.state == state.RUNNING:
return info
elif info.state == state.TERMINATED:
return self.start(cluster_id).result()
elif info.state == state.TERMINATING:
self.wait_get_cluster_terminated(cluster_id)
return self.start(cluster_id).result()
elif info.state in (state.PENDING, state.RESIZING, state.RESTARTING):
return self.wait_get_cluster_running(cluster_id)
elif info.state in (state.ERROR, state.UNKNOWN):
raise RuntimeError(f'Cluster {info.cluster_name} is {info.state}: {info.state_message}')
except OperationFailed as e:
_LOG.debug('Operation failed, retrying', exc_info=e)
raise TimeoutError(f'timed out after {timeout}')