-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[Kernel][Metrics][PR#1] Adds initial interfaces and a Timer class #3902
Conversation
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.
Looks great! Asked some questions.
kernel/kernel-api/src/main/java/io/delta/kernel/internal/metrics/Timer.java
Outdated
Show resolved
Hide resolved
import java.util.UUID; | ||
|
||
/** Defines the common fields that are shared by reports for Delta operations */ | ||
public interface DeltaOperationReport extends MetricsReport { |
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.
Will MetricsReporter::report
ever be called with a different type of MetricsReport
that is not a DeltaOperationReport
?
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 in the future possibly. We want MetricsReport
to be completely generic so we can push anything we want as part of the interface.
kernel/kernel-api/src/main/java/io/delta/kernel/internal/metrics/Timer.java
Show resolved
Hide resolved
232933e
to
e64ae0b
Compare
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!
kernel/kernel-api/src/test/scala/io/delta/kernel/metrics/MetricsUtilsSuite.scala
Outdated
Show resolved
Hide resolved
…lass (delta-io#3902)" This reverts commit 82c2f64.
…lass (delta-io#3902)" This reverts commit 82c2f64.
Which Delta project/connector is this regarding?
Description
Adds the initial interfaces for #3905 as well as a
Timer
class that will be used in follow-up PRs.How was this patch tested?
Just interface changes.
Does this PR introduce any user-facing changes?
No.