[SPARK-5922][GraphX]: Add diff(other: RDD[VertexId, VD]) in VertexRDD #4733

brennonyork · 2015-02-23T22:00:54Z

Changed method invocation of 'diff' to match that of 'innerJoin' and 'leftJoin' from VertexRDD[VD] to RDD[(VertexId, VD)]. This change maintains backwards compatibility and better unifies the VertexRDD methods to match each other.

…'leftJoin' from VertexRDD[VD] to RDD[(VertexId, VD)]

brennonyork · 2015-02-23T22:01:16Z

/cc @ankurdave

AmplabJenkins · 2015-02-23T22:02:10Z

Can one of the admins verify this patch?

srowen · 2015-02-23T22:02:51Z

ok to test

SparkQA · 2015-02-23T22:07:57Z

Test build #27863 has started for PR 4733 at commit f18356e.

This patch merges cleanly.

SparkQA · 2015-02-23T23:27:36Z

Test build #27863 has finished for PR 4733 at commit f18356e.

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

AmplabJenkins · 2015-02-23T23:27:40Z

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

brennonyork · 2015-02-23T23:35:21Z

@srowen is there any way to re-run this test without the MiMa tests?? The entire point of this PR is to change the public methods even though they wouldn't affect any calling implementations and I'm, honestly, unsure if that means I need to represent that in any way / shape / form in the code. Thoughts?

srowen · 2015-02-24T10:20:00Z

It's a public API, right? I don't see that it or the class is private[graphx] so I think MiMa has a point. You shouldn't remove the method, but add a new one. If you believe the existing one should be deprecated you can do that. But yeah I think it's time for @ankurdave or maybe @jegonzal to weigh in.

PS to answer the question, you can't disable the MiMa checks but would instead add to your PR some new MiMa excludes to tell it this is OK, if you did have to override MiMa.

ankurdave · 2015-02-24T18:44:11Z

Though this is source-compatible, MiMa is correct that it breaks binary compatibility. It might be best to add the more general version as an overload without deprecating the more specific version. The code duplication will be small and I think it's worth it to preserve binary compatibility.

SparkQA · 2015-02-24T19:23:01Z

Test build #27898 has started for PR 4733 at commit 93186f3.

This patch merges cleanly.

SparkQA · 2015-02-24T20:16:17Z

Test build #27898 has finished for PR 4733 at commit 93186f3.

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

AmplabJenkins · 2015-02-24T20:16:22Z

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

brennonyork · 2015-02-24T21:50:40Z

retest this please

SparkQA · 2015-02-24T21:52:37Z

Test build #27910 has started for PR 4733 at commit 93186f3.

This patch merges cleanly.

SparkQA · 2015-02-24T23:08:59Z

Test build #27910 has finished for PR 4733 at commit 93186f3.

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

AmplabJenkins · 2015-02-24T23:09:03Z

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

brennonyork · 2015-02-24T23:33:39Z

Any idea why its failing now? Issue is:

[error]  * abstract method diff(org.apache.spark.rdd.RDD)org.apache.spark.graphx.VertexRDD in class org.apache.spark.graphx.VertexRDD does not have a correspondent in old version

I would have assumed MiMa would be okay with this since its merely adding a new method, but I might be wrong. A series of Google searches and diving into MiMa isn't helping either. Should I just add the MiMa exclude? That feels wrong in this case.

srowen · 2015-02-24T23:37:08Z

@brennonyork it has a point; you've added a new abstract method to an abstract class. This would cause any class that extends the previous version to fail since it does not implement the new method.

brennonyork · 2015-02-25T00:50:18Z

@srowen thanks for that, completely missed that!

If I add the MiMa exclude in this case to resolve this issue then shouldn't we deprecate the diff(other: VertexRDD[VD]) as we don't want to propagate two versions of diff in the abstract class (i.e. make all downstream impl's take care of two diff methods)? Either way the MiMa exclude is in and tested with both methods so I'll push that up now. If we'd rather deprecate the old version of diff at this point let me know and I can push up that fix!

SparkQA · 2015-02-25T01:02:39Z

Test build #27924 has started for PR 4733 at commit 2c678c6.

This patch merges cleanly.

SparkQA · 2015-02-25T02:15:28Z

Test build #27924 has finished for PR 4733 at commit 2c678c6.

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

AmplabJenkins · 2015-02-25T02:15:31Z

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

ankurdave · 2015-02-25T06:35:31Z

The VertexRDD interface is not intended to be implemented by users. I just noticed this isn't explicitly documented, but since VertexRDD references ShippableVertexPartition, which is private[graphx], users should in any case get a compile error when they try to implement it.

So LGTM aside from a minor comment: I don't think it makes sense to deprecate the old version, because users don't have the option of switching to the new version unless they add the noise for widening the argument type.

brennonyork · 2015-02-25T17:22:14Z

Roger, that makes good sense! Thanks for the update @ankurdave. Since the patch isn't deprecating anything I'd say we're good to merge in then? Just let me know if anything else needs done!

ankurdave · 2015-02-25T22:27:26Z

@brennonyork Looks like #4705 introduced a minor merge conflict, so it would be great if you could merge that in.

It seems best to use the definition of diff(VertexRDD[VD]) from there, since I just noticed that this PR does some unnecessary work -- calling VertexRDD(other) will always rebuild the vertex partitions even if that's not required.

Also, it would be good to remove the remaining comment about deprecation.

Thanks!

…xRDD[VD]) method

AmplabJenkins · 2015-03-13T21:59:41Z

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

brennonyork · 2015-03-13T22:43:46Z

jenkins, retest this please

SparkQA · 2015-03-13T22:48:08Z

Test build #28596 has started for PR 4733 at commit 398ddb4.

This patch merges cleanly.

SparkQA · 2015-03-14T00:07:18Z

Test build #28596 has finished for PR 4733 at commit 398ddb4.

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

AmplabJenkins · 2015-03-14T00:07:21Z

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

SparkQA · 2015-03-14T00:53:03Z

Test build #28600 has started for PR 4733 at commit 398ddb4.

This patch merges cleanly.

SparkQA · 2015-03-14T02:14:15Z

Test build #28600 has finished for PR 4733 at commit 398ddb4.

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

AmplabJenkins · 2015-03-14T02:14:19Z

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

srowen · 2015-03-14T08:10:18Z

graphx/src/test/scala/org/apache/spark/graphx/VertexRDDSuite.scala

Nit: keep this with other Spark imports

SparkQA · 2015-03-14T16:43:01Z

Test build #28610 has started for PR 4733 at commit f86375c.

This patch merges cleanly.

SparkQA · 2015-03-14T18:03:55Z

Test build #28610 has finished for PR 4733 at commit f86375c.

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

AmplabJenkins · 2015-03-14T18:03:59Z

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

SparkQA · 2015-03-14T23:48:07Z

Test build #28616 has started for PR 4733 at commit b9274af.

This patch merges cleanly.

SparkQA · 2015-03-15T00:45:06Z

Test build #28616 has finished for PR 4733 at commit b9274af.

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

AmplabJenkins · 2015-03-15T00:45:10Z

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

brennonyork · 2015-03-15T01:46:36Z

jenkins, retest this please

SparkQA · 2015-03-15T01:48:09Z

Test build #28620 has started for PR 4733 at commit b9274af.

This patch merges cleanly.

SparkQA · 2015-03-15T02:45:03Z

Test build #28620 has finished for PR 4733 at commit b9274af.

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

AmplabJenkins · 2015-03-15T02:45:07Z

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

SparkQA · 2015-03-15T18:18:09Z

Test build #28631 has started for PR 4733 at commit e800f08.

This patch merges cleanly.

