Skip to content

[SPARK-21578][CORE] Add JavaSparkContextSuite#18778

Closed
dongjoon-hyun wants to merge 2 commits intoapache:masterfrom
dongjoon-hyun:SPARK-21578
Closed

[SPARK-21578][CORE] Add JavaSparkContextSuite#18778
dongjoon-hyun wants to merge 2 commits intoapache:masterfrom
dongjoon-hyun:SPARK-21578

Conversation

@dongjoon-hyun
Copy link
Member

@dongjoon-hyun dongjoon-hyun commented Jul 30, 2017

What changes were proposed in this pull request?

Due to SI-8479, SPARK-1093 introduced redundant SparkContext constructors. However, SI-8479 is already fixed in Scala 2.10.5 and Scala 2.11.1.

The real reason to provide this constructor is that Java code can access SparkContext directly. It's Scala behavior, SI-4278. So, this PR adds an explicit testsuite, JavaSparkContextSuite to prevent future regression, and fixes the outdate comment, too.

How was this patch tested?

Pass the Jenkins with a new test suite.

@SparkQA
Copy link

SparkQA commented Jul 31, 2017

Test build #80061 has finished for PR 18778 at commit ad99361.

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

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Jul 31, 2017

The Python failure is irrelevant.

ERROR: test_toPandas_arrow_toggle (pyspark.sql.tests.ArrowTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/anaconda/envs/py3k/lib/python3.4/site-packages/pandas/__init__.py", line 25, in <module>
    from pandas import hashtable, tslib, lib
ImportError: cannot import name 'hashtable'

@dongjoon-hyun
Copy link
Member Author

Retest this please

@gatorsmile
Copy link
Member

Could you check whether JAVA APIs still work? Could you add the related test cases?

@SparkQA
Copy link

SparkQA commented Jul 31, 2017

Test build #80062 has finished for PR 18778 at commit ad99361.

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

@dongjoon-hyun
Copy link
Member Author

Thank you for review, @gatorsmile .
Do you mean JavaSparkContext? This PR didn't change it here.

@dongjoon-hyun
Copy link
Member Author

Retest this please.

@gatorsmile
Copy link
Member

JavaSparkContext is the JAVA friendly SparkContext. Is that possible the JAVA programs could still directly use the SparkContext APIs. I am afraid these changes could break them.

@dongjoon-hyun
Copy link
Member Author

Do you want to create a new test suite for that under https://github.com/apache/spark/tree/master/core/src/test/java/test/org/apache/spark?

The following suite seems to be irrelevant for that purpose because they uses JavaSparkContext only.

  • JavaAPISuite.java
  • Java8RDDAPISuite.java

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Jul 31, 2017

@gatorsmile . I see what your did concern here.

If we are locked in here in order to preserve the previous behavior, what about updating those comments? It's not about SI-8479 anymore. Now, it's just for backward compatibility.

// NOTE: The below constructors could be consolidated using default arguments. Due to		
// Scala bug SI-8479, however, this causes the compile step to fail when generating docs.		
// Until we have a good workaround for that bug the constructors remain broken out.

@SparkQA
Copy link

SparkQA commented Jul 31, 2017

Test build #80069 has finished for PR 18778 at commit ad99361.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Jul 31, 2017

@gatorsmile . Three usages (the comment-out lines) were broken here as you concerned.

  @Test
  public void scalaSparkContext() {
    List<String> jars = List$.MODULE$.empty();
    Map<String, String> environment = Map$.MODULE$.empty();

    new SparkContext(new SparkConf().setMaster("local").setAppName("name")).stop();
    new SparkContext("local", "name", new SparkConf()).stop();
    // new SparkContext("local", "name").stop();
    // new SparkContext("local", "name", "sparkHome").stop();
    // new SparkContext("local", "name", "sparkHome", jars).stop();
    new SparkContext("local", "name", "sparkHome", jars, environment).stop();
  }

This Scala behavior is due to SI-4278 instead of SI-8479. Since SI-4278 is Won'tFix, may I update the description more clearly, or simply remove it?

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-21578][CORE] Consolidate redundant SparkContext constructors [SPARK-21578][CORE] Add JavaSparkContextSuite Jul 31, 2017
@dongjoon-hyun
Copy link
Member Author

@gatorsmile . I fixes the comment and adds explicit test suite to prevent future regression.
How do you think about this?

@SparkQA
Copy link

SparkQA commented Jul 31, 2017

Test build #80071 has finished for PR 18778 at commit 0111c14.

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

@Test
public void javaSparkContext() {
String[] jars = new String[] {};
java.util.Map<String, String> environment = new java.util.HashMap<>();
Copy link
Member

Choose a reason for hiding this comment

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

Nit: just import these classes as usual?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for review, @srowen !
This is due to the import conflicts on Map. Java code still doesn't allow import aliasing. We can do aliasing only in Scala code.

@dongjoon-hyun
Copy link
Member Author

Hi, @gatorsmile and @srowen .
Could you review this PR again?

Copy link
Member

@gatorsmile gatorsmile left a comment

Choose a reason for hiding this comment

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

LGTM

@gatorsmile
Copy link
Member

Thanks! Merging to master.

@asfgit asfgit closed this in 14e7575 Aug 2, 2017
@dongjoon-hyun
Copy link
Member Author

Thank you, @gatorsmile and @srowen !

@dongjoon-hyun dongjoon-hyun deleted the SPARK-21578 branch August 2, 2017 05:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Comments