-
Notifications
You must be signed in to change notification settings - Fork 119
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
cancel python model job when dbt exit #690
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -77,6 +77,8 @@ | |
from dbt_common.utils import cast_to_str | ||
from requests import Session | ||
|
||
from dbt.adapters.databricks.python_submissions import BaseDatabricksHelper | ||
|
||
if TYPE_CHECKING: | ||
from agate import Table | ||
|
||
|
@@ -475,6 +477,14 @@ class DatabricksConnectionManager(SparkConnectionManager): | |
TYPE: str = "databricks" | ||
credentials_provider: Optional[TCredentialProvider] = None | ||
|
||
def cancel_open(self) -> List[str]: | ||
for run_id in BaseDatabricksHelper.run_ids: | ||
logger.debug(f"Cancel python model job: {run_id}") | ||
BaseDatabricksHelper.cancel_run_id(run_id, self.credentials_provider.as_dict()['token'], self.credentials_provider.as_dict()['host']) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah, this is where you need to retrieve, and here you don't have an instance...we can use singleton pattern maybe? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. give me an hour to take a crack at refactoring this; I have an idea :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Singleton maybe its a way, Let me try some code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah! thank you very much! |
||
BaseDatabricksHelper.run_ids.clear() | ||
return super().cancel_open() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Below you raise an exception on a non-200, but that will interrupt cancelling the other operations. Better to log a warning on non-200 I think. |
||
|
||
|
||
def compare_dbr_version(self, major: int, minor: int) -> int: | ||
version = (major, minor) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why store this on the token? It's already on the DatabricksCredentials.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like I can't get DatabricksCredentials in DatabricksConnectionManager.
I can just use self.credentials_provider in DatabricksConnectionManager, but there are no host in credentials_provider, so I put a host in the token_auth class.
Could you give me a direction how to get DatabricksCredentials in DatabricksConnectionManager?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The BaseDatabricksHelper has a copy of DatabricksCredentials.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, but that's an instance...let me think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to pull down a copy of this PR and see if i can figure it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes..think I can't get instance in DatabricksConnectionManager...
Thank you very much!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, did you already fix this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I can't found a way.. so I still use the way put host in token_auth. so that the host can be retrieve from DatabricksConnectionManager.credentials_provider.