[SPARK-22713][CORE] ExternalAppendOnlyMap leaks when spilled during iteration by eyalfa · Pull Request #21369 · apache/spark

eyalfa · 2018-05-19T05:11:02Z

What changes were proposed in this pull request?

This PR solves SPARK-22713 which describes a memory leak that occurs when and ExternalAppendOnlyMap is spilled during iteration (opposed to insertion).

(Please fill in changes proposed in this fix)
ExternalAppendOnlyMap's iterator supports spilling but it kept a reference to the internal map (via an internal iterator) after spilling, it seems that the original code was actually supposed to 'get rid' of this reference on the next iteration but according to the elaborate investigation described in the JIRA this didn't happen.
the fix was simply replacing the internal iterator immediately after spilling.

How was this patch tested?

I've introduced a new test to test suite ExternalAppendOnlyMapSuite, this test asserts that neither the external map itself nor its iterator hold any reference to the internal map after a spill.
These approach required some access relaxation of some members variables and nested classes of ExternalAppendOnlyMap, this members are now package provate and annotated with @VisibleForTesting.

…removing the reference to the initial iterator.

SparkQA · 2018-05-19T05:14:34Z

Test build #90829 has finished for PR 21369 at commit 82591e6.

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

eyalfa · 2018-05-19T05:16:03Z

@lianhuiwang, @davies, @hvanhovell can you please have a look?

…sues.

SparkQA · 2018-05-19T05:34:47Z

Test build #90830 has finished for PR 21369 at commit 72f6386.

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

… issues.

SparkQA · 2018-05-19T07:05:01Z

Test build #90831 has finished for PR 21369 at commit 536a769.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

eyalfa · 2018-05-19T07:17:10Z

retest this please

SparkQA · 2018-05-19T11:32:33Z

Test build #90832 has finished for PR 21369 at commit 536a769.

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

Ngone51 · 2018-05-19T16:04:40Z

core/src/main/scala/org/apache/spark/util/collection/ExternalAppendOnlyMap.scala

+        val nextUpstream = spillMemoryIteratorToDisk(upstream)
+        assert(!upstream.hasNext)
        hasSpilled = true
+        upstream = nextUpstream


Does the change means we should reassign upstream (which eliminates reference to currentMap) after spill immediately, otherwise, we may hit OOM (e.g. never readNext() after spill - is this the real cause for JIRA issue?) ?

Basically yes, according to my understanding of the code this should have happened on the subsequent hasNext/next call. However according to the analysis in the jira the iterator kept holding this reference, my guess: at this point the entire program started suffering lengthy GC pauses that got it into behaving as if under a deadlock,effectively leaving the ref in place (just a guess)

Thanks for fixing this issue. I think the potential solution is to change the upstream reference, but I have not tested if this change is sufficient and safe.

@JerryLead, I'd appreciate if you could test this.
One more thing that bugs me is that there's another case when the iterator no longer needs the upstream iterator/underlying map but still holds a reference to it:
When the iterator reach EOF its hasNext method returns false which causes the wrapping CompletionIterator to call the cleanup function which simply nulls the underlying map member variable in ExternalAppendOnlyMap, the iterator member is not nulled out so we end up with the CompletionIterator holding two paths to the upstrean iterator which leads to the underlying map: first it still holds a reference to the iterator itself, however it still holds a reference to the cleanup closure which refers the ExternalAppendOnlyMap which still refers to the current iterator which refers upstream...
This can be solven in one of two ways:
Simple way, when creating the completion iterator, provide a closure referring the iterator, not the ExternalAppendOnlyMap.
Thorough way, modify completion iterator to null out references after cleaning up.

Having that said, I'm not sure how long a completed iterator may be 'sitting' before being discarded so I'm not sure if this is worth fixing, especially using the thorough approach.

destroy re-assigns upstream, once destroy is called, we should be fine?

@cloud-fan , the assumption here is that there are two references to the underlying map: the upstream iterator and the external map itself.
the destroy method first removes the ref via upstream-iterator than delegates to the method that clears the ref via the external map member (currentMap I think), so unless we've missed another ref we should be fine.

as I wrote above, I think there's a potentially more fundamental issue with CompletionIterator which keeps holding references via it's sub and completionFunction members , these might stall some objects from being collected and can be eliminated upon exhaustion of the iterator. there might be some more 'candidates' like LazyIterator and InterruptibleIterator, I think this desrves some more investigation.

@cloud-fan , do you think this is worth doing, I'm referring to the CompletionIterator delaying GC of the sub iterator and cleanup function (usually a closure referring to a larger collection).
if so, I'd open a separate JIRA+PR for this.

Ngone51 · 2018-05-19T16:05:28Z

cc @JerryLead

eyalfa · 2018-05-20T07:37:33Z

well, I took the time trying to figure out how's the iterator is eventually being used,
(most of) it boils down to org.apache.spark.scheduler.ShuffleMapTask#runTask which does:
writer.write(rdd.iterator(partition, context).asInstanceOf[Iterator[_ <: Product2[Any, Any]]])
looking at org.apache.spark.shuffle.ShuffleWriter#write implementations, it seems all of them first exhaust the iterator and then perform some kind of postprocessing: i.e. merging spills, sorting, writing partitions files and then concatanating them into a single file... bottom line the Iterator may actually be 'sitting' for some time after reaching EOF.
I'll implement the 'simple approach' for this PR, but I think this deserves a separate JIRA issue + PR.

…showing that an exhausted iterator still reffers the underlying map.

…SpillableIterator.toCompletionIterator and use that instead of 'maually' creating the completion iterator. also introduced SpillableIterator.destoy which removes the reference to the upstream iterator and calls freeCurrentMap().

SparkQA · 2018-05-20T12:45:48Z

Test build #90854 has finished for PR 21369 at commit d5ee172.

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

eyalfa · 2018-05-20T13:48:48Z

retest this please

SparkQA · 2018-05-20T18:06:12Z

Test build #90861 has finished for PR 21369 at commit d5ee172.

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

advancedxy

The change generally looks good to me except some style issues.
And we should make sure this change indeed fixes the memory leak, so appreciated if @JerryLead could verify it.

advancedxy · 2018-05-22T03:14:07Z

core/src/main/scala/org/apache/spark/util/collection/ExternalAppendOnlyMap.scala

-    private val sortedMap = CompletionIterator[(K, C), Iterator[(K, C)]](destructiveIterator(
-      currentMap.destructiveSortedIterator(keyComparator)), freeCurrentMap())
+    private val sortedMap = destructiveIterator(
+      currentMap.destructiveSortedIterator(keyComparator))


These two lines can be merged into one line?

unfortunately no, scala-style enforces a max of 100 chars per line

advancedxy · 2018-05-22T03:14:36Z

core/src/main/scala/org/apache/spark/util/collection/ExternalAppendOnlyMap.scala

      }
    }

