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.

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

[SPARK-12995][GraphX] Remove deprecate APIs from Pregel #10918

Closed
wants to merge 7 commits into from
Closed

[SPARK-12995][GraphX] Remove deprecate APIs from Pregel #10918

wants to merge 7 commits into from

Conversation

maropu
Copy link
Member

@maropu maropu commented Jan 26, 2016

No description provided.

@maropu
Copy link
Member Author

maropu commented Jan 26, 2016

@srowen This is an activity from the discussion in #4402.
I checked that GraphX has deprecate APIs used only in Pregel and this pr removes them.
If there are't any problem, I'll also remove deprecate ones from the test codes in GraphX.

@srowen
Copy link
Member

srowen commented Jan 26, 2016

While we're here we can remove runSVDPlusPlus in SVDPlusPlus?

srowen
srowen reviewed Jan 26, 2016
View reviewed changes
graphx/src/main/scala/org/apache/spark/graphx/Pregel.scala
@@ -109,19 +109,17 @@ object Pregel extends Logging {
* @return the resulting graph at the end of the computation
*
*/
def apply[VD: ClassTag, ED: ClassTag, A: ClassTag]
(graph: Graph[VD, ED],
def apply[VD: ClassTag, ED: ClassTag, A: ClassTag](
Copy link
Member

Choose a reason for hiding this comment

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

Although it's really minor, the paren placement now makes it hard to see that there are two arg lists. I think it was better as-is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@srowen
Copy link
Member

srowen commented Jan 26, 2016

Jenkins, retest this please

@maropu
Copy link
Member Author

maropu commented Jan 26, 2016

Ah, yes. I removed runSVDPlusPlus in this pr.

@SparkQA
Copy link

SparkQA commented Jan 26, 2016

Test build #50097 has finished for PR 10918 at commit 346ebdc.

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

@srowen
Copy link
Member

srowen commented Jan 26, 2016

@maropu yeah I think you'll need to write MiMa exclusions for the removal, to confirm they are intended.

@maropu
Copy link
Member Author

maropu commented Jan 26, 2016

yeah, I'm working on it.

@SparkQA
Copy link

SparkQA commented Jan 26, 2016

Test build #50101 has finished for PR 10918 at commit 15937eb.

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

@maropu
Copy link
Member Author

maropu commented Jan 26, 2016

@srowen Finished.

@srowen
Copy link
Member

srowen commented Jan 26, 2016

Looks OK to me, though I'lll want to leave it for review for a while. CC @ankurdave if possible

@maropu
Copy link
Member Author

maropu commented Jan 26, 2016

Okay. If this pr merged, I'll fix test codes in graphx to remove deprecate Graph#mapReduceTriplets.

@maropu
Copy link
Member Author

maropu commented Jan 28, 2016

@ankurdave @srowen ping

@maropu
Copy link
Member Author

maropu commented Feb 2, 2016

@srowen @ankurdave almost inactive, so what should I do?

@srowen
Copy link
Member

srowen commented Feb 2, 2016

I'm out this week on vacation but definitely intend to proceed next week. I was leaving it for a while in case ankur is around

@maropu
Copy link
Member Author

maropu commented Feb 2, 2016

okay.

@srowen
Copy link
Member

srowen commented Feb 6, 2016

I'm back now. I'm almost ready to merge, but I noticed: should this not also remove mapReduceTriplets? Also the MiMa exclusions are correct but seem to be in the wrong place; at least, it might be clearer to make a new Seq section containing the exclusions just for this issue.

@maropu maropu force-pushed the RemoveDeprecateInPregel branch from f729d67 to 12d368a Compare February 7, 2016 14:56
@maropu
Copy link
Member Author

maropu commented Feb 7, 2016

Welcome back. We cannot remove mapReduceTripelets because some tests in graphx still use it.
So, I'll make another pr to fix the tests, then remove it.

@maropu
Copy link
Member Author

maropu commented Feb 7, 2016

The position of the mima entry fixed.

@srowen
Copy link
Member

srowen commented Feb 7, 2016

If the only usage is tests, then the tests should be removed too, if they're just there to test this deprecated method. If it's found in other tests, can you migrate that code accordingly?

@maropu
Copy link
Member Author

maropu commented Feb 7, 2016

Ah, we cannot simply remove some tests because these tests target other tests for graph processing.
So, we need to replace mapReduceTriplets with newer aggregateMessage.
Yes, I can fix them.

@SparkQA
Copy link

SparkQA commented Feb 7, 2016

Test build #50902 has finished for PR 10918 at commit 12d368a.

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

@maropu
Copy link
Member Author

maropu commented Feb 9, 2016

Jenkins, retest this please.

@SparkQA
Copy link

SparkQA commented Feb 9, 2016

Test build #50967 has finished for PR 10918 at commit 12d368a.

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

@srowen
Copy link
Member

srowen commented Feb 14, 2016

@maropu I'm ready to merge this, but it needs a rebase, and I think this should be completed by removing mapReduceTriplets entirely

@maropu maropu force-pushed the RemoveDeprecateInPregel branch from 12d368a to 8cce85f Compare February 14, 2016 14:35
@maropu maropu force-pushed the RemoveDeprecateInPregel branch from b6d2870 to 0e301dd Compare February 14, 2016 15:09
@maropu
Copy link
Member Author

maropu commented Feb 14, 2016

@srowen okay, I removed `mapReduceTriplets' in this pr.

@SparkQA
Copy link

SparkQA commented Feb 14, 2016

Test build #51271 has finished for PR 10918 at commit 0e301dd.

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

@SparkQA
Copy link

SparkQA commented Feb 14, 2016

Test build #51270 has finished for PR 10918 at commit 8cce85f.

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

@SparkQA
Copy link

SparkQA commented Feb 14, 2016

Test build #51279 has finished for PR 10918 at commit a1609ed.

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

@maropu maropu force-pushed the RemoveDeprecateInPregel branch from a1609ed to 24b7580 Compare February 14, 2016 22:16
@SparkQA
Copy link

SparkQA commented Feb 14, 2016

Test build #51280 has finished for PR 10918 at commit 24b7580.

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

@maropu maropu force-pushed the RemoveDeprecateInPregel branch from 24b7580 to 7d1452d Compare February 14, 2016 22:47
@SparkQA
Copy link

SparkQA commented Feb 15, 2016

Test build #51281 has finished for PR 10918 at commit 7d1452d.

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

@maropu
Copy link
Member Author

maropu commented Feb 15, 2016

@srowen okay, all the tests passed, so plz check this?

@srowen
Copy link
Member

srowen commented Feb 15, 2016

Merged to master

@asfgit asfgit closed this in 56d4939 Feb 15, 2016
@maropu maropu deleted the RemoveDeprecateInPregel branch July 5, 2017 11:43
@dongjoon-hyun dongjoon-hyun mentioned this pull request Oct 20, 2018
Closed
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.
3 participants
@maropu @srowen @SparkQA