-
Notifications
You must be signed in to change notification settings - Fork 175
feat(amber): carry cache-reuse status as a metrics flag #6729
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?
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -30,7 +30,9 @@ case class OperatorAggregatedMetrics( | |||||
| numWorkers: Long, | ||||||
| aggregatedDataProcessingTime: Long, | ||||||
| aggregatedControlProcessingTime: Long, | ||||||
| aggregatedIdleTime: Long | ||||||
| aggregatedIdleTime: Long, | ||||||
| // Provenance: the operator completed by reusing cached results (no workers ran). | ||||||
| reusedFromCache: Boolean = false | ||||||
|
Contributor
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. This is the only one of the twelve fields with a default, and neither construction site takes it —
Suggested change
|
||||||
| ) | ||||||
|
|
||||||
| case class OperatorStatisticsUpdateEvent(operatorStatistics: Map[String, OperatorAggregatedMetrics]) | ||||||
|
|
||||||
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 comment promises more than the input set delivers. The only caller (
WorkflowExecution.scala:94-110) flattensgetAllRegionExecutionsand groups bylogicalOpId, sometricsholds the physical operators that currently have a region execution. For a HashJoin, the probe operator contributes nothing until its region is created, soforallcan return true with a later operator still to come.SyncExecutionResource.scala:189-200documents that same window for the sibling state field, so this is the function's existing shape rather than something you introduced.Can a logical operator ever be partially reused under #5884? If reuse is all-or-nothing, only the comment needs narrowing to "the physical operators reporting so far". If it can be partial, the flag wants the same late-region guard the state field got.