Tighten up field/method visibility in Executor and made some code more clear to read. #4850

rxin · 2015-03-02T10:12:02Z

I was reading Executor just now and found that some latest changes introduced some weird code path with too much monadic chaining and unnecessary fields. I cleaned it up a bit, and also tightened up the visibility of various fields/methods. Also added some inline documentation to help understand this code better.

SparkQA · 2015-03-02T10:12:41Z

Test build #28166 has started for PR 4850 at commit a5e8ffd.

This patch merges cleanly.

SparkQA · 2015-03-02T11:31:53Z

Test build #28166 has finished for PR 4850 at commit a5e8ffd.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class TaskCommitDenied(jobID: Int, partitionID: Int, attemptID: Int) extends TaskFailedReason
- class ExecutorSource(threadPool: ThreadPoolExecutor, executorId: String) extends Source

AmplabJenkins · 2015-03-02T11:31:57Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28166/
Test PASSed.

sryza · 2015-03-02T15:25:02Z

core/src/main/scala/org/apache/spark/executor/Executor.scala

-  def startDriverHeartbeater() {
-    val interval = conf.getInt("spark.executor.heartbeatInterval", 10000)
+  /** Reports heartbeat and metrics for active tasks to the driver. */
+  private def reportHeartBeat(): Unit = {
    val timeout = AkkaUtils.lookupTimeout(conf)
    val retryAttempts = AkkaUtils.numRetries(conf)
    val retryIntervalMs = AkkaUtils.retryWaitMs(conf)
    val heartbeatReceiverRef = AkkaUtils.makeDriverRef("HeartbeatReceiver", conf, env.actorSystem)


Is making it so that now we read these properties and remake the Akka ref on each heartbeat on purpose? I.e. is actor resolution guaranteed to be cheap after the first time?

nah they are better to be taken out of this function. going to put it in the class itself as fields. once we have our own rpc interface, we can delete this.

SparkQA · 2015-03-02T22:53:03Z

Test build #28195 has started for PR 4850 at commit dcba289.

This patch merges cleanly.

SparkQA · 2015-03-03T00:14:24Z

Test build #28195 has finished for PR 4850 at commit dcba289.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class TaskCommitDenied(jobID: Int, partitionID: Int, attemptID: Int) extends TaskFailedReason
- class ExecutorSource(threadPool: ThreadPoolExecutor, executorId: String) extends Source
- class MatrixFactorizationModel(JavaModelWrapper, Saveable, JavaLoader):
- class Saveable(object):
- class Loader(object):
- class JavaLoader(Loader):
- java_class = ".".join([java_package, cls.__name__])

AmplabJenkins · 2015-03-03T00:14:28Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28195/
Test PASSed.

sryza · 2015-03-05T03:52:56Z

core/src/main/scala/org/apache/spark/executor/Executor.scala

+          metrics.setJvmGCTime(curGCTime - taskRunner.startGCTime)
+
+          if (isLocal) {
+            // JobProgressListener will hold an reference of it during


Could be worth fixing the grammar in this comment.

sryza · 2015-03-05T03:56:37Z

I had a few minor nits. Otherwise, this LGTM.

…e clear to read.

SparkQA · 2015-03-19T22:18:12Z

Test build #28892 has started for PR 4850 at commit 866fc60.

This patch merges cleanly.

JoshRosen · 2015-03-19T22:20:53Z

core/src/main/scala/org/apache/spark/executor/CommitDeniedException.scala

@@ -22,14 +22,12 @@ import org.apache.spark.{TaskCommitDenied, TaskEndReason}
 /**
 * Exception thrown when a task attempts to commit output to HDFS but is denied by the driver.
 */
-class CommitDeniedException(
+private[spark] class CommitDeniedException(


Since this was inadvertently public before, and thus was public in Spark 1.3, I think that this change will cause a MiMa failure once we bump the version to 1.4.0-SNAPSHOT. Therefore, this PR sort of implicitly conflicts with #5056, so we'll have to make sure to re-test whichever PR we merge second.

JoshRosen · 2015-03-19T22:35:27Z

LGTM.

SparkQA · 2015-03-19T23:43:10Z

Test build #28892 has finished for PR 4850 at commit 866fc60.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class TaskCommitDenied(jobID: Int, partitionID: Int, attemptID: Int) extends TaskFailedReason
- class ExecutorSource(threadPool: ThreadPoolExecutor, executorId: String) extends Source

AmplabJenkins · 2015-03-19T23:43:14Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28892/
Test PASSed.

sryza reviewed Mar 2, 2015
View reviewed changes

sryza reviewed Mar 5, 2015
View reviewed changes

rxin added 2 commits March 19, 2015 18:12

Tighten up field/method visibility in Executor and made some code mor…

020efbb

…e clear to read.

Code review feedback.

866fc60

rxin force-pushed the executor branch from dcba289 to 866fc60 Compare March 19, 2015 22:16

JoshRosen reviewed Mar 19, 2015
View reviewed changes

asfgit closed this in 0745a30 Mar 20, 2015

rxin deleted the executor branch March 20, 2015 02:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tighten up field/method visibility in Executor and made some code more clear to read. #4850

Tighten up field/method visibility in Executor and made some code more clear to read. #4850

rxin commented Mar 2, 2015

SparkQA commented Mar 2, 2015

SparkQA commented Mar 2, 2015

AmplabJenkins commented Mar 2, 2015

sryza Mar 2, 2015

rxin Mar 2, 2015

SparkQA commented Mar 2, 2015

SparkQA commented Mar 3, 2015

AmplabJenkins commented Mar 3, 2015

sryza Mar 5, 2015

sryza commented Mar 5, 2015

SparkQA commented Mar 19, 2015

JoshRosen Mar 19, 2015

JoshRosen commented Mar 19, 2015

SparkQA commented Mar 19, 2015

AmplabJenkins commented Mar 19, 2015

Tighten up field/method visibility in Executor and made some code more clear to read. #4850

Tighten up field/method visibility in Executor and made some code more clear to read. #4850

Conversation

rxin commented Mar 2, 2015

SparkQA commented Mar 2, 2015

SparkQA commented Mar 2, 2015

AmplabJenkins commented Mar 2, 2015

sryza Mar 2, 2015

Choose a reason for hiding this comment

rxin Mar 2, 2015

Choose a reason for hiding this comment

SparkQA commented Mar 2, 2015

SparkQA commented Mar 3, 2015

AmplabJenkins commented Mar 3, 2015

sryza Mar 5, 2015

Choose a reason for hiding this comment

sryza commented Mar 5, 2015

SparkQA commented Mar 19, 2015

JoshRosen Mar 19, 2015

Choose a reason for hiding this comment

JoshRosen commented Mar 19, 2015

SparkQA commented Mar 19, 2015

AmplabJenkins commented Mar 19, 2015