Skip to content

[SPARK-6371] [build] Update version to 1.4.0-SNAPSHOT. #5056

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 9 commits into from
Closed

[SPARK-6371] [build] Update version to 1.4.0-SNAPSHOT. #5056

wants to merge 9 commits into from

Conversation

@vanzin
Copy link
Contributor

@vanzin vanzin commented Mar 17, 2015

No description provided.

@SparkQA
Copy link

SparkQA commented Mar 17, 2015

Test build #28686 has started for PR 5056 at commit 296cf82.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Mar 17, 2015

Test build #28686 has finished for PR 5056 at commit 296cf82.

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

@AmplabJenkins
Copy link

AmplabJenkins commented Mar 17, 2015

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28686/
Test PASSed.

@sryza
Copy link
Contributor

sryza commented Mar 17, 2015

What's the rationale behind excluding graph from MIMA? And no longer excluding ml?

sryza
sryza reviewed Mar 17, 2015
View reviewed changes
project/MimaExcludes.scala Outdated
Copy link
Contributor

@sryza sryza Mar 17, 2015

Choose a reason for hiding this comment

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

Should be indented two spaces back.

@srowen
Copy link
Member

srowen commented Mar 17, 2015

I think ml still needs to be excluded. I though graphx was still excluded in 1.3 but it doesn't seem like it was so I guess we're tracking API changes there now.

Modulo that and the tiny spacing change, yeah we need to update the snapshot version.

@vanzin
Copy link
Contributor Author

vanzin commented Mar 17, 2015

I didn't add ml because mima didn't complain (while it did complain for graphx). Can add if it's desired.

@SparkQA
Copy link

SparkQA commented Mar 17, 2015

Test build #28737 has started for PR 5056 at commit cef4603.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Mar 17, 2015

Test build #28737 has finished for PR 5056 at commit cef4603.

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

@AmplabJenkins
Copy link

AmplabJenkins commented Mar 17, 2015

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28737/
Test PASSed.

@srowen
Copy link
Member

srowen commented Mar 18, 2015

Paging @ankurdave : graphx should not be excluded from MiMa now right?
Paging @mengxr : ml still needs to be excluded I imagine?

Ignore ml, don't ignore graphx.
0e8e909 
This matches the 1.3 exclusions; let's see what mima complains about.
@SparkQA
Copy link

SparkQA commented Mar 18, 2015

Test build #28810 has started for PR 5056 at commit 0e8e909.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Mar 18, 2015

Test build #28810 has finished for PR 5056 at commit 0e8e909.

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

@AmplabJenkins
Copy link

AmplabJenkins commented Mar 18, 2015

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28810/
Test FAILed.

@vanzin
Copy link
Contributor Author

vanzin commented Mar 18, 2015

There you go:

[info] spark-graphx: found 1 potential binary incompatibilities (filtered 1)
[error]  * abstract method diff(org.apache.spark.rdd.RDD)org.apache.spark.graphx.VertexRDD in class apache.spark.graphx.VertexRDD does not have a correspondent in old version
[error]    filter with: ProblemFilters.exclude[MissingMethodProblem]    ("org.apache.spark.graphx.VertexRDD.diff")

@SparkQA
Copy link

SparkQA commented Mar 18, 2015

Test build #28830 has started for PR 5056 at commit a45a62c.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Mar 18, 2015

Test build #28830 has finished for PR 5056 at commit a45a62c.

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

@AmplabJenkins
Copy link

AmplabJenkins commented Mar 18, 2015

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28830/
Test PASSed.

@vanzin
Copy link
Contributor Author

vanzin commented Mar 18, 2015

I worked around the graphx warning, but would appreciate @ankurdave taking a look.

@srowen
Copy link
Member

srowen commented Mar 19, 2015

This looks right-er to me since the previous version was changed, and we have only excluded ml. I think this is correct but will wait a bit for comments.