SparkQA · 2015-03-15T19:39:20Z

Test build #28631 has finished for PR 4733 at commit e800f08.

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

AmplabJenkins · 2015-03-15T19:39:24Z

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

ankurdave · 2015-03-16T08:05:20Z

LGTM, thanks @brennonyork! Merging into master.

changed method invocation of 'diff' to match that of 'innerJoin' and …

f18356e

…'leftJoin' from VertexRDD[VD] to RDD[(VertexId, VD)]

added back the original diff method to sustain binary compatibility

93186f3

added mima exclude to exclude new public diff method from VertexRDD

2c678c6

Brennon York added 2 commits February 25, 2015 15:10

fixed merge conflicts and set preference to use the diff(other: Verte…

753c963

…xRDD[VD]) method

removed deprecation line

2af0b88

srowen reviewed Mar 14, 2015
View reviewed changes

graphx/src/test/scala/org/apache/spark/graphx/VertexRDDSuite.scala Outdated

Copy link

Member

srowen Mar 14, 2015

Choose a reason for hiding this comment

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

Nit: keep this with other Spark imports

fixed minor include line

f86375c

fixed merge conflicts

b9274af

fixed merge conflicts

e800f08

asfgit closed this in 45f4c66 Mar 16, 2015

srowen mentioned this pull request Mar 20, 2015

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

Closed

[SPARK-5922][GraphX]: Add diff(other: RDD[VertexId, VD]) in VertexRDD #4733

[SPARK-5922][GraphX]: Add diff(other: RDD[VertexId, VD]) in VertexRDD #4733

Uh oh!

Conversation

brennonyork commented Feb 23, 2015

Uh oh!

brennonyork commented Feb 23, 2015

Uh oh!

AmplabJenkins commented Feb 23, 2015

Uh oh!

srowen commented Feb 23, 2015

Uh oh!

SparkQA commented Feb 23, 2015

Uh oh!

SparkQA commented Feb 23, 2015

Uh oh!

AmplabJenkins commented Feb 23, 2015

Uh oh!

brennonyork commented Feb 23, 2015

Uh oh!

srowen commented Feb 24, 2015

Uh oh!

ankurdave commented Feb 24, 2015

Uh oh!

SparkQA commented Feb 24, 2015

Uh oh!

SparkQA commented Feb 24, 2015

Uh oh!

AmplabJenkins commented Feb 24, 2015

Uh oh!

brennonyork commented Feb 24, 2015

Uh oh!

SparkQA commented Feb 24, 2015

Uh oh!

SparkQA commented Feb 24, 2015

Uh oh!

AmplabJenkins commented Feb 24, 2015

Uh oh!

brennonyork commented Feb 24, 2015

Uh oh!

srowen commented Feb 24, 2015

Uh oh!

brennonyork commented Feb 25, 2015

Uh oh!

SparkQA commented Feb 25, 2015

Uh oh!

SparkQA commented Feb 25, 2015

Uh oh!

AmplabJenkins commented Feb 25, 2015

Uh oh!

ankurdave commented Feb 25, 2015

Uh oh!

brennonyork commented Feb 25, 2015

Uh oh!

ankurdave commented Feb 25, 2015

Uh oh!

AmplabJenkins commented Mar 13, 2015

Uh oh!

brennonyork commented Mar 13, 2015

Uh oh!

SparkQA commented Mar 13, 2015

Uh oh!

SparkQA commented Mar 14, 2015

Uh oh!

AmplabJenkins commented Mar 14, 2015

Uh oh!

SparkQA commented Mar 14, 2015

Uh oh!

SparkQA commented Mar 14, 2015

Uh oh!

AmplabJenkins commented Mar 14, 2015

Uh oh!

srowen Mar 14, 2015

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 14, 2015

Uh oh!

SparkQA commented Mar 14, 2015

Uh oh!

AmplabJenkins commented Mar 14, 2015