You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2021/03/24 06:20:31 UTC
[GitHub] [spark] AngersZhuuuu opened a new pull request #31948: [SPARK-34848][CORE] Add duration to TaskMetricDistributions
AngersZhuuuu opened a new pull request #31948:
URL: https://github.com/apache/spark/pull/31948
### What changes were proposed in this pull request?
Task duration distribution is also very important for us to judge whether a stage's task is skew enough.
### Why are the changes needed?
Add important information in TaskMetricsDistribution
### Does this PR introduce _any_ user-facing change?
People can see duration distribution from TaskMetricsDistribution
### How was this patch tested?
Existed UT
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #31948: [SPARK-34848][CORE] Add duration to TaskMetricDistributions
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #31948:
URL: https://github.com/apache/spark/pull/31948#discussion_r663948849
##########
File path: core/src/main/scala/org/apache/spark/status/api/v1/api.scala
##########
@@ -342,6 +342,7 @@ class ShuffleWriteMetrics private[spark](
class TaskMetricDistributions private[spark](
val quantiles: IndexedSeq[Double],
+ val duration: IndexedSeq[Double],
Review comment:
Thanks for you explanation. Will notice this problem when raise pr.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31948: [SPARK-34848][CORE] Add duration to TaskMetricDistributions
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31948:
URL: https://github.com/apache/spark/pull/31948#issuecomment-805660201
**[Test build #136440 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136440/testReport)** for PR 31948 at commit [`aeb8b66`](https://github.com/apache/spark/commit/aeb8b666dce505d5772b690eb8adc7d5d3e22001).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #31948: [SPARK-34848][CORE] Add duration to TaskMetricDistributions
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #31948:
URL: https://github.com/apache/spark/pull/31948#discussion_r663648579
##########
File path: core/src/main/scala/org/apache/spark/status/api/v1/api.scala
##########
@@ -342,6 +342,7 @@ class ShuffleWriteMetrics private[spark](
class TaskMetricDistributions private[spark](
val quantiles: IndexedSeq[Double],
+ val duration: IndexedSeq[Double],
Review comment:
TBH I am not clear what happened since I am not familiar what is mima doing...How should I check about this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on a change in pull request #31948: [SPARK-34848][CORE] Add duration to TaskMetricDistributions
Posted by GitBox <gi...@apache.org>.
srowen commented on a change in pull request #31948:
URL: https://github.com/apache/spark/pull/31948#discussion_r663546965
##########
File path: core/src/main/scala/org/apache/spark/status/api/v1/api.scala
##########
@@ -342,6 +342,7 @@ class ShuffleWriteMetrics private[spark](
class TaskMetricDistributions private[spark](
val quantiles: IndexedSeq[Double],
+ val duration: IndexedSeq[Double],
Review comment:
@AngersZhuuuu is this OK to 'break' as it's it's private[spark]? we just discovered that it was kind of accidentally excluded from a previous change between 3.0 -> 3.1. If so we can just retain the exclusion, just checking.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on pull request #31948: [SPARK-34848][CORE] Add duration to TaskMetricDistributions
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on pull request #31948:
URL: https://github.com/apache/spark/pull/31948#issuecomment-806307382
gentle ping @srowen Could you take a look at this? it's very useful too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31948: [SPARK-34848][CORE] Add duration to TaskMetricDistributions
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31948:
URL: https://github.com/apache/spark/pull/31948#issuecomment-805646827
Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41024/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on a change in pull request #31948: [SPARK-34848][CORE] Add duration to TaskMetricDistributions
Posted by GitBox <gi...@apache.org>.
srowen commented on a change in pull request #31948:
URL: https://github.com/apache/spark/pull/31948#discussion_r663914637
##########
File path: core/src/main/scala/org/apache/spark/status/api/v1/api.scala
##########
@@ -342,6 +342,7 @@ class ShuffleWriteMetrics private[spark](
class TaskMetricDistributions private[spark](
val quantiles: IndexedSeq[Double],
+ val duration: IndexedSeq[Double],
Review comment:
I think this one is fine, but a brief recap: Mima tries to detect changes to public APIs, which could cause binary or source incompatibilities, and we generally never allow those in minor releases. This method signature change triggers Mima, although it's actually 'private'; because it's private to a package, it's technically public in the bytecode, and Mima doesn't know it was private[spark] in Scala. So that's fine, it should be 'excluded' in Mima.
The reason this never failed the tests originally was because this method was already excluded, because it changed from 3.0 to 3.1 too. The way Mima was set up, we were reusing exclusions from 3.0 to 3.1 when evaluating changes from 3.1 to 3.2 as well. That could be a problem, if the change from 3.1 to 3.2 was not intended. When we fixed the Mima config, this method popped up as possibly accidentally excluded, so Dongjoon was double-checking.
I think the ones he found are actually OK, just checking.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on pull request #31948: [SPARK-34848][CORE] Add duration to TaskMetricDistributions
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on pull request #31948:
URL: https://github.com/apache/spark/pull/31948#issuecomment-808647512
> It seems that [unit test`stage with summaries` in `HistoryServerSuite.scala`](https://github.com/apache/spark/blob/master/core/src/test/scala/org/apache/spark/deploy/history/HistoryServerSuite.scala#L184) starts failing after this PR. Shall we update [stage_with_summaries_expectation.json](https://github.com/apache/spark/blob/master/core/src/test/resources/HistoryServerExpectations/stage_with_summaries_expectation.json) as well? cc @srowen , thanks.
Will rasie a pr to fix this now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31948: [SPARK-34848][CORE] Add duration to TaskMetricDistributions
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31948:
URL: https://github.com/apache/spark/pull/31948#issuecomment-805536009
**[Test build #136440 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136440/testReport)** for PR 31948 at commit [`aeb8b66`](https://github.com/apache/spark/commit/aeb8b666dce505d5772b690eb8adc7d5d3e22001).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31948: [SPARK-34848][CORE] Add duration to TaskMetricDistributions
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31948:
URL: https://github.com/apache/spark/pull/31948#issuecomment-805674327
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #31948: [SPARK-34848][CORE] Add duration to TaskMetricDistributions
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31948:
URL: https://github.com/apache/spark/pull/31948#issuecomment-805536009
**[Test build #136440 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136440/testReport)** for PR 31948 at commit [`aeb8b66`](https://github.com/apache/spark/commit/aeb8b666dce505d5772b690eb8adc7d5d3e22001).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31948: [SPARK-34848][CORE] Add duration to TaskMetricDistributions
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31948:
URL: https://github.com/apache/spark/pull/31948#issuecomment-805674328
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on pull request #31948: [SPARK-34848][CORE] Add duration to TaskMetricDistributions
Posted by GitBox <gi...@apache.org>.
srowen commented on pull request #31948:
URL: https://github.com/apache/spark/pull/31948#issuecomment-806317879
Dumb question but did this already exist in any metrics (without quantiles)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31948: [SPARK-34848][CORE] Add duration to TaskMetricDistributions
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31948:
URL: https://github.com/apache/spark/pull/31948#issuecomment-805595216
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41024/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on a change in pull request #31948: [SPARK-34848][CORE] Add duration to TaskMetricDistributions
Posted by GitBox <gi...@apache.org>.
srowen commented on a change in pull request #31948:
URL: https://github.com/apache/spark/pull/31948#discussion_r663914637
##########
File path: core/src/main/scala/org/apache/spark/status/api/v1/api.scala
##########
@@ -342,6 +342,7 @@ class ShuffleWriteMetrics private[spark](
class TaskMetricDistributions private[spark](
val quantiles: IndexedSeq[Double],
+ val duration: IndexedSeq[Double],
Review comment:
I think this one is fine, but a brief recap: Mima tries to detect changes to public APIs, which could cause binary or source incompatibilities, and we generally never allow those in minor releases. This method signature change triggers Mima, although it's actually 'private'; because it's private to a package, it's technically public in the bytecode, and Mima doesn't know it was private[spark] in Scala. So that's fine, it should be 'excluded' in Mima.
The reason this never failed the tests originally was because this method was already excluded, because it changed from 3.0 to 3.1 too. The way Mima was set up, we were reusing exclusions from 3.0 to 3.1 when evaluating changes from 3.1 to 3.2 as well. That could be a problem, if the change from 3.1 to 3.2 was not intended. When we fixed the Mima config, this method popped up as possibly accidentally excluded, so Dongjoon was double-checking.
I think the ones he found are actually OK, just checking.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on pull request #31948: [SPARK-34848][CORE] Add duration to TaskMetricDistributions
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on pull request #31948:
URL: https://github.com/apache/spark/pull/31948#issuecomment-806325775
> Dumb question but did this already exist in any metrics (without quantiles)?
It exist in `TaskData`, but if we return all taskData it's so huge. we just need to know the distribution. but i distribution, there is no duration data.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen closed pull request #31948: [SPARK-34848][CORE] Add duration to TaskMetricDistributions
Posted by GitBox <gi...@apache.org>.
srowen closed pull request #31948:
URL: https://github.com/apache/spark/pull/31948
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a change in pull request #31948: [SPARK-34848][CORE] Add duration to TaskMetricDistributions
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #31948:
URL: https://github.com/apache/spark/pull/31948#discussion_r663304856
##########
File path: core/src/main/scala/org/apache/spark/status/api/v1/api.scala
##########
@@ -342,6 +342,7 @@ class ShuffleWriteMetrics private[spark](
class TaskMetricDistributions private[spark](
val quantiles: IndexedSeq[Double],
+ val duration: IndexedSeq[Double],
Review comment:
This is detected as a binary-incompatibility by MiMa.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on pull request #31948: [SPARK-34848][CORE] Add duration to TaskMetricDistributions
Posted by GitBox <gi...@apache.org>.
srowen commented on pull request #31948:
URL: https://github.com/apache/spark/pull/31948#issuecomment-808398257
Merged to master
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AngersZhuuuu commented on a change in pull request #31948: [SPARK-34848][CORE] Add duration to TaskMetricDistributions
Posted by GitBox <gi...@apache.org>.
AngersZhuuuu commented on a change in pull request #31948:
URL: https://github.com/apache/spark/pull/31948#discussion_r663948849
##########
File path: core/src/main/scala/org/apache/spark/status/api/v1/api.scala
##########
@@ -342,6 +342,7 @@ class ShuffleWriteMetrics private[spark](
class TaskMetricDistributions private[spark](
val quantiles: IndexedSeq[Double],
+ val duration: IndexedSeq[Double],
Review comment:
Thanks for you explanation. Will notice this problem when raise pr.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] c21 commented on pull request #31948: [SPARK-34848][CORE] Add duration to TaskMetricDistributions
Posted by GitBox <gi...@apache.org>.
c21 commented on pull request #31948:
URL: https://github.com/apache/spark/pull/31948#issuecomment-808647121
It seems that [unit test`stage with summaries` in `HistoryServerSuite.scala`](https://github.com/apache/spark/blob/master/core/src/test/scala/org/apache/spark/deploy/history/HistoryServerSuite.scala#L184) starts failing after this PR. Shall we update [stage_with_summaries_expectation.json](https://github.com/apache/spark/blob/master/core/src/test/resources/HistoryServerExpectations/stage_with_summaries_expectation.json) as well? cc @srowen , thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org