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.

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

[SPARK-8962] Add Scalastyle rule to ban direct use of Class.forName; fix existing uses #7350

Closed
wants to merge 6 commits into from

Conversation

JoshRosen
Copy link
Contributor

This pull request adds a Scalastyle regex rule which fails the style check if Class.forName is used directly. Class.forName always loads classes from the default / system classloader, but in a majority of cases, we should be using Spark's own Utils.classForName instead, which tries to load classes from the current thread's context classloader and falls back to the classloader which loaded Spark when the context classloader is not defined.

Review on Reviewable

@JoshRosen
Copy link
Contributor Author

/cc @yhuai @tdas

@JoshRosen
Copy link
Contributor Author

This pull request might become intractable to review due to how many files it touches, so I recommend using the Reviewable link to keep track of changes.

@SparkQA
Copy link

SparkQA commented Jul 10, 2015

Test build #37065 has finished for PR 7350 at commit 62882ee.

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

@shivaram
Copy link
Contributor

The RBackendHandler change looks good to me. I do find it weird that we have scala style rules for not using specific functions, but I guess its hard to track this during code review in a large codebase.

@JoshRosen
Copy link
Contributor Author

@shivaram, in this case I think the style rule is justified even if it's inconvenient or a hassle to implement because this Class.forName issue has led to a number of separate bugs over the years.

@rxin
Copy link
Contributor

rxin commented Jul 13, 2015

@JoshRosen looks like you missed a few places. Can you fix them so we can merge this?

@SparkQA
Copy link

SparkQA commented Jul 13, 2015

Test build #1053 has finished for PR 7350 at commit 62882ee.

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

@SparkQA
Copy link

SparkQA commented Jul 13, 2015

Test build #37158 has finished for PR 7350 at commit d707ba7.

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

@SparkQA
Copy link

SparkQA commented Jul 14, 2015

Test build #37159 has finished for PR 7350 at commit c0b7885.

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

@rxin
Copy link
Contributor

rxin commented Jul 14, 2015

Unfortunately another pr created a conflict again ...

@JoshRosen
Copy link
Contributor Author

Argh... will fix now.

@SparkQA
Copy link

SparkQA commented Jul 14, 2015

Test build #37190 has finished for PR 7350 at commit e3e96f7.

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

@JoshRosen
Copy link
Contributor Author

Jenkins, retest this please.

1 similar comment
@JoshRosen
Copy link
Contributor Author

Jenkins, retest this please.

@SparkQA
Copy link

SparkQA commented Jul 14, 2015

Test build #37247 has finished for PR 7350 at commit e3e96f7.

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

@JoshRosen
Copy link
Contributor Author

Jenkins, retest this please.

@SparkQA
Copy link

SparkQA commented Jul 14, 2015

Test build #37260 has finished for PR 7350 at commit e3e96f7.

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

@rxin
Copy link
Contributor

rxin commented Jul 14, 2015

Thanks - merging this in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants