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.

Sign up for GitHub

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

Jump to bottom

[SPARK-6428] Added explicit types for all public methods in core. #5125

Closed
wants to merge 4 commits into from
Closed

[SPARK-6428] Added explicit types for all public methods in core. #5125

wants to merge 4 commits into from

Conversation

rxin
Copy link
Contributor

@rxin rxin commented Mar 22, 2015

No description provided.

@SparkQA
Copy link

SparkQA commented Mar 22, 2015

Test build #28957 has started for PR 5125 at commit f3f69d1.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Mar 22, 2015

Test build #28957 timed out for PR 5125 at commit f3f69d1 after a configured wait of 120m.

@AmplabJenkins
Copy link

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

@rxin
Copy link
Contributor Author

rxin commented Mar 22, 2015

Jenkins, retest this please.

@SparkQA
Copy link

SparkQA commented Mar 22, 2015

Test build #28968 has started for PR 5125 at commit f3f69d1.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Mar 22, 2015

Test build #28968 has finished for PR 5125 at commit f3f69d1.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class ExecutorCacheTaskLocation(override val host: String, executorId: String)

@AmplabJenkins
Copy link

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

@rxin
Copy link
Contributor Author

rxin commented Mar 22, 2015

Jenkins, retest this please.

@SparkQA
Copy link

SparkQA commented Mar 22, 2015

Test build #28969 has started for PR 5125 at commit f3f69d1.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Mar 22, 2015

Test build #28969 has finished for PR 5125 at commit f3f69d1.

  • This patch fails MiMa tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class ExecutorCacheTaskLocation(override val host: String, executorId: String)

@AmplabJenkins
Copy link

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

@SparkQA
Copy link

SparkQA commented Mar 23, 2015

Test build #28977 has started for PR 5125 at commit 7243c27.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Mar 23, 2015

Test build #28977 has finished for PR 5125 at commit 7243c27.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class ExecutorCacheTaskLocation(override val host: String, executorId: String)

@AmplabJenkins
Copy link

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

squito
squito reviewed Mar 23, 2015
View reviewed changes
core/src/main/scala/org/apache/spark/SparkContext.scala
@@ -986,7 +986,7 @@ class SparkContext(config: SparkConf) extends Logging with ExecutorAllocationCli
union(Seq(first) ++ rest)

/** Get an RDD that has no partitions or elements. */
def emptyRDD[T: ClassTag] = new EmptyRDD[T](this)
def emptyRDD[T: ClassTag]: EmptyRDD[T] = new EmptyRDD[T](this)
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't the return type here by RDD[T], since EmptyRDD is private[spark] and just an implementation detail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it should - except then it broke binary compatibility :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW functions are like this are why we should always declare types explicitly.

Copy link
Member

Choose a reason for hiding this comment

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

This is written up a bit more in https://issues.apache.org/jira/browse/SPARK-2331 which should be reopened for the new "2+" bucket in JIRA. This should be fixed when binary compatibility can be broken. Yes, big +1 to tightening up types like this.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah right, thanks for clarifying.

its really a shame that the scala compiler didn't warn in the first place that this method led to EmptyRDD was escaping its declared scope.

@JoshRosen
Copy link
Contributor

LGTM overall.

@SparkQA
Copy link

SparkQA commented Mar 24, 2015

Test build #29053 has started for PR 5125 at commit 81b66e4.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Mar 24, 2015

Test build #29053 has finished for PR 5125 at commit 81b66e4.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class ExecutorCacheTaskLocation(override val host: String, executorId: String)

@AmplabJenkins
Copy link

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

@SparkQA
Copy link

SparkQA commented Mar 24, 2015

Test build #29054 has started for PR 5125 at commit f471415.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Mar 24, 2015

Test build #29054 has finished for PR 5125 at commit f471415.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class ExecutorCacheTaskLocation(override val host: String, executorId: String)

@AmplabJenkins
Copy link

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

@asfgit asfgit closed this in 4ce2782 Mar 24, 2015
@rxin rxin deleted the core-explicit-type branch March 24, 2015 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
6 participants
@rxin @SparkQA @AmplabJenkins @JoshRosen @squito @srowen