srowen
srowen reviewed Mar 19, 2015
View reviewed changes
graphx/src/main/scala/org/apache/spark/graphx/VertexRDD.scala Outdated
Copy link
Member

@srowen srowen Mar 19, 2015

Choose a reason for hiding this comment

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

Ah BTW I'm curious what this changes, and I wonder why it would resolve a MiMa warning? You're on top of it. I just want to make sure we're not missing anything.

Copy link
Contributor Author

@vanzin vanzin Mar 19, 2015

Choose a reason for hiding this comment

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

The previous version was an abstract method, which mima complains about (since it breaks source code that extends this class). The new version has an implementation of the method that throws a "NotImplemented" exception (don't recall the exact type).

Copy link
Contributor

@rxin rxin Mar 19, 2015

Choose a reason for hiding this comment

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

We shouldn't put ??? here, since it means "we will implement this but we haven't got around to implement it yet".

Copy link
Contributor Author

@vanzin vanzin Mar 20, 2015

Choose a reason for hiding this comment

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

So what's the correct fix? Adding a mima exception is not, for the reason I already mentioned (breaks existing source code that worked with previous version).

Copy link
Contributor

@rxin rxin Mar 20, 2015

Choose a reason for hiding this comment

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

Would it work if we add an exclude rule to mima?

Copy link
Contributor Author

@vanzin vanzin Mar 20, 2015

Choose a reason for hiding this comment

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

See my previous comment. The build would work, but it's not the right fix.

Copy link
Contributor

@rxin rxin Mar 20, 2015

Choose a reason for hiding this comment

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

But this is not meant to be an interface for users to implement. It'd be a problem if this is a user facing class that was meant to be extended by user application code.

Copy link
Contributor Author

@vanzin vanzin Mar 20, 2015

Choose a reason for hiding this comment

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

I see. In that case the exclusion is the right thing, then. I'll update the code.

Copy link
Contributor Author

@vanzin vanzin Mar 20, 2015

Choose a reason for hiding this comment

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

BTW let me use the same fix that Sean mentioned. That will tighten the exclusion so that other methods are still checked by MiMA.

@JoshRosen JoshRosen mentioned this pull request Mar 19, 2015
Closed
@srowen
Copy link
Member

srowen commented Mar 20, 2015

Yeah, I think this should be handled with a MiMa exclusion, on the grounds that this was how it was handled when it was introduced:

45f4c66

Ankur seems OK with that: #4733 (comment)

@SparkQA
Copy link

SparkQA commented Mar 20, 2015

Test build #28924 has started for PR 5056 at commit 75b2375.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Mar 20, 2015

Test build #28924 has finished for PR 5056 at commit 75b2375.

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

@AmplabJenkins
Copy link

AmplabJenkins commented Mar 20, 2015

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28924/
Test FAILed.

@SparkQA
Copy link

SparkQA commented Mar 20, 2015

Test build #28926 has started for PR 5056 at commit 178ba71.

  • This patch merges cleanly.

@pwendell
Copy link
Contributor

pwendell commented Mar 20, 2015

LGTM - I did some searching around and I think this upgrades all the necessary places.

@SparkQA
Copy link

SparkQA commented Mar 20, 2015

Test build #28930 has started for PR 5056 at commit 63220df.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Mar 20, 2015

Test build #28926 has finished for PR 5056 at commit 178ba71.

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

@AmplabJenkins
Copy link

AmplabJenkins commented Mar 20, 2015

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28926/
Test PASSed.

@asfgit asfgit closed this in a745645 Mar 20, 2015
@SparkQA
Copy link

SparkQA commented Mar 20, 2015

Test build #28930 has finished for PR 5056 at commit 63220df.

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

@AmplabJenkins
Copy link

AmplabJenkins commented Mar 20, 2015

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28930/
Test PASSed.

@vanzin vanzin deleted the SPARK-6371 branch March 20, 2015 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@vanzin @SparkQA @AmplabJenkins @sryza @srowen @pwendell @rxin