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-13928] Move org.apache.spark.Logging into org.apache.spark.internal.Logging #11764

Closed
wants to merge 4 commits into from

Conversation

cloud-fan
Copy link
Contributor

What changes were proposed in this pull request?

Logging was made private in Spark 2.0. If we move it, then users would be able to create a Logging trait themselves to avoid changing their own code.

How was this patch tested?

existing tests.

@srowen
Copy link
Member

srowen commented Mar 16, 2016

What does this change accomplish? I wasn't clear from the JIRA

@SparkQA
Copy link

SparkQA commented Mar 16, 2016

Test build #53322 has finished for PR 11764 at commit 4f12a69.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@cloud-fan
Copy link
Contributor Author

We made the Logging class private and broke user code. To minimize the code change, users can create org.apache.spark.Logging class themselves. However, we have to move our org.apache.spark.Logging to another place or they will conflict. cc @rxin for more details and correct me if I said wrong.

@srowen
Copy link
Member

srowen commented Mar 16, 2016

Logging is an internal class though ... it's not something anyone should use. For 2.x that can be enforced more strongly. I don't think this is harmful as a change but seems unnecessary.

@SparkQA
Copy link

SparkQA commented Mar 16, 2016

Test build #53323 has finished for PR 11764 at commit 60544de.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Mar 16, 2016

Test build #53325 has finished for PR 11764 at commit 98417ff.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@rxin
Copy link
Contributor

rxin commented Mar 16, 2016

@srowen it was only documented as private but not actually set as private in 1.x. As a result, a lot of user applications are unfortunately using it.

@SparkQA
Copy link

SparkQA commented Mar 16, 2016

Test build #53326 has finished for PR 11764 at commit 7b9feda.

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

@JoshRosen
Copy link
Contributor

I think the following exclude should fix things cleanly:

diff --git a/project/MimaExcludes.scala b/project/MimaExcludes.scala
index 2a4a874..ce982d2 100644
--- a/project/MimaExcludes.scala
+++ b/project/MimaExcludes.scala
@@ -322,6 +322,13 @@ object MimaExcludes {
       ) ++ Seq(
         // [SPARK-13686][MLLIB][STREAMING] Add a constructor parameter `reqParam` to (Streaming)LinearRegressionWithSGD
         ProblemFilters.exclude[MissingMethodProblem]("org.apache.spark.mllib.regression.LinearRegressionWithSGD.this")
+      ) ++ Seq(
+        // [SPARK-13928] Move org.apache.spark.Logging into org.apache.spark.internal.Logging
+        (problem: Problem) => problem match {
+          case MissingTypesProblem(_, missing)
+            if missing.map(_.fullName).sameElements(Seq("org.apache.spark.Logging")) => false
+          case _ => true
+        }
       )
     case v if v.startsWith("1.6") =>
       Seq(

@cloud-fan
Copy link
Contributor Author

@JoshRosen thanks a lot! It's very helpful.

@cloud-fan
Copy link
Contributor Author

hi @rxin , this PR changes too many files and can't be reviewed on github. You can fetch my branch and run git diff locally after tests pass. It should be easy to review as most of the changes are 1 or 2 lines per file.

@SparkQA
Copy link

SparkQA commented Mar 17, 2016

Test build #53381 has finished for PR 11764 at commit 6c89f00.

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

@SparkQA
Copy link

SparkQA commented Mar 17, 2016

Test build #53383 has finished for PR 11764 at commit dfe39c6.

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

@cloud-fan
Copy link
Contributor Author

The log says: java.lang.RuntimeException: spark-core: Binary compatibility check failed!, but no reason is provided... cc @JoshRosen

@JoshRosen
Copy link
Contributor

[error]  * trait org.apache.spark.Logging does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[MissingClassProblem]("org.apache.spark.Logging")

@cloud-fan
Copy link
Contributor Author

Sorry I missed it as this message is so far away from the final one... @JoshRosen thanks again!

@SparkQA
Copy link

SparkQA commented Mar 17, 2016

Test build #53385 has finished for PR 11764 at commit 2f9129a.

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

@SparkQA
Copy link

SparkQA commented Mar 17, 2016

Test build #53380 has finished for PR 11764 at commit a155ce2.

  • This patch fails Spark unit tests.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

@rxin
Copy link
Contributor

rxin commented Mar 17, 2016

LGTM - merge it as soon as you get tests to pass.

@cloud-fan
Copy link
Contributor Author

retest this please

@SparkQA
Copy link

SparkQA commented Mar 17, 2016

Test build #53392 has finished for PR 11764 at commit e875d82.

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

@SparkQA
Copy link

SparkQA commented Mar 17, 2016

Test build #53391 has finished for PR 11764 at commit 2f9129a.

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

@SparkQA
Copy link

SparkQA commented Mar 17, 2016

Test build #53396 has finished for PR 11764 at commit 0ee9d40.

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

@SparkQA
Copy link

SparkQA commented Mar 17, 2016

Test build #53403 has finished for PR 11764 at commit 80c11de.

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

@asfgit asfgit closed this in 8ef3399 Mar 17, 2016
@cloud-fan
Copy link
Contributor Author

merging to master

roygao94 pushed a commit to roygao94/spark that referenced this pull request Mar 22, 2016
…ernal.Logging

## What changes were proposed in this pull request?

Logging was made private in Spark 2.0. If we move it, then users would be able to create a Logging trait themselves to avoid changing their own code.

## How was this patch tested?

existing tests.

Author: Wenchen Fan <wenchen@databricks.com>

Closes apache#11764 from cloud-fan/logger.
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