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-3433][BUILD] Fix for Mima false-positives with @DeveloperAPI and @Experimental annotations. #2285

Closed
wants to merge 3 commits into from

Conversation

ScrapCodes
Copy link
Member

Actually false positive reported was due to mima generator not picking up the new jars in presence of old jars(theoretically this should not have happened.). So as a workaround, ran them both separately and just append them together.

@@ -187,7 +187,7 @@ object OldDeps {
Some("org.apache.spark" % fullId % "1.0.0")
}

def oldDepsSettings() = Defaults.defaultSettings ++ Seq(
def oldDepsSettings() = Defaults.coreDefaultSettings ++ Seq(
Copy link
Member Author

Choose a reason for hiding this comment

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

This just fixes a deprecated warning.

@@ -25,12 +25,16 @@ FWDIR="$(cd `dirname $0`/..; pwd)"
cd "$FWDIR"

echo -e "q\n" | sbt/sbt oldDeps/update
rm -f .generated-mima*

./bin/spark-class org.apache.spark.tools.GenerateMIMAIgnore new
Copy link
Member Author

Choose a reason for hiding this comment

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

run with just new jars first.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the args array being used in GenerateMIMAIgnore.scala, where are the "new" and "old" arguments being used anywhere?

@ScrapCodes ScrapCodes changed the title Fix for false positives reported by mima on PR 2194. [BUILD] Fix for false positives reported by mima on PR 2194. Sep 5, 2014
@SparkQA
Copy link

SparkQA commented Sep 5, 2014

QA tests have started for PR 2285 at commit 24f3381.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 5, 2014

QA tests have finished for PR 2285 at commit 24f3381.

  • This patch passes unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class AddJar(path: String) extends LeafNode with Command

@rxin
Copy link
Contributor

rxin commented Sep 6, 2014

Can you change an old API to make sure this is reporting correctly?

@ScrapCodes
Copy link
Member Author

test this please.

@SparkQA
Copy link

SparkQA commented Sep 8, 2014

QA tests have started for PR 2285 at commit be0d196.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 8, 2014

QA tests have finished for PR 2285 at commit be0d196.

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

@ScrapCodes
Copy link
Member Author

@rxin The test failed as expected. I will fix it once you confirm the code change.

@rxin
Copy link
Contributor

rxin commented Sep 8, 2014

LGTM (although I don't really understand this part very well).

@ScrapCodes ScrapCodes changed the title [BUILD] Fix for false positives reported by mima on PR 2194. [SPARK-3433][BUILD] Fix for Mima false-positives with @DeveloperAPI and @Experimental annotations. Sep 8, 2014
@ScrapCodes
Copy link
Member Author

I will let @JoshRosen take a look too, since he reported this issue as well.

@SparkQA
Copy link

SparkQA commented Sep 8, 2014

QA tests have started for PR 2285 at commit 35b6c71.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 8, 2014

QA tests have finished for PR 2285 at commit 35b6c71.

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

@@ -25,11 +25,15 @@ FWDIR="$(cd `dirname $0`/..; pwd)"
cd "$FWDIR"

echo -e "q\n" | sbt/sbt oldDeps/update
rm -f .generated-mima*

./bin/spark-class org.apache.spark.tools.GenerateMIMAIgnore
Copy link
Contributor

Choose a reason for hiding this comment

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

does this need to be called twice? It's now called here and below as well. If it is needed twice, it would be good to document why,

@SparkQA
Copy link

SparkQA commented Sep 9, 2014

QA tests have started for PR 2285 at commit 093c76f.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 9, 2014

QA tests have finished for PR 2285 at commit 093c76f.

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

@ScrapCodes
Copy link
Member Author

Jenkins, retest this please.

@SparkQA
Copy link

SparkQA commented Sep 12, 2014

QA tests have started for PR 2285 at commit 093c76f.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 12, 2014

QA tests have finished for PR 2285 at commit 093c76f.

  • This patch passes unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • throw new IllegalStateException("The main method in the given main class must be static")
    • case class CreateTableAsSelect(
    • case class CreateTableAsSelect(

@rxin
Copy link
Contributor

rxin commented Sep 16, 2014

@ScrapCodes is this good to merge now?

@ScrapCodes
Copy link
Member Author

Yes, it should fix this problem.

@rxin
Copy link
Contributor

rxin commented Sep 16, 2014

Jenkins, retest this please.

@SparkQA
Copy link

SparkQA commented Sep 16, 2014

QA tests have started for PR 2285 at commit 093c76f.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 16, 2014

QA tests have finished for PR 2285 at commit 093c76f.

  • This patch passes unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class ArrayConstructor extends net.razorvine.pickle.objects.ArrayConstructor
    • class SCCallSiteSync(object):

@rxin
Copy link
Contributor

rxin commented Sep 16, 2014

Ok merging this one. Thanks!

@asfgit asfgit closed this in ecf0c02 Sep 16, 2014
@JoshRosen
Copy link
Contributor

I've backported this to branch-1.1 (1.1.2) in order to fix a false-positive MiMa failure in that branch.

asfgit pushed a commit that referenced this pull request Jan 13, 2015
@experimental annotations.

Actually false positive reported was due to mima generator not picking up the new jars in presence of old jars(theoretically this should not have happened.). So as a workaround, ran them both separately and just append them together.

Author: Prashant Sharma <prashant@apache.org>
Author: Prashant Sharma <prashant.s@imaginea.com>

Closes #2285 from ScrapCodes/mima-fix and squashes the following commits:

093c76f [Prashant Sharma] Update mima
59012a8 [Prashant Sharma] Update mima
35b6c71 [Prashant Sharma] SPARK-3433 Fix for Mima false-positives with @DeveloperAPI and @experimental annotations.

(cherry picked from commit ecf0c02)
Signed-off-by: Josh Rosen <joshrosen@databricks.com>

Conflicts:
	project/MimaExcludes.scala
@ScrapCodes ScrapCodes deleted the mima-fix branch June 3, 2015 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants