Skip to content

[SPARK-14679] [UI] Fix UI DAG visualization OOM. - #12437

Closed
rdblue wants to merge 2 commits into
apache:masterfrom
rdblue:SPARK-14679-fix-ui-oom
Closed

[SPARK-14679] [UI] Fix UI DAG visualization OOM.#12437
rdblue wants to merge 2 commits into
apache:masterfrom
rdblue:SPARK-14679-fix-ui-oom

Conversation

@rdblue

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

The DAG visualization can cause an OOM when generating the DOT file.
This happens because clusters are not correctly deduped by a contains
check because they use the default equals implementation. This adds a
working equals implementation.

How was this patch tested?

This adds a test suite that checks the new equals implementation.

The DAG visualization can cause an OOM when generating the DOT file.
This happens because clusters are not correctly deduped by a contains
check because they use the default equals implementation.
@rdbluerdblue changed the title SPARK-14679: Fix UI DAG visualization OOM.[SPARK-14679] [UI] Fix UI DAG visualization OOM.Apr 16, 2016
@SparkQA

Copy link
Copy Markdown

Test build #55986 has finished for PR 12437 at commit e0bda13.

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


override def equals(other: Any): Boolean = other match {
case that: RDDOperationCluster =>
(that canEqual this) &&

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.

Do we need canEqual since that is already known to be a RDDOperationCluster?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This check ensures that equality is symmetric if subclasses have a different definition of equals. As I understand scala best practices, it should be there if the class can be extended. Right now this isn't extended, but I thought it may be in the future so I included it. I'm fine either way.

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.

OK this is another way or saying "this.getClass == that.getClass", essentially? I think it is OK.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

It's similar, but it's calling a method on the other object to see if it would reject equality. There's a good explanation of canEqual on StackOverflow.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The only benefit you get with canEqual over this.getClass == that.getClass is that it's a little bit less strict.
See https://www.artima.com/lejava/articles/equality.html (look for "getClass" in the text to avoid reading the whole thing)

@SaintBacchus

Copy link
Copy Markdown
Contributor

@rdblue Can this PR fix the case like this:

2016-02-2415:40:20,260 | ERROR | [qtp1927776715-4120] | Failedtomakedotfileofstage619 | org.apache.spark.Logging$class.logError(Logging.scala:96)
java.lang.OutOfMemoryError: RequestedarraysizeexceedsVMlimitatjava.util.Arrays.copyOf(Arrays.java:3332)
atjava.lang.AbstractStringBuilder.expandCapacity(AbstractStringBuilder.java:137)

@SparkQA

Copy link
Copy Markdown

Test build #56123 has finished for PR 12437 at commit 9764b43.

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

@rdblue

Copy link
Copy Markdown
ContributorAuthor

@SaintBacchus, I think that error message is essentially the same as that we were seeing, it's just hitting a different limitation when expanding:
71b87a58-f567-4e3e-bc92-9698cb5ca739

@rdblue
rdblueforce-pushed the SPARK-14679-fix-ui-oom branch from 9764b43 to f05edc1CompareApril 18, 2016 19:56
@SparkQA

Copy link
Copy Markdown

Test build #56125 has finished for PR 12437 at commit f05edc1.

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

@rdblue

Copy link
Copy Markdown
ContributorAuthor

The test failure looks unrelated to my changes.

@SparkQA

Copy link
Copy Markdown

Test build #2828 has finished for PR 12437 at commit f05edc1.

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

@srowen

Copy link
Copy Markdown
Member

Merged to master/1.6

asfgit pushed a commit that referenced this pull request Apr 20, 2016
## What changes were proposed in this pull request?
The DAG visualization can cause an OOM when generating the DOT file.
This happens because clusters are not correctly deduped by a contains
check because they use the default equals implementation. This adds a
working equals implementation.
## How was this patch tested?
This adds a test suite that checks the new equals implementation.
Author: Ryan Blue <blue@apache.org>
Closes#12437 from rdblue/SPARK-14679-fix-ui-oom.
(cherry picked from commit a345111)
Signed-off-by: Sean Owen <sowen@cloudera.com>
@rdblue

Copy link
Copy Markdown
ContributorAuthor

Thank you @srowen!

zzcclp pushed a commit to zzcclp/spark that referenced this pull request Apr 21, 2016
## What changes were proposed in this pull request?
The DAG visualization can cause an OOM when generating the DOT file.
This happens because clusters are not correctly deduped by a contains
check because they use the default equals implementation. This adds a
working equals implementation.
## How was this patch tested?
This adds a test suite that checks the new equals implementation.
Author: Ryan Blue <blue@apache.org>
Closesapache#12437 from rdblue/SPARK-14679-fix-ui-oom.
(cherry picked from commit a345111)
Signed-off-by: Sean Owen <sowen@cloudera.com>
(cherry picked from commit 17b1384)
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.

6 participants

@rdblue@SparkQA@SaintBacchus@srowen@joan38@hvanhovell