-
Notifications
You must be signed in to change notification settings - Fork 83
Standardize Profiler Path Extract #2237
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2237 +/- ##
=======================================
Coverage 63.95% 63.95%
=======================================
Files 99 99
Lines 8644 8644
Branches 890 890
=======================================
Hits 5528 5528
Misses 2944 2944
Partials 172 172 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
✅ 130/130 passed, 8 flaky, 5 skipped, 9m32s total Flaky tests:
Running from acceptance #3486 |
| version: "1.0" | ||
| # TODO: This needs to be removed. | ||
| extract_folder: "/tmp/data/synapse_assessment" | ||
| extract_folder: "~/.databricks/labs/lakebridge_profilers/synapse_assessment" |
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.
FYI we'll need to add the run Id as a path prefix in a follow-up PR.
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 at the folder level? can we not do it at table level inside the database?
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 don't we make the profiler output configurable by the user?
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.
having it central will help us share state more easily across component, That was the principle behind a common central location.
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 can see the value of that. Maybe we need to think about how to manage the state for all modules and then standardize the optionality of outputs for all.
goodwillpunning
left a comment
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.
LGTM!
eri-adepoju
left a comment
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.
LGTM
Changes
What does this PR do?
Standardize Profiler Path Extract
Relevant implementation details
Caveats/things to watch out for when reviewing:
Linked issues
Resolves #..
Functionality
databricks labs lakebridge ...Tests