+    def destroy() : Unit = {


Should be private as freeCurrentMap

yes, fixing

advancedxy · 2018-05-22T03:16:06Z

core/src/main/scala/org/apache/spark/util/collection/ExternalAppendOnlyMap.scala

+      upstream = Iterator.empty
+    }
+
+    def toCompletionIterator: CompletionIterator[(K, C), SpillableIterator] = {


I'd prefer private for this method

advancedxy · 2018-05-22T03:16:31Z

core/src/main/scala/org/apache/spark/util/collection/ExternalAppendOnlyMap.scala


  /**
-   * A comparator which sorts arbitrary keys based on their hash codes.
+   * A comparator which sorts arbitrary keys bas on their hash codes.


typo?
bas -> based.

advancedxy · 2018-05-22T03:16:56Z

core/src/test/scala/org/apache/spark/util/collection/ExternalAppendOnlyMapSuite.scala

+import org.apache.spark.util.CompletionIterator

-class ExternalAppendOnlyMapSuite extends SparkFunSuite with LocalSparkContext {
+class ExternalAppendOnlyMapSuite extends SparkFunSuite with LocalSparkContext{


Note the space. LocalSparkContext {

JerryLead · 2018-05-22T06:30:45Z

Thank you all for fixing this issue. I'm sorry that I'm now writing a research paper about Spark GC and going to submit this paper next month. Since there is a lot of paper work to do, the verification may be performed next month.

@advancedxy

@advancedxy

SparkQA · 2018-05-22T11:40:12Z

Test build #90943 has finished for PR 21369 at commit 1d1ddce.

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

eyalfa · 2018-05-22T12:36:37Z

@advancedxy , using jvisualvm+heap dump I could see that the second introduced test case ("drop all references to the underlying map once the iterator is exhausted") eliminated all references to the underlying map:
heap contained one instance of SizeTrackingAppendOnlyMap, but all references to it where unreachable, hence it was due to be evicted.
found one instance of CompletionIterator (actually anonymous class deriving it) which had references to the SpillableIterator, direct ref as member 'sub' and another one via member completionFunction$1.
the SpillableIterator had a single ref to the ExternalAppendOnlyMap which already had its currentMap field already nullified.

Ngone51 · 2018-05-22T13:06:17Z

core/src/main/scala/org/apache/spark/util/collection/ExternalAppendOnlyMap.scala

  def destructiveIterator(inMemoryIterator: Iterator[(K, C)]): Iterator[(K, C)] = {
    readingIterator = new SpillableIterator(inMemoryIterator)
-    readingIterator
+    readingIterator.toCompletionIterator


This change the original behavior of destructiveIterator . I'd prefer do like this:

CompletionIterator[(K, C), Iterator[(K, C)]]( destructiveIterator(currentMap.iterator), readingIterator.destroy)

which keep compatibility with current code, and do not introduce unnecessary function.

What behavior does it change? Your suggested codes does exactly the same but is less streamlined and relies on an intermediate value (fortunately it's already a member variable)

destructiveIterator should just return a destructive iterator (especially for map buffer) as it's function name implies, and it is none business of CompletionIterator . And developers should be free to define the complete function for the returned destructive iterator, in case of we want a different one somewhere else in future.

Your suggested codes does exactly the same but is less streamlined

I don't think this little change will pay a huge influence on streamlined .

and relies on an intermediate value (fortunately it's already a member variable)

The current fix leads to this, not me. And even this variable is not a member variable, we can define a temp local variable. It's not a big deal.

Ngone51 · 2018-05-22T13:06:38Z

core/src/main/scala/org/apache/spark/util/collection/ExternalAppendOnlyMap.scala


+    private def destroy() : Unit = {
+      freeCurrentMap()
+      upstream = Iterator.empty


Why empy, not null?

Safer, class remains usable if for some reason hasNext is called again, and this costs absolutely nothing.

Ngone51 · 2018-05-22T13:12:37Z

core/src/test/scala/org/apache/spark/util/collection/ExternalAppendOnlyMapSuite.scala

    sc.stop()
  }

+  test("spill during iteration") {


I understand what this test want to do. But it seems code without this PR could also pass it if everything goes normally. And I know it's a little hard to reflect the change by unit test. So, I'd prefer to leave some comments to explain the potential memory leak in source code above.

This test was written BEFORE the actual fix and it did fail up untill the fix was in place. I do agree it's a bit clumsy and potential future changes may break the original intention of the test. I've referred a potential testing approach (currently limited to scala's source code) which couldn't be (easily) applied to this code base so I made a best effort to test this.
I agree this needs better documentation, I'll start be referring the issue in the test's name and will also add comments to the code.

advancedxy · 2018-05-22T16:14:46Z

@advancedxy , using jvisualvm+heap dump I could see that the second introduced test case ("drop all references to the underlying map once the iterator is exhausted") eliminated all references to the underlying map:
heap contained one instance of SizeTrackingAppendOnlyMap, but all references to it where unreachable, hence it was due to be evicted.
found one instance of CompletionIterator (actually anonymous class deriving it) which had references to the SpillableIterator, direct ref as member 'sub' and another one via member completionFunction$1.
the SpillableIterator had a single ref to the ExternalAppendOnlyMap which already had its currentMap field already nullified.

Thanks. cc @cloud-fan for final review

cloud-fan · 2018-05-23T14:53:18Z

core/src/test/scala/org/apache/spark/util/collection/ExternalAppendOnlyMapSuite.scala

+    assert(keys == (0 until 100))
+  }
+
+  test("drop all references to the underlying map once the iterator is exhausted") {


let's also put the jira number in the test name.

cloud-fan · 2018-05-23T14:55:48Z

core/src/main/scala/org/apache/spark/util/collection/ExternalAppendOnlyMap.scala

+      upstream = Iterator.empty
+    }
+
+    private[ExternalAppendOnlyMap]


it's pretty reasonable to have this method public.

hmm... the class itself is private (slightly relaxed to package private to ease testing) so I'm not sure what's the benefit in making the method public,
in any case I think that once we see the use case for making this method public we'd probably has to further change the iterator/external map classes.

It's weird to see a class private method. I'd suggest just remove private[ExternalAppendOnlyMap]. spill is only called in ExternalAppendOnlyMap and it's public.

cloud-fan · 2018-05-23T15:07:14Z

core/src/main/scala/org/apache/spark/util/collection/ExternalAppendOnlyMap.scala

+        val nextUpstream = spillMemoryIteratorToDisk(upstream)
+        assert(!upstream.hasNext)
        hasSpilled = true
+        upstream = nextUpstream


destroy re-assigns upstream, once destroy is called, we should be fine?

cloud-fan · 2018-05-23T15:08:04Z

core/src/test/scala/org/apache/spark/util/collection/ExternalAppendOnlyMapSuite.scala

+    assert(map.numSpills == 0, "map was not supposed to spill")
+
+    val it = map.iterator
+    assert( it.isInstanceOf[CompletionIterator[_, _]])


nit: no space after assert(

cloud-fan · 2018-05-23T15:12:42Z

core/src/test/scala/org/apache/spark/util/collection/ExternalAppendOnlyMapSuite.scala

+    assert(map.currentMap == null)
+    assert(underlyingIt.upstream ne underlyingMapIterator)
+    assert(underlyingIt.upstream.getClass != underlyingMapIteratorClass)
+    assert(underlyingIt.upstream.getClass.getEnclosingClass != classOf[AppendOnlyMap[_, _]])


we want to prove we are no longer holding the reference, why do we check type here?

the underlying map's iterator is an anonymous class, this is the best I could come up with to check if the upstream iterator holds a ref to the underlying map.
@cloud-fan , do you have a better idea (I'm not 100% happy with this one)

can we simply check assert(underlyingIt.upstream eq Iterator.empty)?

hmm, we can in line 508 but not in this test.
in this test we look at the iterator immediately after a spill, at this point upstream is supposed to be replaced by a DiskMapIterator, I guess we can check for this directly (after relaxing its visibility to package private).

in line 508, we can simply compare with Iterator.empty

cloud-fan · 2018-05-23T15:14:09Z

cc @JoshRosen

@cloud-fan

@cloud-fan

SparkQA · 2018-05-24T00:15:48Z

Test build #91065 has finished for PR 21369 at commit e3c61fd.

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

…write the first test using WeakReference.

…ive_spill__weak_ref_test

…iled getting the weak ref testing based approach to work

hvanhovell · 2018-08-10T12:31:36Z

retest this please

SparkQA · 2018-08-10T16:22:05Z

Test build #94559 has finished for PR 21369 at commit 621bd23.

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

…e wek ref to assert the map is no longer reachable.

…x tests to effectively test for non-reachabillity of the internal map.

…o SPARK-22713__ExternalAppendOnlyMap_effective_spill__weak_ref_test

…x after merger.

…k_ref_test' into SPARK-22713__ExternalAppendOnlyMap_effective_spill

eyalfa · 2018-08-11T19:53:51Z

retest this please

…ve_spill

eyalfa · 2018-08-11T20:15:40Z

@hvanhovell ,thanks for picking this up 😎

SparkQA · 2018-08-11T23:39:02Z

Test build #94626 has finished for PR 21369 at commit 1bfca67.

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

SparkQA · 2018-08-12T00:32:25Z

Test build #94627 has finished for PR 21369 at commit 1bfca67.

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

SparkQA · 2018-08-12T00:57:47Z

Test build #94629 has finished for PR 21369 at commit 25be99b.

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

cloud-fan · 2018-08-13T02:23:39Z

core/src/main/scala/org/apache/spark/util/collection/ExternalAppendOnlyMap.scala

+  /**
+   * Exposed for testing
+   */
+  @volatile private[collection] var readingIterator: SpillableIterator = null


This is not exposed in the test.

cloud-fan · 2018-08-13T02:24:03Z

core/src/main/scala/org/apache/spark/util/collection/ExternalAppendOnlyMap.scala

+  /**
+   * Exposed for testing
+   */
+  private[collection] class SpillableIterator(var upstream: Iterator[(K, C)])


cloud-fan · 2018-08-13T02:24:42Z

core/src/test/scala/org/apache/spark/util/collection/ExternalAppendOnlyMapSuite.scala

+    // https://github.com/scala/scala/blob/2.13.x/test/junit/scala/tools/testing/AssertUtil.scala
+    // (lines 69-89)
+    // assert(map.currentMap == null)
+    eventually{


nit: add a space eventually {

cloud-fan · 2018-08-13T02:25:15Z

core/src/main/scala/org/apache/spark/util/collection/ExternalAppendOnlyMap.scala

      }
    }

+    private def destroy() : Unit = {


nit: no space before :

cloud-fan · 2018-08-13T02:25:32Z

core/src/main/scala/org/apache/spark/util/collection/ExternalAppendOnlyMap.scala

+    }
+
+    def toCompletionIterator: CompletionIterator[(K, C), SpillableIterator] = {
+      CompletionIterator[(K, C), SpillableIterator](this, this.destroy )


nit: no space before )

cloud-fan · 2018-08-13T02:25:45Z

core/src/test/scala/org/apache/spark/util/collection/ExternalAppendOnlyMapSuite.scala

+    // (lines 69-89)
+    assert(map.currentMap == null)
+
+    eventually{


cloud-fan · 2018-08-13T02:26:42Z

LGTM except some code style issues. Thanks for improving the test!

@cloud-fan

…ssues commented by @cloud-fan .

SparkQA · 2018-08-13T07:05:02Z

Test build #94670 has finished for PR 21369 at commit 855854a.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

eyalfa · 2018-08-13T07:30:42Z

retest this please

SparkQA · 2018-08-13T12:12:36Z

Test build #94678 has finished for PR 21369 at commit 855854a.

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

cloud-fan · 2018-08-13T12:56:54Z

thanks, merging to master!

…teration This PR solves [SPARK-22713](https://issues.apache.org/jira/browse/SPARK-22713) which describes a memory leak that occurs when and ExternalAppendOnlyMap is spilled during iteration (opposed to insertion). (Please fill in changes proposed in this fix) ExternalAppendOnlyMap's iterator supports spilling but it kept a reference to the internal map (via an internal iterator) after spilling, it seems that the original code was actually supposed to 'get rid' of this reference on the next iteration but according to the elaborate investigation described in the JIRA this didn't happen. the fix was simply replacing the internal iterator immediately after spilling. I've introduced a new test to test suite ExternalAppendOnlyMapSuite, this test asserts that neither the external map itself nor its iterator hold any reference to the internal map after a spill. These approach required some access relaxation of some members variables and nested classes of ExternalAppendOnlyMap, this members are now package provate and annotated with VisibleForTesting. Closes apache#21369 from eyalfa/SPARK-22713__ExternalAppendOnlyMap_effective_spill. Authored-by: Eyal Farago <eyal@nrgene.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry-picked from commit 2e3abdf) Ref: LIHADOOP-40170 RB=1413724 BUG=LIHADOOP-40170 G=superfriends-reviewers R=fli,mshen,yezhou,edlu A=yezhou

Eyal Farago added 2 commits May 19, 2018 01:20

SPARK-22713__ExternalAppendOnlyMap_effective_spill: add failing test.

1c4a6af

SPARK-22713__ExternalAppendOnlyMap_effective_spill: fix the issue by …

82591e6

…removing the reference to the initial iterator.

SPARK-22713__ExternalAppendOnlyMap_effective_spill: fix scalastyle is…

72f6386

…sues.

Eyal Farago added 2 commits May 19, 2018 08:49

SPARK-22713__ExternalAppendOnlyMap_effective_spill: some more styling…

48224d9

… issues.

SPARK-22713__ExternalAppendOnlyMap_effective_spill: fix typo

536a769

Ngone51 reviewed May 19, 2018

View reviewed changes

Eyal Farago added 2 commits May 20, 2018 11:51

SPARK-22713__ExternalAppendOnlyMap_effective_spill: introduce a test …

589b423

…showing that an exhausted iterator still reffers the underlying map.

advancedxy reviewed May 22, 2018

View reviewed changes

SPARK-22713__ExternalAppendOnlyMap_effective_spill: address comments by

1d1ddce

@advancedxy

Ngone51 reviewed May 22, 2018

View reviewed changes

cloud-fan reviewed May 23, 2018

View reviewed changes

SPARK-22713__ExternalAppendOnlyMap_effective_spill: address comments by

e3c61fd

@cloud-fan

Eyal Farago added 3 commits May 27, 2018 14:17

SPARK-22713__ExternalAppendOnlyMap_effective_spill__weak_ref_test: re…

a2e78e2

…write the first test using WeakReference.

Merge commit '589b423' into SPARK-22713__ExternalAppendOnlyMap_effect…

4366eb4

…ive_spill__weak_ref_test

SPARK-22713__ExternalAppendOnlyMap_effective_spill__weak_ref_test: fa…

686b4d9

…iled getting the weak ref testing based approach to work

Eyal Farago added 5 commits August 11, 2018 21:36

SPARK-22713__ExternalAppendOnlyMap_effective_spill__weak_ref_test: us…

4e44585

…e wek ref to assert the map is no longer reachable.

SPARK-22713__ExternalAppendOnlyMap_effective_spill__weak_ref_test: fi…

0cf8913

…x tests to effectively test for non-reachabillity of the internal map.

Merge branch 'SPARK-22713__ExternalAppendOnlyMap_effective_spill' int…

9eb5600

…o SPARK-22713__ExternalAppendOnlyMap_effective_spill__weak_ref_test

SPARK-22713__ExternalAppendOnlyMap_effective_spill__weak_ref_test: fi…

11b5bb4

…x after merger.

Merge branch 'SPARK-22713__ExternalAppendOnlyMap_effective_spill__wea…

1bfca67

…k_ref_test' into SPARK-22713__ExternalAppendOnlyMap_effective_spill

Merge branch 'master' into SPARK-22713__ExternalAppendOnlyMap_effecti…

25be99b

…ve_spill

cloud-fan reviewed Aug 13, 2018

View reviewed changes

SPARK-22713__ExternalAppendOnlyMap_effective_spill: address styling i…

855854a

…ssues commented by @cloud-fan .

asfgit closed this in 2e3abdf Aug 13, 2018

Conversation

eyalfa commented May 19, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented May 19, 2018

Uh oh!

eyalfa commented May 19, 2018

Uh oh!

SparkQA commented May 19, 2018

Uh oh!

SparkQA commented May 19, 2018

Uh oh!

eyalfa commented May 19, 2018

Uh oh!

SparkQA commented May 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JerryLead May 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ngone51 commented May 19, 2018

Uh oh!

eyalfa commented May 20, 2018

Uh oh!

SparkQA commented May 20, 2018

Uh oh!

eyalfa commented May 20, 2018

Uh oh!

SparkQA commented May 20, 2018

Uh oh!

advancedxy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JerryLead commented May 22, 2018

Uh oh!

SparkQA commented May 22, 2018

Uh oh!

eyalfa commented May 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ngone51 May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JerryLead May 20, 2018 •

edited

Loading

Ngone51 May 22, 2018 •

edited

Loading

Ngone51 May 22, 2018 •

edited

Loading