-
Notifications
You must be signed in to change notification settings - Fork 72
Synapse Profiler Scripts #2020
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
base: main
Are you sure you want to change the base?
Synapse Profiler Scripts #2020
Conversation
✅ 29/29 passed, 2 flaky, 1m19s total Flaky tests:
Running from acceptance #2398 |
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.
we need a better interface to execute
I am not sure if we want to leave it to the developers to always initiate, the workspace and creds. this will lead to inconsistencies sooner or later.
I also would split extracting the "metrics" and persisting them so we need at least two methods extract
and persist
raise FileNotFoundError(f"Credentials file not found at {path}") from e | ||
|
||
|
||
def create_credential_manager(file_path: Path): |
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.
what is the difference from databricks.labs.lakebridge.connections.credential_manager.create_credential_manager
we should merge them together and make it reusable
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.
+1
return CredentialManager(loader, secret_providers) | ||
|
||
|
||
def get_sqlpool_reader( |
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.
src/databricks/labs/lakebridge/connections/database_manager.py
also implements a synapse client. what is the difference and we should merge them?
) | ||
|
||
|
||
def get_synapse_jdbc_settings(config: dict): |
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.
this is not used anywhere. please remove
return MetricsQueryClient(credential=DefaultAzureCredential()) | ||
|
||
|
||
def save_resultset_to_db(result, table_name: str, db_path: str, mode: str): |
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.
can we move the duckdb functions to their own file
return max_column_val | ||
|
||
|
||
def get_serverless_database_groups( |
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.
this is used in one place only, can go next to it
from sqlalchemy import text | ||
|
||
|
||
def execute(): |
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 long execute methods need to be split up. as this is needed to add the missing unit tests
) | ||
|
||
|
||
def execute(): |
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.
same
from databricks.labs.lakebridge.resources.assessments.synapse.common.profiler_classes import SynapseWorkspace | ||
|
||
|
||
def execute(): |
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.
same
from sqlalchemy import text | ||
|
||
|
||
def execute(): |
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.
same
Changes
What does this PR do?
Relevant implementation details
Caveats/things to watch out for when reviewing:
Linked issues
Resolves #..
Functionality
databricks labs lakebridge ...
Tests