Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-5231][WebUI] History Server shows wrong job submission time. #4029

Closed
wants to merge 5 commits into from

Conversation

sarutak
Copy link
Member

@sarutak sarutak commented Jan 14, 2015

History Server doesn't show collect job submission time.
It's because JobProgressListener updates job submission time every time onJobStart method is invoked from ReplayListenerBus.

@SparkQA
Copy link

SparkQA commented Jan 14, 2015

Test build #25490 has finished for PR 4029 at commit 11107a8.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class SparkListenerJobEnd(

@JoshRosen
Copy link
Contributor

It looks like this failed due to a test compilation error:

[error] /home/jenkins/workspace/SparkPullRequestBuilder/core/src/test/scala/org/apache/spark/scheduler/SparkListenerSuite.scala:47: not enough arguments for method apply: (jobId: Int, time: Option[Long], jobResult: org.apache.spark.scheduler.JobResult)org.apache.spark.scheduler.SparkListenerJobEnd in object SparkListenerJobEnd.
[error] Unspecified value parameter jobResult.
[error]     (1 to 5).foreach { _ => bus.post(SparkListenerJobEnd(0, JobSucceeded)) }
[error]                                                         ^
[error] /home/jenkins/workspace/SparkPullRequestBuilder/core/src/test/scala/org/apache/spark/scheduler/SparkListenerSuite.scala:57: not enough arguments for method apply: (jobId: Int, time: Option[Long], jobResult: org.apache.spark.scheduler.JobResult)org.apache.spark.scheduler.SparkListenerJobEnd in object SparkListenerJobEnd.
[error] Unspecified value parameter jobResult.
[error]     (1 to 5).foreach { _ => bus.post(SparkListenerJobEnd(0, JobSucceeded)) }
[error]                                                         ^
[error] /home/jenkins/workspace/SparkPullRequestBuilder/core/src/test/scala/org/apache/spark/scheduler/SparkListenerSuite.scala:102: not enough arguments for method apply: (jobId: Int, time: Option[Long], jobResult: org.apache.spark.scheduler.JobResult)org.apache.spark.scheduler.SparkListenerJobEnd in object SparkListenerJobEnd.
[error] Unspecified value parameter jobResult.
[error]     bus.post(SparkListenerJobEnd(0, JobSucceeded))
[error]                                 ^
[error] /home/jenkins/workspace/SparkPullRequestBuilder/core/src/test/scala/org/apache/spark/scheduler/SparkListenerSuite.scala:348: not enough arguments for method apply: (jobId: Int, time: Option[Long], jobResult: org.apache.spark.scheduler.JobResult)org.apache.spark.scheduler.SparkListenerJobEnd in object SparkListenerJobEnd.
[error] Unspecified value parameter jobResult.
[error]     (1 to 5).foreach { _ => bus.post(SparkListenerJobEnd(0, JobSucceeded)) }
[error]                                                         ^
[info] Compiling 1 Scala source and 1 Java source to 

To prevent these sorts of issues, you can run sbt/sbt test:compile to compile the tests without running them.

@sarutak
Copy link
Member Author

sarutak commented Jan 14, 2015

Thanks, I just did assembly.
Now the compilation error is resolved.

@SparkQA
Copy link

SparkQA commented Jan 14, 2015

Test build #25516 has finished for PR 4029 at commit 2d47bd3.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class SparkListenerJobEnd(

@SparkQA
Copy link

SparkQA commented Jan 14, 2015

