[GitHub] spark pull request #19132: [SPARK-21922] Fix duration always updating when t...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/spark/pull/19132


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19132: [SPARK-21922] Fix duration always updating when t...

[caneGuy]

Github user caneGuy commented on a diff in the pull request:

https://github.com/apache/spark/pull/19132#discussion_r138811435
  
--- Diff: 
core/src/main/scala/org/apache/spark/status/api/v1/AllStagesResource.scala ---
@@ -69,7 +70,8 @@ private[v1] object AllStagesResource {
   }
 
 val taskData = if (includeDetails) {
-  Some(stageUiData.taskData.map { case (k, v) => k -> 
convertTaskData(v) } )
+  Some(stageUiData.taskData.map { case (k, v) =>
+k -> convertTaskData(v, stageUiData.lastUpdateTime) } )
--- End diff --

Thanks for your patient.Actually i did not check carefully.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19132: [SPARK-21922] Fix duration always updating when t...

[jerryshao]

Github user jerryshao commented on a diff in the pull request:

https://github.com/apache/spark/pull/19132#discussion_r138805063
  
--- Diff: 
core/src/main/scala/org/apache/spark/status/api/v1/AllStagesResource.scala ---
@@ -69,7 +70,8 @@ private[v1] object AllStagesResource {
   }
 
 val taskData = if (includeDetails) {
-  Some(stageUiData.taskData.map { case (k, v) => k -> 
convertTaskData(v) } )
+  Some(stageUiData.taskData.map { case (k, v) =>
+k -> convertTaskData(v, stageUiData.lastUpdateTime) } )
--- End diff --

Sorry I may stated clearly, it should be:

```
Some(stageUiData.taskData.map { case (k, v) =>
  k -> convertTaskData(v, stageUiData.lastUpdateTime) })
```
No whitespace after "}".



---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19132: [SPARK-21922] Fix duration always updating when t...

[jerryshao]

Github user jerryshao commented on a diff in the pull request:

https://github.com/apache/spark/pull/19132#discussion_r138789492
  
--- Diff: 
core/src/main/scala/org/apache/spark/status/api/v1/OneStageResource.scala ---
@@ -81,7 +83,8 @@ private[v1] class OneStageResource(ui: SparkUI) {
   @DefaultValue("20") @QueryParam("length") length: Int,
   @DefaultValue("ID") @QueryParam("sortBy") sortBy: TaskSorting): 
Seq[TaskData] = {
 withStageAttempt(stageId, stageAttemptId) { stage =>
-  val tasks = 
stage.ui.taskData.values.map{AllStagesResource.convertTaskData}.toIndexedSeq
+  val tasks = stage.ui.taskData.values.map{
--- End diff --

The style should be changed to `map {  AllStagesResource.convertTaskData(_, 
ui.lastUpdateTime) }`, requires whitespace between `{` and `}`. You could check 
other similar codes about the style.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19132: [SPARK-21922] Fix duration always updating when t...

[jerryshao]

Github user jerryshao commented on a diff in the pull request:

https://github.com/apache/spark/pull/19132#discussion_r138789213
  
--- Diff: core/src/main/scala/org/apache/spark/ui/jobs/UIData.scala ---
@@ -97,6 +97,7 @@ private[spark] object UIData {
 var memoryBytesSpilled: Long = _
 var diskBytesSpilled: Long = _
 var isBlacklisted: Int = _
+var jobLastUpdateTime: Option[Long] = None
--- End diff --

Is it better to rename to `stageLastUpdateTime` or just `lastUpdateTime`? 
Since this structure unrelated to job, would be better to not involve "job".


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19132: [SPARK-21922] Fix duration always updating when t...

[jerryshao]

Github user jerryshao commented on a diff in the pull request:

https://github.com/apache/spark/pull/19132#discussion_r138533547
  
--- Diff: 
core/src/main/scala/org/apache/spark/status/api/v1/AllStagesResource.scala ---
@@ -47,7 +47,8 @@ private[v1] class AllStagesResource(ui: SparkUI) {
 listener.stageIdToData.get((stageInfo.stageId, 
stageInfo.attemptId))
   }
 } yield {
-  AllStagesResource.stageUiToStageData(status, stageInfo, stageUiData, 
includeDetails = false)
+  AllStagesResource.stageUiToStageData(
+status, stageInfo, stageUiData, includeDetails = false, Some(ui))
--- End diff --

It's not a good idea to pass in `SparkUI` to only get `lastUpdate`, the API 
looks weird to add this `SparkUI` argument, the fix here only just make it 
work. It is better to add one more field in `StageUIData` or `TaskUIData` if 
possible.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19132: [SPARK-21922] Fix duration always updating when t...

[jerryshao]

Github user jerryshao commented on a diff in the pull request:

https://github.com/apache/spark/pull/19132#discussion_r138526050
  
--- Diff: 
core/src/main/scala/org/apache/spark/status/api/v1/AllStagesResource.scala ---
@@ -142,7 +142,7 @@ private[v1] object AllStagesResource {
   index = uiData.taskInfo.index,
   attempt = uiData.taskInfo.attemptNumber,
   launchTime = new Date(uiData.taskInfo.launchTime),
-  duration = uiData.taskDuration,
+  duration = uiData.taskDuration(),
--- End diff --

Yes, if it is not a big change I think it should be fixed here. Because 
currently with this fix UI and REST API are inconsistent.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19132: [SPARK-21922] Fix duration always updating when t...

[caneGuy]

Github user caneGuy commented on a diff in the pull request:

https://github.com/apache/spark/pull/19132#discussion_r138523362
  
--- Diff: 
core/src/main/scala/org/apache/spark/status/api/v1/AllStagesResource.scala ---
@@ -142,7 +142,7 @@ private[v1] object AllStagesResource {
   index = uiData.taskInfo.index,
   attempt = uiData.taskInfo.attemptNumber,
   launchTime = new Date(uiData.taskInfo.launchTime),
-  duration = uiData.taskDuration,
+  duration = uiData.taskDuration(),
--- End diff --

You are right, @jerryshao .IIUC, the `ui` in `AllStagesResource.scala` is 
passed from `ApiRootResource` which also create `sparkUI` by 
`FSHistoryProvider`.So we can also get `lastUpdateTime` from this `ui` in 
`AllStagesResource` and pass to the `taskDuration` interface.I think it is 
another problem for REST?Should we fix here?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19132: [SPARK-21922] Fix duration always updating when t...

[jerryshao]

Github user jerryshao commented on a diff in the pull request:

https://github.com/apache/spark/pull/19132#discussion_r138510683
  
--- Diff: 
core/src/main/scala/org/apache/spark/status/api/v1/AllStagesResource.scala ---
@@ -142,7 +142,7 @@ private[v1] object AllStagesResource {
   index = uiData.taskInfo.index,
   attempt = uiData.taskInfo.attemptNumber,
   launchTime = new Date(uiData.taskInfo.launchTime),
-  duration = uiData.taskDuration,
+  duration = uiData.taskDuration(),
--- End diff --

Here what if we call the REST API on history server to get stage info? 
Looks like we may still have this issue since we don't have last update time 
here, what do you think @ajbozarth ?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19132: [SPARK-21922] Fix duration always updating when t...

[caneGuy]

Github user caneGuy commented on a diff in the pull request:

https://github.com/apache/spark/pull/19132#discussion_r138243757
  
--- Diff: core/src/main/scala/org/apache/spark/ui/SparkUI.scala ---
@@ -50,6 +50,7 @@ private[spark] class SparkUI private (
 val operationGraphListener: RDDOperationGraphListener,
 var appName: String,
 val basePath: String,
+val lastUpdateTime: Long = -1L,
--- End diff --

Update @jerryshao Thanks for your time.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19132: [SPARK-21922] Fix duration always updating when t...

[jerryshao]

Github user jerryshao commented on a diff in the pull request:

https://github.com/apache/spark/pull/19132#discussion_r138240053
  
--- Diff: core/src/main/scala/org/apache/spark/ui/SparkUI.scala ---
@@ -50,6 +50,7 @@ private[spark] class SparkUI private (
 val operationGraphListener: RDDOperationGraphListener,
 var appName: String,
 val basePath: String,
+val lastUpdateTime: Long = -1L,
--- End diff --

I would like to user `Option[Long] = None` as default value to reflect 
there's no update time. 


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19132: [SPARK-21922] Fix duration always updating when t...

[caneGuy]

Github user caneGuy commented on a diff in the pull request:

https://github.com/apache/spark/pull/19132#discussion_r137255922
  
--- Diff: 
core/src/main/scala/org/apache/spark/status/api/v1/AllStagesResource.scala ---
@@ -142,7 +142,7 @@ private[v1] object AllStagesResource {
   index = uiData.taskInfo.index,
   attempt = uiData.taskInfo.attemptNumber,
   launchTime = new Date(uiData.taskInfo.launchTime),
-  duration = uiData.taskDuration,
+  duration = uiData.taskDuration(),
--- End diff --

Thanks @ajbozarth 


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19132: [SPARK-21922] Fix duration always updating when t...

[ajbozarth]

Github user ajbozarth commented on a diff in the pull request:

https://github.com/apache/spark/pull/19132#discussion_r137238007
  
--- Diff: 
core/src/main/scala/org/apache/spark/status/api/v1/AllStagesResource.scala ---
@@ -142,7 +142,7 @@ private[v1] object AllStagesResource {
   index = uiData.taskInfo.index,
   attempt = uiData.taskInfo.attemptNumber,
   launchTime = new Date(uiData.taskInfo.launchTime),
-  duration = uiData.taskDuration,
+  duration = uiData.taskDuration(),
--- End diff --

odd, I thought for sure it'd be fine, then this LGTM


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19132: [SPARK-21922] Fix duration always updating when t...

[caneGuy]

Github user caneGuy commented on a diff in the pull request:

https://github.com/apache/spark/pull/19132#discussion_r137214624
  
--- Diff: 
core/src/main/scala/org/apache/spark/status/api/v1/AllStagesResource.scala ---
@@ -142,7 +142,7 @@ private[v1] object AllStagesResource {
   index = uiData.taskInfo.index,
   attempt = uiData.taskInfo.attemptNumber,
   launchTime = new Date(uiData.taskInfo.launchTime),
-  duration = uiData.taskDuration,
+  duration = uiData.taskDuration(),
--- End diff --

Since i changed `taskDuration` below,if do not add `()` a compiler error 
will occur.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19132: [SPARK-21922] Fix duration always updating when t...

[ajbozarth]

Github user ajbozarth commented on a diff in the pull request:

https://github.com/apache/spark/pull/19132#discussion_r137195530
  
--- Diff: 
core/src/main/scala/org/apache/spark/status/api/v1/AllStagesResource.scala ---
@@ -142,7 +142,7 @@ private[v1] object AllStagesResource {
   index = uiData.taskInfo.index,
   attempt = uiData.taskInfo.attemptNumber,
   launchTime = new Date(uiData.taskInfo.launchTime),
-  duration = uiData.taskDuration,
+  duration = uiData.taskDuration(),
--- End diff --

this is unrelated and unnecessary


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #19132: [SPARK-21922] Fix duration always updating when t...

[caneGuy]

GitHub user caneGuy opened a pull request:

https://github.com/apache/spark/pull/19132

[SPARK-21922] Fix duration always updating when task failed but status is 
still RUN…

…NING

## What changes were proposed in this pull request?
When executor failed and task metrics have not send to driver,the status 
will always be 'RUNNING' and the duration will be 'CurrentTime - launchTime'.
We can fix this time by modify time of event log.
And the result picture is uploaded in 
[SPARK-21922](https://issues.apache.org/jira/browse/SPARK-21922)

## How was this patch tested?
Deploy historyserver and open a same job.



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/caneGuy/spark zhoukang/fix-duration

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/19132.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #19132


commit 03a33a96c3cae0e4272a7bae2230f3a8c2c4589a
Author: zhoukang 
Date:   2017-09-05T10:28:06Z

Fix duration always updating when task failed but status is still RUNNING




---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org