Skip to content

[SPARK-5549] Define TaskContext interface in Scala. #4324

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
Closed
wants to merge 3 commits into from
Closed

[SPARK-5549] Define TaskContext interface in Scala. #4324

wants to merge 3 commits into from

Conversation

rxin
Copy link
Contributor

@rxin rxin commented Feb 3, 2015

So the interface documentation shows up in ScalaDoc.

@rxin
[SPARK-5549] Define TaskContext interface in Scala.
87dd537 
So the interface documentation shows up in ScalaDoc.
rxin
rxin reviewed Feb 3, 2015
View reviewed changes
core/src/test/java/test/org/apache/spark/JavaTaskCompletionListenerImpl.java
@@ -15,9 +15,10 @@
* limitations under the License.
*/

package org.apache.spark.util;
package test.org.apache.spark;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note that I changed the package to make sure we test package visibility correctly also.

We should do this for other Java API tests, but we can do those later.

@SparkQA
Copy link

SparkQA commented Feb 3, 2015

Test build #26614 has started for PR 4324 at commit 87dd537.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Feb 3, 2015

Test build #26614 has finished for PR 4324 at commit 87dd537.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@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/26614/
Test FAILed.

JoshRosen
JoshRosen reviewed Feb 3, 2015
View reviewed changes
core/src/main/scala/org/apache/spark/TaskContext.scala
/** @deprecated use {@link #isRunningLocally()} */
@Deprecated
public abstract boolean runningLocally();
/** @deprecated use { @link #isRunningLocally()}*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Scalastyle didn't like the whitespace here.

@JoshRosen
Copy link
Contributor

LGTM; this is a pretty straightforward change. Compared to implementing this in Java, this gains the benefit of having this class's docs appear in Scaladoc. I guess there's a potential downside of Java compatibility / usability being broken should we add new methods, but I think we can address that via our review processes. It might be nice to add a comment at the top of the TaskContext and TaskContextImpl files referencing the JavaTaskCompletionListenerImpl test case so that people remember to update it when adding new methods.

@JoshRosen
Copy link
Contributor

I don't know whether MiMa actually checks our Java classes, so that might be another side-benefit of porting this back to Scala.

@SparkQA
Copy link

SparkQA commented Feb 3, 2015

Test build #26618 has started for PR 4324 at commit 573756f.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Feb 3, 2015

Test build #26620 has started for PR 4324 at commit 2480a17.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Feb 3, 2015

Test build #26620 has finished for PR 4324 at commit 2480a17.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@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/26620/
Test FAILed.

@SparkQA
Copy link

SparkQA commented Feb 3, 2015

Test build #26618 has finished for PR 4324 at commit 573756f.

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

@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/26618/
Test PASSed.

@rxin
Copy link
Contributor Author

rxin commented Feb 3, 2015

Jenkins, retest this please.

@SparkQA
Copy link

SparkQA commented Feb 3, 2015

Test build #26633 has started for PR 4324 at commit 2480a17.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Feb 3, 2015

Test build #26633 has finished for PR 4324 at commit 2480a17.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@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/26633/
Test FAILed.

@rxin
Copy link
Contributor Author

rxin commented Feb 3, 2015

Going to merge since tests passed previously, and the latest failure was due to a flaky test in streaming.

@asfgit asfgit closed this in bebf4c4 Feb 3, 2015
@rxin rxin mentioned this pull request Feb 3, 2015
Closed
asfgit pushed a commit that referenced this pull request Feb 3, 2015
@rxin
Minor: Fix TaskContext deprecated annotations.
f7948f3 
Made a mistake in #4324

Author: Reynold Xin <[email protected]>

Closes #4333 from rxin/taskcontext-deprecate and squashes the following commits:

61c44ee [Reynold Xin] Minor: Fix TaskContext deprecated annotations.
@kayousterhout kayousterhout mentioned this pull request Apr 7, 2015
Closed
asfgit pushed a commit that referenced this pull request Apr 8, 2015
@kayousterhout @rxin
[SPARK-6754] Remove unnecessary TaskContextHelper
8d2a36c 
The TaskContextHelper was originally necessary because TaskContext was written in Java, which does
not have a way to specify that classes are package-private, so TaskContextHelper existed to work
around this. Now that TaskContext has been re-written in Scala, this class is no longer necessary.

rxin can you look at this? It looks like you missed this bit of cleanup when you moved TaskContext from Java to Scala in #4324

cc ScrapCodes and pwendell who added this originally.

Author: Kay Ousterhout <[email protected]>

Closes #5402 from kayousterhout/SPARK-6754 and squashes the following commits:

f089800 [Kay Ousterhout] [SPARK-6754] Remove unnecessary TaskContextHelper
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
Development

Successfully merging this pull request may close these issues.
4 participants
@rxin @SparkQA @AmplabJenkins @JoshRosen