Test build #25527 has finished for PR 4029 at commit 26b9b99.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class SparkListenerJobEnd(
    • val classServer = new HttpServer(conf, outputDir, new SecurityManager(conf), classServerPort, "HTTP class server")
    • case class QualifiedTableName(database: String, name: String)
    • case class CreateMetastoreDataSource(

@JoshRosen
Copy link
Contributor

This is a nice patch, but I wonder whether there's a smaller fix that doesn't require changing SparkListener events; that will make it easier to backport that patch to branch-1.2. The job page already knows the last stage in the job (the result stage), so I think we might be able to use the final stage's completion time as the job completion time and the first stage's submission time as the job start time. However, there are a couple of corner-cases that this might miss: I could submit a job that spends a bunch of time queued behind other jobs before its first stage starts running, in which case it would be helpful to be able to distinguish between scheduler delays and stage durations. Similarly, there might be a corner-case related to the job completion time if we have a job that spends a lot of time fetching results back to the driver after they've been stored in the block manager by completed tasks.

So, I guess the approach here seems like the right fix. I'd guess we might be able to do a separate fix in branch-1.2 to use the first/last stage time approximations.

I have a couple of comments on the code here, so I'll comment on those inline.

@@ -58,6 +58,7 @@ case class SparkListenerTaskEnd(
@DeveloperApi
case class SparkListenerJobStart(
jobId: Int,
time: Option[Long],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is an option for backwards-compatibility reasons? We definitely know the time when posting this event to the listener bus, so I think the right approach is to have time just be a regular Long and pass a dummy value (-1) when replaying JSON that's missing that field.

@sarutak
Copy link
Member Author

sarutak commented Jan 14, 2015

@JoshRosen Thanks for your advises. I reflected your comment and added a test case.
For now, I will take the original approach and also, I will try to address this issue using the approximation approach you mentioned for 1.2.x.
What do you think?

@SparkQA
Copy link

SparkQA commented Jan 14, 2015

Test build #25569 has finished for PR 4029 at commit da8bd14.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class SparkListenerJobEnd(

@SparkQA
Copy link

SparkQA commented Jan 16, 2015

Test build #25627 has finished for PR 4029 at commit 0af9e22.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@andrewor14
Copy link
Contributor

@sarutak @JoshRosen I would prefer to use the same solution for both master and 1.2 if possible, so if something goes wrong we only need to reason about one fix. Since we should not push this PR's current changes to branch-1.2 (it will break upgrades for 1.2.0 users), I would actually rather not fix this in branch-1.2 if we're not using the approximation solution. In general it would be good to minimize the complexity that arises from the divergence of behavior across branches.

@andrewor14
Copy link
Contributor

The changes here actually LGTM, so I will merge this in master. Thanks @sarutak!

@JoshRosen
Copy link
Contributor

If the only backwards incompatibility is via pattern matching on listener events, can we maybe just make the new field a mutable var that's set outside of the constructor? This should avoid that incompatibility even if it's a little messy.

Sent from my phone

On Jan 16, 2015, at 10:04 AM, andrewor14 notifications@github.com wrote:

The changes here actually LGTM, so I will merge this in master. Thanks @sarutak!


Reply to this email directly or view it on GitHub.

@asfgit asfgit closed this in e8422c5 Jan 16, 2015
@andrewor14
Copy link
Contributor

@JoshRosen that could work. If we do decide to do it we must absolutely make sure that we don't break anything for 1.2.0 users, since compatibility in a patch release even for a developer API is expected by most users.

@@ -32,6 +32,7 @@ import org.apache.spark.executor._
import org.apache.spark.scheduler._
import org.apache.spark.storage._
import org.apache.spark._
import org.apache.hadoop.hdfs.web.JsonUtil

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note this breaks Mapr3 builds. <hadoop.version>1.0.3-mapr-3.0.3</hadoop.version>

The version of mapr3 should be upgraded then to 1.0.3-mapr-3.1.1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like this import is even used anywhere, so I think we can probably remove it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JoshRosen you're right. Is this something you're looking after or do you want me to raise a PR? Should I make a JIRA? Its breaking my mapr3 build

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please file a JIRA, since that helps us to track and prioritize bugs. Feel free to open a pull request if removing this import fixes your build.

asfgit pushed a commit that referenced this pull request Feb 3, 2015
Simple PR to Remove unused import JsonUtil from from org.apache.spark.util.JsonProtocol.scala which fails builds with older versions of hadoop-core
This import is unused. It was introduced in PR #4029 #4029 as a part of JIRA SPARK-5231

Author: nemccarthy <nathan@nemccarthy.me>

Closes #4320 from nemccarthy/master and squashes the following commits:

8e34a11 [nemccarthy] [SPARK-5543][WebUI] Remove unused import JsonUtil from from org.apache.spark.util.JsonProtocol.scala which fails builds with older versions of hadoop-core
@sarutak sarutak deleted the SPARK-5231 branch April 11, 2015 05:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants