-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-36030][SQL] Support DS v2 metrics at writing path #33239
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
|
cc @cloud-fan |
This comment has been minimized.
This comment has been minimized.
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
|
cc @maropu too |
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
|
Test build #140740 has finished for PR 33239 at commit
|
|
Test build #140743 has finished for PR 33239 at commit
|
|
@cloud-fan Do we consider this in 3.2 to make the API complete in this release? |
|
gentle ping @cloud-fan |
sql/core/src/test/scala/org/apache/spark/sql/execution/ui/SQLAppStatusListenerSuite.scala
Outdated
Show resolved
Hide resolved
sql/core/src/test/scala/org/apache/spark/sql/execution/ui/SQLAppStatusListenerSuite.scala
Outdated
Show resolved
Hide resolved
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 PR looks reasonable to me, @viirya . I left a few comments about test cases.
It would be great if we can get your advice, @cloud-fan , @gengliangwang , @HeartSaVioR , @Ngone51 , @maropu , @HyukjinKwon , @sunchao , @huaxingao .
|
Thank you @dongjoon-hyun! I will address the comments. |
| taskAttemptContext: TaskAttemptContext, | ||
| committer: FileCommitProtocol) extends DataWriter[InternalRow] { | ||
| committer: FileCommitProtocol, | ||
| customMetrics: Map[String, SQLMetric]) extends DataWriter[InternalRow] { |
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.
Shall we add tests for the changes in this File?
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.
Let me try to add one.
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 added custom metric for writing to InMemory table for test purpose. The tests are in FileFormatDataWriterMetricSuite.
|
@viirya Thanks for the work! Overall this LGTM |
|
Test build #141294 has finished for PR 33239 at commit
|
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
|
Test build #141298 has finished for PR 33239 at commit
|
gengliangwang
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
|
Test build #141313 has finished for PR 33239 at commit
|
|
retest this please |
|
Thanks @dongjoon-hyun @gengliangwang @sunchao. I will merge this after tests pass. cc @cloud-fan |
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
|
Test build #141350 has finished for PR 33239 at commit
|
|
retest this please |
|
Test build #141359 has finished for PR 33239 at commit
|
|
retest this please |
|
Kubernetes integration test starting |
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
|
Kubernetes integration test status success |
|
Kubernetes integration test starting |
|
Kubernetes integration test status failure |
|
Test build #141362 has finished for PR 33239 at commit
|
|
Thanks. Merging to master/3.2. |
### What changes were proposed in this pull request? We add the interface for DS v2 metrics in SPARK-34366. It is only added for reading path, though. This patch extends the metrics interface to writing path. ### Why are the changes needed? Complete DS v2 metrics interface support in writing path. ### Does this PR introduce _any_ user-facing change? No. For developer, yes, as this adds metrics support at DS v2 writing path. ### How was this patch tested? Added test. Closes #33239 from viirya/v2-write-metrics. Authored-by: Liang-Chi Hsieh <viirya@gmail.com> Signed-off-by: Liang-Chi Hsieh <viirya@gmail.com> (cherry picked from commit 2653201) Signed-off-by: Liang-Chi Hsieh <viirya@gmail.com>
|
late LGTM |

What changes were proposed in this pull request?
We add the interface for DS v2 metrics in SPARK-34366. It is only added for reading path, though. This patch extends the metrics interface to writing path.
Why are the changes needed?
Complete DS v2 metrics interface support in writing path.
Does this PR introduce any user-facing change?
No. For developer, yes, as this adds metrics support at DS v2 writing path.
How was this patch tested?
Added test.