Skip to content

Conversation

@weiqingy
Copy link
Contributor

What changes were proposed in this pull request?

Fix ClassCircularityError in ReplSuite tests when Spark is built by Maven build.

How was this patch tested?

(1)

build/mvn -DskipTests -Phadoop-2.3 -Pyarn -Phive -Phive-thriftserver -Pkinesis-asl -Pmesos clean package

Then test:

build/mvn -Dtest=none -DwildcardSuites=org.apache.spark.repl.ReplSuite test

ReplSuite tests passed

(2)
Manual Tests against some Spark applications in Yarn client mode and Yarn cluster mode. Need to check if spark caller contexts are written into HDFS hdfs-audit.log and Yarn RM audit log successfully.

… in Maven build: use 'Class.forName' instead of 'Utils.classForName'
@tgravescs
Copy link
Contributor

please change [Follow UP] to [HOTFIX]

@weiqingy weiqingy changed the title [SPARK-16757][Follow UP] Fix ClassCircularityError in ReplSuite tests in Maven build: use 'Class.forName' instead of 'Utils.classForName' [SPARK-16757][HOTFIX] Fix ClassCircularityError in ReplSuite tests in Maven build: use 'Class.forName' instead of 'Utils.classForName' Sep 28, 2016
@tgravescs
Copy link
Contributor

Also please file a separate jira to investigate more. Link it to https://issues.apache.org/jira/browse/SPARK-15857 and if needed we pull in people more familiar with the Repl classloader stuff.

@weiqingy
Copy link
Contributor Author

Yes. The title has been changed. Thanks. @tgravescs

@tgravescs
Copy link
Contributor

changes look fine, waiting on Jenkins.

@JoshRosen
Copy link
Contributor

Can you change the JIRA number to SPARK-17710, which is the JIRA that I created for this build break?

https://issues.apache.org/jira/browse/SPARK-17710

@weiqingy weiqingy changed the title [SPARK-16757][HOTFIX] Fix ClassCircularityError in ReplSuite tests in Maven build: use 'Class.forName' instead of 'Utils.classForName' [SPARK-17710][HOTFIX] Fix ClassCircularityError in ReplSuite tests in Maven build: use 'Class.forName' instead of 'Utils.classForName' Sep 28, 2016
@weiqingy
Copy link
Contributor Author

@JoshRosen Yes. The title has been changed.
@tgravescs Yes. Now I am creating a separate jira to investigate this more.

@SparkQA
Copy link

SparkQA commented Sep 29, 2016

Test build #66066 has finished for PR 15286 at commit 59bfa23.

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

@tgravescs
Copy link
Contributor

committed this to master

@asfgit asfgit closed this in 7dfad4b Sep 29, 2016
@weiqingy
Copy link
Contributor Author

Thank you very much. @tgravescs @JoshRosen

try {
val callerContext = Utils.classForName("org.apache.hadoop.ipc.CallerContext")
val Builder = Utils.classForName("org.apache.hadoop.ipc.CallerContext$Builder")
// scalastyle:off classforname
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's important to document why we are not using Utils.classForName here. Otherwise somebody is going to come in the future and get confused or change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I will submit a PR for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rxin I have submitted a PR #15776 for this.

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.

5 participants