Skip to content

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

Closed
maropu wants to merge 7 commits into
apache:masterfrom
maropu:RemoveDeprecateInPregel
Closed

[SPARK-12995][GraphX] Remove deprecate APIs from Pregel#10918
maropu wants to merge 7 commits into
apache:masterfrom
maropu:RemoveDeprecateInPregel

Conversation

@maropu

Copy link
Copy Markdown
Member

No description provided.

@maropu

Copy link
Copy Markdown
MemberAuthor

@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
Copy Markdown
Member

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

Copy link
Copy Markdown
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
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Fixed.

@srowen

Copy link
Copy Markdown
Member

Jenkins, retest this please

@maropu

Copy link
Copy Markdown
MemberAuthor

Ah, yes. I removed runSVDPlusPlus in this pr.

@SparkQA

Copy link
Copy Markdown

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
Copy Markdown
Member

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

@maropu

Copy link
Copy Markdown
MemberAuthor

yeah, I'm working on it.

@SparkQA

Copy link
Copy Markdown

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
Copy Markdown
MemberAuthor

@srowen Finished.

@srowen

Copy link
Copy Markdown
Member

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

@maropu

Copy link
Copy Markdown
MemberAuthor

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

@maropu

Copy link
Copy Markdown
MemberAuthor

@ankurdave@srowen ping

@maropu

Copy link
Copy Markdown
MemberAuthor

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

@srowen

Copy link
Copy Markdown
Member

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
Copy Markdown
MemberAuthor

okay.

@srowen

Copy link
Copy Markdown
Member

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
maropuforce-pushed the RemoveDeprecateInPregel branch from f729d67 to 12d368aCompareFebruary 7, 2016 14:56
@maropu

Copy link
Copy Markdown
MemberAuthor

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
Copy Markdown
MemberAuthor

The position of the mima entry fixed.

@srowen

Copy link
Copy Markdown
Member

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
Copy Markdown
MemberAuthor

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
Copy Markdown

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
Copy Markdown
MemberAuthor

Jenkins, retest this please.

@SparkQA

Copy link
Copy Markdown

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
Copy Markdown
Member

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

@maropu
maropuforce-pushed the RemoveDeprecateInPregel branch from 12d368a to 8cce85fCompareFebruary 14, 2016 14:35
@maropu
maropuforce-pushed the RemoveDeprecateInPregel branch from b6d2870 to 0e301ddCompareFebruary 14, 2016 15:09
@maropu

Copy link
Copy Markdown
MemberAuthor

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

@SparkQA

Copy link
Copy Markdown

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
Copy Markdown

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
Copy Markdown

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
maropuforce-pushed the RemoveDeprecateInPregel branch from a1609ed to 24b7580CompareFebruary 14, 2016 22:16
@SparkQA

Copy link
Copy Markdown

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
maropuforce-pushed the RemoveDeprecateInPregel branch from 24b7580 to 7d1452dCompareFebruary 14, 2016 22:47
@SparkQA

Copy link
Copy Markdown

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
Copy Markdown
MemberAuthor

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

@srowen

Copy link
Copy Markdown
Member

Merged to master

@maropu
maropu deleted the RemoveDeprecateInPregel branch July 5, 2017 11:43
Sign up for freeto 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.

3 participants

@maropu@srowen@SparkQA