You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GayathriMurali <gi...@git.apache.org> on 2016/03/06 08:05:12 UTC

[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

GitHub user GayathriMurali opened a pull request:

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

    [SPARK-13396] Stop using our internal deprecated .metrics on Exceptio…

    JIRA: https://issues.apache.org/jira/browse/SPARK-13396
    
    Stop using our internal deprecated .metrics on ExceptionFailure instead use accumUpdates

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

    $ git pull https://github.com/GayathriMurali/spark SPARK-13396

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

    https://github.com/apache/spark/pull/11544.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 #11544
    
----
commit 11c1303d09423105e1808286a65cd5c5b9a42ab3
Author: GayathriMurali <ga...@gmail.com>
Date:   2016-03-06T07:02:18Z

    [SPARK-13396] Stop using our internal deprecated .metrics on ExceptionFailure instead use accumUpdates

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/11544#issuecomment-197068266
  
    **[Test build #53242 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/53242/consoleFull)** for PR 11544 at commit [`8314a05`](https://github.com/apache/spark/commit/8314a055683fd3bf5d8d827cf11552f0f45d9de5).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/11544#issuecomment-196704221
  
    **[Test build #53163 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/53163/consoleFull)** for PR 11544 at commit [`5f452cc`](https://github.com/apache/spark/commit/5f452ccacb11684e65a88ba4c48dd5490e5bc289).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/11544#issuecomment-196958157
  
    **[Test build #53204 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/53204/consoleFull)** for PR 11544 at commit [`9beb858`](https://github.com/apache/spark/commit/9beb8583a16a929683ea69653411a704acef3fc5).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by GayathriMurali <gi...@git.apache.org>.
Github user GayathriMurali commented on the pull request:

    https://github.com/apache/spark/pull/11544#issuecomment-195210293
  
    @jodersky Sure, thanks


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/11544#issuecomment-196531461
  
    **[Test build #53117 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/53117/consoleFull)** for PR 11544 at commit [`c675798`](https://github.com/apache/spark/commit/c67579865352a11ab8ae2be2ee9afc4dfb7399cf).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/11544#issuecomment-196958623
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/53204/
    Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by GayathriMurali <gi...@git.apache.org>.
Github user GayathriMurali commented on the pull request:

    https://github.com/apache/spark/pull/11544#issuecomment-196896476
  
    @andrewor14 I incorporated your suggestion to use Some instead of Option. Can you please let me know if there is anything I can to do to get this PR merged. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/11544#issuecomment-196671690
  
    **[Test build #53163 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/53163/consoleFull)** for PR 11544 at commit [`5f452cc`](https://github.com/apache/spark/commit/5f452ccacb11684e65a88ba4c48dd5490e5bc289).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by GayathriMurali <gi...@git.apache.org>.
Github user GayathriMurali commented on a diff in the pull request:

    https://github.com/apache/spark/pull/11544#discussion_r55908239
  
    --- Diff: core/src/main/scala/org/apache/spark/ui/jobs/JobProgressListener.scala ---
    @@ -374,28 +374,30 @@ class JobProgressListener(conf: SparkConf) extends SparkListener with Logging {
           execSummary.taskTime += info.duration
           stageData.numActiveTasks -= 1
     
    -      val (errorMessage, metrics): (Option[String], Option[TaskMetrics]) =
    +      val (errorMessage, metrics): (Option[String], Seq[AccumulableInfo]) =
             taskEnd.reason match {
               case org.apache.spark.Success =>
                 stageData.completedIndices.add(info.index)
                 stageData.numCompleteTasks += 1
    -            (None, Option(taskEnd.taskMetrics))
    -          case e: ExceptionFailure =>  // Handle ExceptionFailure because we might have metrics
    +            (None, taskEnd.taskMetrics.accumulatorUpdates())
    +          case e: ExceptionFailure => // Handle ExceptionFailure because we might have metrics
                 stageData.numFailedTasks += 1
    -            (Some(e.toErrorString), e.metrics)
    -          case e: TaskFailedReason =>  // All other failure cases
    +            (Some(e.toErrorString), e.accumUpdates)
    +          case e: TaskFailedReason => // All other failure cases
                 stageData.numFailedTasks += 1
    -            (Some(e.toErrorString), None)
    +            (Some(e.toErrorString), Seq.empty[AccumulableInfo])
             }
     
    -      metrics.foreach { m =>
    +      val taskMetrics: Option[TaskMetrics] = if (metrics.nonEmpty) Option(TaskMetrics.
    +        fromAccumulatorUpdates(metrics)) else None
    --- End diff --
    
    Can you please explain the idea behind using Some instead of Option? When `fromAccumulatorUpdates` is enclosed within a if that checks if metrics is nonempty, we are in the safe zone. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/11544#issuecomment-196704355
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/53163/
    Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:

    https://github.com/apache/spark/pull/11544#discussion_r56304266
  
    --- Diff: core/src/main/scala/org/apache/spark/ui/jobs/JobProgressListener.scala ---
    @@ -374,28 +374,34 @@ class JobProgressListener(conf: SparkConf) extends SparkListener with Logging {
           execSummary.taskTime += info.duration
           stageData.numActiveTasks -= 1
     
    -      val (errorMessage, metrics): (Option[String], Option[TaskMetrics]) =
    +      val (errorMessage, accums): (Option[String], Seq[AccumulableInfo]) =
             taskEnd.reason match {
               case org.apache.spark.Success =>
                 stageData.completedIndices.add(info.index)
                 stageData.numCompleteTasks += 1
    -            (None, Option(taskEnd.taskMetrics))
    -          case e: ExceptionFailure =>  // Handle ExceptionFailure because we might have metrics
    +            (None, taskEnd.taskMetrics.accumulatorUpdates())
    +          case e: ExceptionFailure => // Handle ExceptionFailure because we might have accumUpdates
                 stageData.numFailedTasks += 1
    -            (Some(e.toErrorString), e.metrics)
    -          case e: TaskFailedReason =>  // All other failure cases
    +            (Some(e.toErrorString), e.accumUpdates)
    +          case e: TaskFailedReason => // All other failure cases
                 stageData.numFailedTasks += 1
    -            (Some(e.toErrorString), None)
    +            (Some(e.toErrorString), Seq.empty[AccumulableInfo])
             }
     
    -      metrics.foreach { m =>
    +      val taskMetrics =
    +        if (accums.nonEmpty) {
    +        Some(TaskMetrics.fromAccumulatorUpdates(accums))
    --- End diff --
    
    The indentation is wrong on this and subsequent lines, but maybe I can just carefully fix this on merge


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by jodersky <gi...@git.apache.org>.
Github user jodersky commented on a diff in the pull request:

    https://github.com/apache/spark/pull/11544#discussion_r55438396
  
    --- Diff: core/src/main/scala/org/apache/spark/ui/jobs/JobProgressListener.scala ---
    @@ -374,28 +374,29 @@ class JobProgressListener(conf: SparkConf) extends SparkListener with Logging {
           execSummary.taskTime += info.duration
           stageData.numActiveTasks -= 1
     
    -      val (errorMessage, metrics): (Option[String], Option[TaskMetrics]) =
    +      val (errorMessage, metrics): (Option[String], Seq[AccumulableInfo]) =
             taskEnd.reason match {
               case org.apache.spark.Success =>
                 stageData.completedIndices.add(info.index)
                 stageData.numCompleteTasks += 1
    -            (None, Option(taskEnd.taskMetrics))
    -          case e: ExceptionFailure =>  // Handle ExceptionFailure because we might have metrics
    +            (None, taskEnd.taskMetrics.accumulatorUpdates())
    +          case e: ExceptionFailure => // Handle ExceptionFailure because we might have metrics
                 stageData.numFailedTasks += 1
    -            (Some(e.toErrorString), e.metrics)
    -          case e: TaskFailedReason =>  // All other failure cases
    +            (Some(e.toErrorString), e.accumUpdates)
    +          case e: TaskFailedReason => // All other failure cases
                 stageData.numFailedTasks += 1
    -            (Some(e.toErrorString), None)
    +            (Some(e.toErrorString), Seq.empty[AccumulableInfo])
             }
     
    -      metrics.foreach { m =>
    +      if (metrics.nonEmpty) {
             val oldMetrics = stageData.taskData.get(info.taskId).flatMap(_.taskMetrics)
    -        updateAggregateMetrics(stageData, info.executorId, m, oldMetrics)
    +        updateAggregateMetrics(stageData, info.executorId,
    +          TaskMetrics.fromAccumulatorUpdates(metrics), oldMetrics)
           }
    -
           val taskData = stageData.taskData.getOrElseUpdate(info.taskId, new TaskUIData(info))
           taskData.taskInfo = info
    -      taskData.taskMetrics = metrics
    +      taskData.taskMetrics = if (metrics.nonEmpty) Option(TaskMetrics.
    +        fromAccumulatorUpdates(metrics)) else None
    --- End diff --
    
    I was thinking of something like this:
    ```scala
    val (errorMessage, metrics): (Option[String], Seq[AccumulableInfo]) = //as before
    
    val taskMetrics: Option[TaskMetrics] = if (metrics.nonEmpty) TaskMetrics.fromAccumulatorUpdates(metrics) else None
    
    taskMetrics.foreach{m =>
      val oldMetrics = //as before
      updateAggregateMetrics(stageData, info.executorId, m, oldMetrics)
    }
    
    taskData.taskMetrics = taskMetrics
    ```
    That way you avoid calling TaskMetrics.fromAccumulatorUpdates twice. Maybe its a nitpick, depending on the complexity of creating task metrics from accumulators. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:

    https://github.com/apache/spark/pull/11544#issuecomment-195505678
  
    Looks great. Just some minor comments.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by jodersky <gi...@git.apache.org>.
Github user jodersky commented on the pull request:

    https://github.com/apache/spark/pull/11544#issuecomment-195012502
  
    Looks good to me now. Jenkins, test this please.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by jodersky <gi...@git.apache.org>.
Github user jodersky commented on the pull request:

    https://github.com/apache/spark/pull/11544#issuecomment-195207520
  
    Jenkins has been behaving weirdly lately (at least for me). Lets try again


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:

    https://github.com/apache/spark/pull/11544#discussion_r55147006
  
    --- Diff: core/src/main/scala/org/apache/spark/ui/jobs/JobProgressListener.scala ---
    @@ -374,28 +374,30 @@ class JobProgressListener(conf: SparkConf) extends SparkListener with Logging {
           execSummary.taskTime += info.duration
           stageData.numActiveTasks -= 1
     
    -      val (errorMessage, metrics): (Option[String], Option[TaskMetrics]) =
    +      val (errorMessage, metrics): (Option[String], Seq[AccumulableInfo]) =
             taskEnd.reason match {
               case org.apache.spark.Success =>
                 stageData.completedIndices.add(info.index)
                 stageData.numCompleteTasks += 1
    -            (None, Option(taskEnd.taskMetrics))
    -          case e: ExceptionFailure =>  // Handle ExceptionFailure because we might have metrics
    +            (None, taskEnd.taskMetrics.accumulatorUpdates())
    +          case e: ExceptionFailure => // Handle ExceptionFailure because we might have metrics
                 stageData.numFailedTasks += 1
    -            (Some(e.toErrorString), e.metrics)
    -          case e: TaskFailedReason =>  // All other failure cases
    +            (Some(e.toErrorString), e.accumUpdates)
    +          case e: TaskFailedReason => // All other failure cases
                 stageData.numFailedTasks += 1
    -            (Some(e.toErrorString), None)
    +            (Some(e.toErrorString), Seq.empty[AccumulableInfo])
             }
     
    -      metrics.foreach { m =>
    +      if (!metrics.isEmpty) {
    --- End diff --
    
    `nonEmpty` instead of `!...isEmpty`?
    It looks like you create metrics from updates twice in this code.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/11544#issuecomment-195563762
  
    **[Test build #52934 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/52934/consoleFull)** for PR 11544 at commit [`228e078`](https://github.com/apache/spark/commit/228e078e1da2ce0942e6c4895e64e099cbb343e8).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by jodersky <gi...@git.apache.org>.
Github user jodersky commented on the pull request:

    https://github.com/apache/spark/pull/11544#issuecomment-195207537
  
    ok to test


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/11544#issuecomment-196537330
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/53117/
    Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by GayathriMurali <gi...@git.apache.org>.
Github user GayathriMurali commented on a diff in the pull request:

    https://github.com/apache/spark/pull/11544#discussion_r56221050
  
    --- Diff: core/src/main/scala/org/apache/spark/ui/jobs/JobProgressListener.scala ---
    @@ -374,28 +374,30 @@ class JobProgressListener(conf: SparkConf) extends SparkListener with Logging {
           execSummary.taskTime += info.duration
           stageData.numActiveTasks -= 1
     
    -      val (errorMessage, metrics): (Option[String], Option[TaskMetrics]) =
    +      val (errorMessage, metrics): (Option[String], Seq[AccumulableInfo]) =
    --- End diff --
    
    @andrewor14 I am not sure If I understand your comment. Can you please elaborate? 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by GayathriMurali <gi...@git.apache.org>.
Github user GayathriMurali commented on the pull request:

    https://github.com/apache/spark/pull/11544#issuecomment-195206169
  
    @jodersky Test has not started yet on this 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by jodersky <gi...@git.apache.org>.
Github user jodersky commented on a diff in the pull request:

    https://github.com/apache/spark/pull/11544#discussion_r55908688
  
    --- Diff: core/src/main/scala/org/apache/spark/ui/jobs/JobProgressListener.scala ---
    @@ -374,28 +374,30 @@ class JobProgressListener(conf: SparkConf) extends SparkListener with Logging {
           execSummary.taskTime += info.duration
           stageData.numActiveTasks -= 1
     
    -      val (errorMessage, metrics): (Option[String], Option[TaskMetrics]) =
    +      val (errorMessage, metrics): (Option[String], Seq[AccumulableInfo]) =
             taskEnd.reason match {
               case org.apache.spark.Success =>
                 stageData.completedIndices.add(info.index)
                 stageData.numCompleteTasks += 1
    -            (None, Option(taskEnd.taskMetrics))
    -          case e: ExceptionFailure =>  // Handle ExceptionFailure because we might have metrics
    +            (None, taskEnd.taskMetrics.accumulatorUpdates())
    +          case e: ExceptionFailure => // Handle ExceptionFailure because we might have metrics
                 stageData.numFailedTasks += 1
    -            (Some(e.toErrorString), e.metrics)
    -          case e: TaskFailedReason =>  // All other failure cases
    +            (Some(e.toErrorString), e.accumUpdates)
    +          case e: TaskFailedReason => // All other failure cases
                 stageData.numFailedTasks += 1
    -            (Some(e.toErrorString), None)
    +            (Some(e.toErrorString), Seq.empty[AccumulableInfo])
             }
     
    -      metrics.foreach { m =>
    +      val taskMetrics: Option[TaskMetrics] = if (metrics.nonEmpty) Option(TaskMetrics.
    +        fromAccumulatorUpdates(metrics)) else None
    --- End diff --
    
    `Option(x)` is equivalent to `if (x == null) None else Some(x)`, which is not needed in this case


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/11544#issuecomment-197101019
  
    **[Test build #53242 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/53242/consoleFull)** for PR 11544 at commit [`8314a05`](https://github.com/apache/spark/commit/8314a055683fd3bf5d8d827cf11552f0f45d9de5).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds the following public classes _(experimental)_:
      * `      checkArgument(!isExample || mainClass != null, \"Missing example class name.\");`
      * `class SparkOptimizer(experimentalMethods: ExperimentalMethods) extends Optimizer `
      * `class SparkPlanner(`
      * `case class CollapseCodegenStages(conf: SQLConf) extends Rule[SparkPlan] `
      * `case class EnsureRequirements(conf: SQLConf) extends Rule[SparkPlan] `
      * `case class ReuseExchange(conf: SQLConf) extends Rule[SparkPlan] `
      * `case class PlanSubqueries(sessionState: SessionState) extends Rule[SparkPlan] `


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by GayathriMurali <gi...@git.apache.org>.
Github user GayathriMurali commented on the pull request:

    https://github.com/apache/spark/pull/11544#issuecomment-195001394
  
    @jodersky Please let me know if there is anything else I need to do, to help merge this patch. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/11544#issuecomment-196537328
  
    Merged build finished. Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:

    https://github.com/apache/spark/pull/11544#issuecomment-197229081
  
    Merged to master


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/11544#issuecomment-196704353
  
    Merged build finished. Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:

    https://github.com/apache/spark/pull/11544#discussion_r56232735
  
    --- Diff: core/src/main/scala/org/apache/spark/ui/jobs/JobProgressListener.scala ---
    @@ -374,28 +374,30 @@ class JobProgressListener(conf: SparkConf) extends SparkListener with Logging {
           execSummary.taskTime += info.duration
           stageData.numActiveTasks -= 1
     
    -      val (errorMessage, metrics): (Option[String], Option[TaskMetrics]) =
    +      val (errorMessage, metrics): (Option[String], Seq[AccumulableInfo]) =
    --- End diff --
    
    Write `val (errorMessage, accums): ...`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/11544#issuecomment-195505249
  
    **[Test build #52934 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/52934/consoleFull)** for PR 11544 at commit [`228e078`](https://github.com/apache/spark/commit/228e078e1da2ce0942e6c4895e64e099cbb343e8).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/11544#issuecomment-196958618
  
    Merged build finished. Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/11544#issuecomment-195564067
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/52934/
    Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:

    https://github.com/apache/spark/pull/11544#discussion_r56046157
  
    --- Diff: core/src/main/scala/org/apache/spark/ui/jobs/JobProgressListener.scala ---
    @@ -374,28 +374,30 @@ class JobProgressListener(conf: SparkConf) extends SparkListener with Logging {
           execSummary.taskTime += info.duration
           stageData.numActiveTasks -= 1
     
    -      val (errorMessage, metrics): (Option[String], Option[TaskMetrics]) =
    +      val (errorMessage, metrics): (Option[String], Seq[AccumulableInfo]) =
             taskEnd.reason match {
               case org.apache.spark.Success =>
                 stageData.completedIndices.add(info.index)
                 stageData.numCompleteTasks += 1
    -            (None, Option(taskEnd.taskMetrics))
    -          case e: ExceptionFailure =>  // Handle ExceptionFailure because we might have metrics
    +            (None, taskEnd.taskMetrics.accumulatorUpdates())
    +          case e: ExceptionFailure => // Handle ExceptionFailure because we might have metrics
                 stageData.numFailedTasks += 1
    -            (Some(e.toErrorString), e.metrics)
    -          case e: TaskFailedReason =>  // All other failure cases
    +            (Some(e.toErrorString), e.accumUpdates)
    +          case e: TaskFailedReason => // All other failure cases
                 stageData.numFailedTasks += 1
    -            (Some(e.toErrorString), None)
    +            (Some(e.toErrorString), Seq.empty[AccumulableInfo])
             }
     
    -      metrics.foreach { m =>
    +      val taskMetrics: Option[TaskMetrics] = if (metrics.nonEmpty) Option(TaskMetrics.
    +        fromAccumulatorUpdates(metrics)) else None
    --- End diff --
    
    TLDR use `Some(x)` when you know for sure `x` is not null. Otherwise, if `x` *could* be null, use `Option(x)`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by jodersky <gi...@git.apache.org>.
Github user jodersky commented on a diff in the pull request:

    https://github.com/apache/spark/pull/11544#discussion_r55278924
  
    --- Diff: core/src/main/scala/org/apache/spark/ui/jobs/JobProgressListener.scala ---
    @@ -374,28 +374,29 @@ class JobProgressListener(conf: SparkConf) extends SparkListener with Logging {
           execSummary.taskTime += info.duration
           stageData.numActiveTasks -= 1
     
    -      val (errorMessage, metrics): (Option[String], Option[TaskMetrics]) =
    +      val (errorMessage, metrics): (Option[String], Seq[AccumulableInfo]) =
             taskEnd.reason match {
               case org.apache.spark.Success =>
                 stageData.completedIndices.add(info.index)
                 stageData.numCompleteTasks += 1
    -            (None, Option(taskEnd.taskMetrics))
    -          case e: ExceptionFailure =>  // Handle ExceptionFailure because we might have metrics
    +            (None, taskEnd.taskMetrics.accumulatorUpdates())
    +          case e: ExceptionFailure => // Handle ExceptionFailure because we might have metrics
                 stageData.numFailedTasks += 1
    -            (Some(e.toErrorString), e.metrics)
    -          case e: TaskFailedReason =>  // All other failure cases
    +            (Some(e.toErrorString), e.accumUpdates)
    +          case e: TaskFailedReason => // All other failure cases
                 stageData.numFailedTasks += 1
    -            (Some(e.toErrorString), None)
    +            (Some(e.toErrorString), Seq.empty[AccumulableInfo])
             }
     
    -      metrics.foreach { m =>
    +      if (metrics.nonEmpty) {
             val oldMetrics = stageData.taskData.get(info.taskId).flatMap(_.taskMetrics)
    -        updateAggregateMetrics(stageData, info.executorId, m, oldMetrics)
    +        updateAggregateMetrics(stageData, info.executorId,
    +          TaskMetrics.fromAccumulatorUpdates(metrics), oldMetrics)
    --- End diff --
    
    Create metrics once


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/11544#issuecomment-197101208
  
    Merged build finished. Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:

    https://github.com/apache/spark/pull/11544#discussion_r55905361
  
    --- Diff: core/src/main/scala/org/apache/spark/ui/jobs/JobProgressListener.scala ---
    @@ -374,28 +374,30 @@ class JobProgressListener(conf: SparkConf) extends SparkListener with Logging {
           execSummary.taskTime += info.duration
           stageData.numActiveTasks -= 1
     
    -      val (errorMessage, metrics): (Option[String], Option[TaskMetrics]) =
    +      val (errorMessage, metrics): (Option[String], Seq[AccumulableInfo]) =
             taskEnd.reason match {
               case org.apache.spark.Success =>
                 stageData.completedIndices.add(info.index)
                 stageData.numCompleteTasks += 1
    -            (None, Option(taskEnd.taskMetrics))
    -          case e: ExceptionFailure =>  // Handle ExceptionFailure because we might have metrics
    +            (None, taskEnd.taskMetrics.accumulatorUpdates())
    +          case e: ExceptionFailure => // Handle ExceptionFailure because we might have metrics
                 stageData.numFailedTasks += 1
    -            (Some(e.toErrorString), e.metrics)
    -          case e: TaskFailedReason =>  // All other failure cases
    +            (Some(e.toErrorString), e.accumUpdates)
    +          case e: TaskFailedReason => // All other failure cases
                 stageData.numFailedTasks += 1
    -            (Some(e.toErrorString), None)
    +            (Some(e.toErrorString), Seq.empty[AccumulableInfo])
             }
     
    -      metrics.foreach { m =>
    +      val taskMetrics: Option[TaskMetrics] = if (metrics.nonEmpty) Option(TaskMetrics.
    +        fromAccumulatorUpdates(metrics)) else None
    --- End diff --
    
    I see, can we do this then:
    ```
    val taskMetrics =
      if (metrics.nonEmpty) {
        Some(TaskMetrics.fromAccumulatorUpdates(metrics))
      } else {
        None
      }
    ```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by GayathriMurali <gi...@git.apache.org>.
Github user GayathriMurali commented on a diff in the pull request:

    https://github.com/apache/spark/pull/11544#discussion_r55875321
  
    --- Diff: core/src/main/scala/org/apache/spark/ui/jobs/JobProgressListener.scala ---
    @@ -374,28 +374,30 @@ class JobProgressListener(conf: SparkConf) extends SparkListener with Logging {
           execSummary.taskTime += info.duration
           stageData.numActiveTasks -= 1
     
    -      val (errorMessage, metrics): (Option[String], Option[TaskMetrics]) =
    +      val (errorMessage, metrics): (Option[String], Seq[AccumulableInfo]) =
             taskEnd.reason match {
               case org.apache.spark.Success =>
                 stageData.completedIndices.add(info.index)
                 stageData.numCompleteTasks += 1
    -            (None, Option(taskEnd.taskMetrics))
    -          case e: ExceptionFailure =>  // Handle ExceptionFailure because we might have metrics
    +            (None, taskEnd.taskMetrics.accumulatorUpdates())
    +          case e: ExceptionFailure => // Handle ExceptionFailure because we might have metrics
                 stageData.numFailedTasks += 1
    -            (Some(e.toErrorString), e.metrics)
    -          case e: TaskFailedReason =>  // All other failure cases
    +            (Some(e.toErrorString), e.accumUpdates)
    +          case e: TaskFailedReason => // All other failure cases
                 stageData.numFailedTasks += 1
    -            (Some(e.toErrorString), None)
    +            (Some(e.toErrorString), Seq.empty[AccumulableInfo])
             }
     
    -      metrics.foreach { m =>
    +      val taskMetrics: Option[TaskMetrics] = if (metrics.nonEmpty) Option(TaskMetrics.
    +        fromAccumulatorUpdates(metrics)) else None
    --- End diff --
    
    `fromAccumulatorUpdates` does not work if accums is Null/Nill/None. So when there is an exception, accums is Nil and from AccumulatorUpdates will throw an exception. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by jodersky <gi...@git.apache.org>.
Github user jodersky commented on a diff in the pull request:

    https://github.com/apache/spark/pull/11544#discussion_r55442441
  
    --- Diff: core/src/main/scala/org/apache/spark/ui/jobs/JobProgressListener.scala ---
    @@ -374,28 +374,29 @@ class JobProgressListener(conf: SparkConf) extends SparkListener with Logging {
           execSummary.taskTime += info.duration
           stageData.numActiveTasks -= 1
     
    -      val (errorMessage, metrics): (Option[String], Option[TaskMetrics]) =
    +      val (errorMessage, metrics): (Option[String], Seq[AccumulableInfo]) =
             taskEnd.reason match {
               case org.apache.spark.Success =>
                 stageData.completedIndices.add(info.index)
                 stageData.numCompleteTasks += 1
    -            (None, Option(taskEnd.taskMetrics))
    -          case e: ExceptionFailure =>  // Handle ExceptionFailure because we might have metrics
    +            (None, taskEnd.taskMetrics.accumulatorUpdates())
    +          case e: ExceptionFailure => // Handle ExceptionFailure because we might have metrics
                 stageData.numFailedTasks += 1
    -            (Some(e.toErrorString), e.metrics)
    -          case e: TaskFailedReason =>  // All other failure cases
    +            (Some(e.toErrorString), e.accumUpdates)
    +          case e: TaskFailedReason => // All other failure cases
                 stageData.numFailedTasks += 1
    -            (Some(e.toErrorString), None)
    +            (Some(e.toErrorString), Seq.empty[AccumulableInfo])
             }
     
    -      metrics.foreach { m =>
    +      if (metrics.nonEmpty) {
             val oldMetrics = stageData.taskData.get(info.taskId).flatMap(_.taskMetrics)
    -        updateAggregateMetrics(stageData, info.executorId, m, oldMetrics)
    +        updateAggregateMetrics(stageData, info.executorId,
    +          TaskMetrics.fromAccumulatorUpdates(metrics), oldMetrics)
           }
    -
           val taskData = stageData.taskData.getOrElseUpdate(info.taskId, new TaskUIData(info))
           taskData.taskInfo = info
    -      taskData.taskMetrics = metrics
    +      taskData.taskMetrics = if (metrics.nonEmpty) Option(TaskMetrics.
    +        fromAccumulatorUpdates(metrics)) else None
    --- End diff --
    
    > This exact code would error out , as Option[TaskMetrics] cannot be assigned to TaskMetrics.
    Sorry that's a typo in my code, I meant
    ```scala
    val taskMetrics: Option[TaskMetrics] = if (metrics.nonEmpty) {   Some(TaskMetrics.fromAccumulatorUpdates(metrics)) 
    } else {
      None
    }```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:

    https://github.com/apache/spark/pull/11544#issuecomment-195504342
  
    ok to test


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/11544#issuecomment-197101210
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/53242/
    Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/11544#issuecomment-195564066
  
    Merged build finished. Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:

    https://github.com/apache/spark/pull/11544#discussion_r55874631
  
    --- Diff: core/src/main/scala/org/apache/spark/ui/jobs/JobProgressListener.scala ---
    @@ -374,28 +374,30 @@ class JobProgressListener(conf: SparkConf) extends SparkListener with Logging {
           execSummary.taskTime += info.duration
           stageData.numActiveTasks -= 1
     
    -      val (errorMessage, metrics): (Option[String], Option[TaskMetrics]) =
    +      val (errorMessage, metrics): (Option[String], Seq[AccumulableInfo]) =
             taskEnd.reason match {
               case org.apache.spark.Success =>
                 stageData.completedIndices.add(info.index)
                 stageData.numCompleteTasks += 1
    -            (None, Option(taskEnd.taskMetrics))
    -          case e: ExceptionFailure =>  // Handle ExceptionFailure because we might have metrics
    +            (None, taskEnd.taskMetrics.accumulatorUpdates())
    +          case e: ExceptionFailure => // Handle ExceptionFailure because we might have metrics
                 stageData.numFailedTasks += 1
    -            (Some(e.toErrorString), e.metrics)
    -          case e: TaskFailedReason =>  // All other failure cases
    +            (Some(e.toErrorString), e.accumUpdates)
    +          case e: TaskFailedReason => // All other failure cases
                 stageData.numFailedTasks += 1
    -            (Some(e.toErrorString), None)
    +            (Some(e.toErrorString), Seq.empty[AccumulableInfo])
             }
     
    -      metrics.foreach { m =>
    +      val taskMetrics: Option[TaskMetrics] = if (metrics.nonEmpty) Option(TaskMetrics.
    +        fromAccumulatorUpdates(metrics)) else None
    --- End diff --
    
    Why not just?
    `val taskMetrics = TaskMetrics.fromAccumulatorUpdates(accums)`
    Are you worried that this may fail? Then you can just use a `Try`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/11544#issuecomment-192818252
  
    Can one of the admins verify this patch?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:

    https://github.com/apache/spark/pull/11544#discussion_r56219394
  
    --- Diff: core/src/main/scala/org/apache/spark/ui/jobs/JobProgressListener.scala ---
    @@ -374,28 +374,30 @@ class JobProgressListener(conf: SparkConf) extends SparkListener with Logging {
           execSummary.taskTime += info.duration
           stageData.numActiveTasks -= 1
     
    -      val (errorMessage, metrics): (Option[String], Option[TaskMetrics]) =
    +      val (errorMessage, metrics): (Option[String], Seq[AccumulableInfo]) =
    --- End diff --
    
    @GayathriMurali this still needs to change


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:

    https://github.com/apache/spark/pull/11544#discussion_r55874351
  
    --- Diff: core/src/main/scala/org/apache/spark/ui/jobs/JobProgressListener.scala ---
    @@ -374,28 +374,30 @@ class JobProgressListener(conf: SparkConf) extends SparkListener with Logging {
           execSummary.taskTime += info.duration
           stageData.numActiveTasks -= 1
     
    -      val (errorMessage, metrics): (Option[String], Option[TaskMetrics]) =
    +      val (errorMessage, metrics): (Option[String], Seq[AccumulableInfo]) =
             taskEnd.reason match {
               case org.apache.spark.Success =>
                 stageData.completedIndices.add(info.index)
                 stageData.numCompleteTasks += 1
    -            (None, Option(taskEnd.taskMetrics))
    -          case e: ExceptionFailure =>  // Handle ExceptionFailure because we might have metrics
    +            (None, taskEnd.taskMetrics.accumulatorUpdates())
    +          case e: ExceptionFailure => // Handle ExceptionFailure because we might have metrics
                 stageData.numFailedTasks += 1
    -            (Some(e.toErrorString), e.metrics)
    -          case e: TaskFailedReason =>  // All other failure cases
    +            (Some(e.toErrorString), e.accumUpdates)
    +          case e: TaskFailedReason => // All other failure cases
                 stageData.numFailedTasks += 1
    -            (Some(e.toErrorString), None)
    +            (Some(e.toErrorString), Seq.empty[AccumulableInfo])
             }
     
    -      metrics.foreach { m =>
    +      val taskMetrics: Option[TaskMetrics] = if (metrics.nonEmpty) Option(TaskMetrics.
    +        fromAccumulatorUpdates(metrics)) else None
    +      taskMetrics.foreach{m =>
    --- End diff --
    
    style: `taskMetrics.foreach { m =>`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by jodersky <gi...@git.apache.org>.
Github user jodersky commented on the pull request:

    https://github.com/apache/spark/pull/11544#issuecomment-193470576
  
    You create TaskMetrics twice (see my code comments). I'm not sure of the actual impact, but it looks as though a duplicate is not needed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/11544#issuecomment-196901679
  
    **[Test build #53204 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/53204/consoleFull)** for PR 11544 at commit [`9beb858`](https://github.com/apache/spark/commit/9beb8583a16a929683ea69653411a704acef3fc5).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/11544#issuecomment-196537280
  
    **[Test build #53117 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/53117/consoleFull)** for PR 11544 at commit [`c675798`](https://github.com/apache/spark/commit/c67579865352a11ab8ae2be2ee9afc4dfb7399cf).
     * This patch **fails MiMa tests**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:

    https://github.com/apache/spark/pull/11544#discussion_r55874268
  
    --- Diff: core/src/main/scala/org/apache/spark/ui/jobs/JobProgressListener.scala ---
    @@ -374,28 +374,30 @@ class JobProgressListener(conf: SparkConf) extends SparkListener with Logging {
           execSummary.taskTime += info.duration
           stageData.numActiveTasks -= 1
     
    -      val (errorMessage, metrics): (Option[String], Option[TaskMetrics]) =
    +      val (errorMessage, metrics): (Option[String], Seq[AccumulableInfo]) =
    --- End diff --
    
    call this accums


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:

    https://github.com/apache/spark/pull/11544#discussion_r56219651
  
    --- Diff: core/src/main/scala/org/apache/spark/ui/jobs/JobProgressListener.scala ---
    @@ -374,28 +374,30 @@ class JobProgressListener(conf: SparkConf) extends SparkListener with Logging {
           execSummary.taskTime += info.duration
           stageData.numActiveTasks -= 1
     
    -      val (errorMessage, metrics): (Option[String], Option[TaskMetrics]) =
    +      val (errorMessage, metrics): (Option[String], Seq[AccumulableInfo]) =
             taskEnd.reason match {
               case org.apache.spark.Success =>
                 stageData.completedIndices.add(info.index)
                 stageData.numCompleteTasks += 1
    -            (None, Option(taskEnd.taskMetrics))
    -          case e: ExceptionFailure =>  // Handle ExceptionFailure because we might have metrics
    +            (None, taskEnd.taskMetrics.accumulatorUpdates())
    +          case e: ExceptionFailure => // Handle ExceptionFailure because we might have metrics
                 stageData.numFailedTasks += 1
    -            (Some(e.toErrorString), e.metrics)
    -          case e: TaskFailedReason =>  // All other failure cases
    +            (Some(e.toErrorString), e.accumUpdates)
    +          case e: TaskFailedReason => // All other failure cases
                 stageData.numFailedTasks += 1
    -            (Some(e.toErrorString), None)
    +            (Some(e.toErrorString), Seq.empty[AccumulableInfo])
             }
     
    -      metrics.foreach { m =>
    +      val taskMetrics = if (metrics.nonEmpty) Some(TaskMetrics.
    --- End diff --
    
    The line break here is odd; can this not go on one line? if not I'd probably do:
    ```
    val taskMetrics = if (metrics.nonEmpty) 
        Some(TaskMetrics.fromAccumulatorUpdates(metrics)) 
      else 
        None
    ```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by jodersky <gi...@git.apache.org>.
Github user jodersky commented on a diff in the pull request:

    https://github.com/apache/spark/pull/11544#discussion_r55278991
  
    --- Diff: core/src/main/scala/org/apache/spark/ui/jobs/JobProgressListener.scala ---
    @@ -374,28 +374,29 @@ class JobProgressListener(conf: SparkConf) extends SparkListener with Logging {
           execSummary.taskTime += info.duration
           stageData.numActiveTasks -= 1
     
    -      val (errorMessage, metrics): (Option[String], Option[TaskMetrics]) =
    +      val (errorMessage, metrics): (Option[String], Seq[AccumulableInfo]) =
             taskEnd.reason match {
               case org.apache.spark.Success =>
                 stageData.completedIndices.add(info.index)
                 stageData.numCompleteTasks += 1
    -            (None, Option(taskEnd.taskMetrics))
    -          case e: ExceptionFailure =>  // Handle ExceptionFailure because we might have metrics
    +            (None, taskEnd.taskMetrics.accumulatorUpdates())
    +          case e: ExceptionFailure => // Handle ExceptionFailure because we might have metrics
                 stageData.numFailedTasks += 1
    -            (Some(e.toErrorString), e.metrics)
    -          case e: TaskFailedReason =>  // All other failure cases
    +            (Some(e.toErrorString), e.accumUpdates)
    +          case e: TaskFailedReason => // All other failure cases
                 stageData.numFailedTasks += 1
    -            (Some(e.toErrorString), None)
    +            (Some(e.toErrorString), Seq.empty[AccumulableInfo])
             }
     
    -      metrics.foreach { m =>
    +      if (metrics.nonEmpty) {
             val oldMetrics = stageData.taskData.get(info.taskId).flatMap(_.taskMetrics)
    -        updateAggregateMetrics(stageData, info.executorId, m, oldMetrics)
    +        updateAggregateMetrics(stageData, info.executorId,
    +          TaskMetrics.fromAccumulatorUpdates(metrics), oldMetrics)
           }
    -
           val taskData = stageData.taskData.getOrElseUpdate(info.taskId, new TaskUIData(info))
           taskData.taskInfo = info
    -      taskData.taskMetrics = metrics
    +      taskData.taskMetrics = if (metrics.nonEmpty) Option(TaskMetrics.
    +        fromAccumulatorUpdates(metrics)) else None
    --- End diff --
    
    ... and a second time. Is this intentional?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by GayathriMurali <gi...@git.apache.org>.
Github user GayathriMurali commented on a diff in the pull request:

    https://github.com/apache/spark/pull/11544#discussion_r55441647
  
    --- Diff: core/src/main/scala/org/apache/spark/ui/jobs/JobProgressListener.scala ---
    @@ -374,28 +374,29 @@ class JobProgressListener(conf: SparkConf) extends SparkListener with Logging {
           execSummary.taskTime += info.duration
           stageData.numActiveTasks -= 1
     
    -      val (errorMessage, metrics): (Option[String], Option[TaskMetrics]) =
    +      val (errorMessage, metrics): (Option[String], Seq[AccumulableInfo]) =
             taskEnd.reason match {
               case org.apache.spark.Success =>
                 stageData.completedIndices.add(info.index)
                 stageData.numCompleteTasks += 1
    -            (None, Option(taskEnd.taskMetrics))
    -          case e: ExceptionFailure =>  // Handle ExceptionFailure because we might have metrics
    +            (None, taskEnd.taskMetrics.accumulatorUpdates())
    +          case e: ExceptionFailure => // Handle ExceptionFailure because we might have metrics
                 stageData.numFailedTasks += 1
    -            (Some(e.toErrorString), e.metrics)
    -          case e: TaskFailedReason =>  // All other failure cases
    +            (Some(e.toErrorString), e.accumUpdates)
    +          case e: TaskFailedReason => // All other failure cases
                 stageData.numFailedTasks += 1
    -            (Some(e.toErrorString), None)
    +            (Some(e.toErrorString), Seq.empty[AccumulableInfo])
             }
     
    -      metrics.foreach { m =>
    +      if (metrics.nonEmpty) {
             val oldMetrics = stageData.taskData.get(info.taskId).flatMap(_.taskMetrics)
    -        updateAggregateMetrics(stageData, info.executorId, m, oldMetrics)
    +        updateAggregateMetrics(stageData, info.executorId,
    +          TaskMetrics.fromAccumulatorUpdates(metrics), oldMetrics)
           }
    -
           val taskData = stageData.taskData.getOrElseUpdate(info.taskId, new TaskUIData(info))
           taskData.taskInfo = info
    -      taskData.taskMetrics = metrics
    +      taskData.taskMetrics = if (metrics.nonEmpty) Option(TaskMetrics.
    +        fromAccumulatorUpdates(metrics)) else None
    --- End diff --
    
    This exact code would error out , as Option[TaskMetrics] cannot be assigned to TaskMetrics. But I get the idea, will make changes. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by jodersky <gi...@git.apache.org>.
Github user jodersky commented on the pull request:

    https://github.com/apache/spark/pull/11544#issuecomment-195208607
  
    It can also be that jenkins is busy. If the test doesn't start in a couple of hours it would probably be best to contact one of the committers, maybe Sean Owen since he previously commented on this PR


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-13396] Stop using our internal deprecat...

Posted by GayathriMurali <gi...@git.apache.org>.
Github user GayathriMurali commented on a diff in the pull request:

    https://github.com/apache/spark/pull/11544#discussion_r55430280
  
    --- Diff: core/src/main/scala/org/apache/spark/ui/jobs/JobProgressListener.scala ---
    @@ -374,28 +374,29 @@ class JobProgressListener(conf: SparkConf) extends SparkListener with Logging {
           execSummary.taskTime += info.duration
           stageData.numActiveTasks -= 1
     
    -      val (errorMessage, metrics): (Option[String], Option[TaskMetrics]) =
    +      val (errorMessage, metrics): (Option[String], Seq[AccumulableInfo]) =
             taskEnd.reason match {
               case org.apache.spark.Success =>
                 stageData.completedIndices.add(info.index)
                 stageData.numCompleteTasks += 1
    -            (None, Option(taskEnd.taskMetrics))
    -          case e: ExceptionFailure =>  // Handle ExceptionFailure because we might have metrics
    +            (None, taskEnd.taskMetrics.accumulatorUpdates())
    +          case e: ExceptionFailure => // Handle ExceptionFailure because we might have metrics
                 stageData.numFailedTasks += 1
    -            (Some(e.toErrorString), e.metrics)
    -          case e: TaskFailedReason =>  // All other failure cases
    +            (Some(e.toErrorString), e.accumUpdates)
    +          case e: TaskFailedReason => // All other failure cases
                 stageData.numFailedTasks += 1
    -            (Some(e.toErrorString), None)
    +            (Some(e.toErrorString), Seq.empty[AccumulableInfo])
             }
     
    -      metrics.foreach { m =>
    +      if (metrics.nonEmpty) {
             val oldMetrics = stageData.taskData.get(info.taskId).flatMap(_.taskMetrics)
    -        updateAggregateMetrics(stageData, info.executorId, m, oldMetrics)
    +        updateAggregateMetrics(stageData, info.executorId,
    +          TaskMetrics.fromAccumulatorUpdates(metrics), oldMetrics)
           }
    -
           val taskData = stageData.taskData.getOrElseUpdate(info.taskId, new TaskUIData(info))
           taskData.taskInfo = info
    -      taskData.taskMetrics = metrics
    +      taskData.taskMetrics = if (metrics.nonEmpty) Option(TaskMetrics.
    +        fromAccumulatorUpdates(metrics)) else None
    --- End diff --
    
    @jodersky `fromAccumulatorUpdates` method errors out when a None/nil value is passed to it. So it is important to call the method within i`f(metrics.nonEmpty)`. So it is intentional. I am open to thoughts on if the redundancy can be removed. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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