[SPARK-26119][CORE][WEBUI]Task summary table should contain only successful tasks' metrics #23088

shahidki31 · 2018-11-19T19:51:07Z

What changes were proposed in this pull request?

Task summary table in the stage page currently displays the summary of all the tasks. However, we should display the task summary of only successful tasks, to follow the behavior of previous versions of spark.

How was this patch tested?

Added UT. attached screenshot
Before patch:

SparkQA · 2018-11-20T00:10:44Z

Test build #99016 has finished for PR 23088 at commit cfe2d5f.

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

shahidki31 · 2018-11-20T05:26:24Z

cc @vanzin @srowen

shahidki31 · 2018-11-20T10:08:44Z

Need to handle more scenarios and optimization, I'm closing this for some moment. Will update the PR.

gengliangwang · 2018-11-20T14:39:52Z

core/src/main/scala/org/apache/spark/status/AppStatusStore.scala

@@ -238,8 +239,16 @@ private[spark] class AppStatusStore(
            val diff = idx - currentIdx


diff could be negative here?

Hi @gengliangwang I will update the PR. Actually there are more changes required to fix this. Thanks

gengliangwang · 2018-11-20T14:42:48Z

As the task section shows all the tasks, I think maybe it would be better to show aggregated metrics for all the tasks.

shahidki31 · 2018-11-20T14:44:58Z

@gengliangwang Previous versions of Spark were showing only completed tasks summary metrics. I think it make more sense to show only finished tasks, rather than showing metrics of killed and failed

srowen · 2018-11-21T15:34:44Z

When evaluating these things I use git blame (or visual tools here and in an IDE) to figure out if there was a previous change that explicitly tried to change the behavior, and whether it was on purpose, wrong, or incomplete. That would really help understand whether it's just a clear bug or something else.

shahidki31 · 2018-11-21T18:47:04Z

@srowen Yes. I did check the commit history, and the PR which modified the behavior didn't mention about this behavior change. If it is intentional, then we should change the title of the table from "summary metrics for completed tasks" to "summary metrics for all tasks".

shahidki31 · 2018-11-21T18:48:35Z

(Still need more things to test, so changed to WIP)

SparkQA · 2018-11-21T22:58:32Z

Test build #99137 has finished for PR 23088 at commit 8f4498e.

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

SparkQA · 2018-11-21T23:11:03Z

Test build #99139 has finished for PR 23088 at commit 96fd281.

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

shahidki31 · 2018-11-25T15:48:45Z

@srowen , could you please review the PR?

srowen

I'm not sure about the code changes, but I don't know this part. It seems to be changing a number of things that aren't just related to filtering for "SUCCESS"ful tasks.

When did the behavior change? you said you found the commit.

srowen · 2018-11-25T19:09:29Z

core/src/main/scala/org/apache/spark/status/AppStatusStore.scala

+        .zipWithIndex
+        .filter(x => indices.contains(x._2))
+
+      if(quantileTasks.size > indices.length) {


The formatting needs adjustment around here: space after 'if', less indent on the next line, no space after (, etc.

srowen · 2018-11-25T19:10:57Z

core/src/main/scala/org/apache/spark/status/AppStatusStore.scala

+        .index(index)
+        .first(0L)
+        .asScala
+        .filter(_.status.startsWith("S")) // Filter "SUCCESS" tasks


Why not check for "SUCCESS"?

srowen · 2018-11-25T19:11:35Z

core/src/main/scala/org/apache/spark/status/AppStatusStore.scala

-            }
-          }
-        }.toIndexedSeq
+      val quantileTasks = store.view(classOf[TaskDataWrapper])


This used to get an iterator. I wonder if it's safe to materialize the whole collection? it seems to take care to use the iterator to skip over things for example.

Yes. That was the original intend. unfortunately after converting to the scala collection, the skip() functionality is not there. Also the kvstore iterator doesn't have any filter API to filter the "success" tasks.

The PR was for reducing the computational time for loading the stagePage from the diskStore ( for history server), by avoiding in memory sorting.

I get it, but, it seems like there was a particular reason for using an iterator and skipping, and I'm not sure we can just undo it. If this is only to filter for "SUCCESS", that much seems easy enough in the original code with an 'if' statement somewhere, no? similar code above still uses an iterator.

Yes. If we do, "if (status == "SUCCESS")" for every iterator value, we can't do the "skip()" function. Becuase, earlier we know the exact index we need to take. ie. we can directly skip to 25th percentile, 50th percentile and so on. Now, we don't know which index has the 25th percentile of the "SUCCESS" value, unless we iterate each.

Otherwise, we have to filter the "SUCCESS" tasks prior, like I have done in the PR.

Hm, I think this reintroduces the problem that this code fixed though. What about adding some kind of filter() method to KVStoreView? it could configure an optional Predicate<T> that is applied to iterator(). Commons Collections has a FilterIterator for this.

@vanzin Yes. It seems, loading the stage page take lot more time if we enable disk store.
InMemory store seems okay. (Also, first time loading the Jobs page from the history server too, taking lots of time. Mostly due to eventLog parsing time.)

Pardon if I'm missing something, but does this avoid the problem? everything is loaded, and then it's filtered.

@srowen Yes. everything is loaded in "sorted" order based on the index, and then we do filtering. For In memory case, this doesn't cause any issue. but for diskStore extra de serialization overhead is there.

May be one possible solution can be, for diskStore case, bring only first time and sort based on the corresponding indices to compute the quantiles.

If the solution seems complicated, then we can tell the user that, summary metrics display the quantile summary for all the tasks, instead of completed ones.

correct me if I am wrong

I see so there's no real way to do it right without deserializing everything because that's the only way to know for sure what's successful.

Leaving it as-is isn't so bad; if failed tasks are infrequent then the quantiles are still about right.

I can think of fudges like searching ahead from the current index for the next successful task and using that. For the common case that might make the quantiles a little better. Iterate ahead, incrementing idx, and give up when hitting the next index.

Hm @vanzin is that too complex vs taking the hit and deserializing it all? or just punt on this?

I'm not sure the fudging would give you much better results. Perhaps a little bit...

As for the correct solution, I don't see a lot of good options.

loading all successful tasks in memory will be a little slow and memory-intensive for large stages

Using my 100k stage example, deserializing everything, even once, will stlil be a little expensive (deserializing 100k tasks vs. deserializing metricCount * quantileCount tasks which is 100 tasks and change in the UI case).

changing the way the indexing works, so that you can index by specific metrics for successful and failed tasks differently, would be tricky, and also would require changing the disk store version (to invalidate old stores).

maybe something else I'm not able to come up with at the moment.

Unless someone has a better idea, maybe just using this code when using an in-memory store is an ok temporary "fix".

srowen · 2018-11-25T19:14:31Z

core/src/main/scala/org/apache/spark/status/AppStatusStore.scala

+          quantileTasks.map(task => fn(task._1).toDouble).toIndexedSeq
+      } else {
+        indices.map( index =>
+         fn(quantileTasks.filter(_._2 == index).head._1).toDouble).toIndexedSeq


.filter + .head = .find? it is more efficient. Why not create a Map though if this needs to be accessed by key?

Modified. Thanks

srowen · 2018-11-25T19:15:35Z

core/src/test/scala/org/apache/spark/status/AppStatusStoreSuite.scala

+  test("summary should contain task metrics of only successfull tasks") {
+    val store = new InMemoryStore()
+    (0 until 5).foreach { i =>
+      if (i % 2 == 1) store.write(failedTaskData(i))


Braces for if-else. I also personally think a for loop is more obvious here: for (i <- 0 until 5) {

srowen · 2018-11-25T19:16:08Z

core/src/test/scala/org/apache/spark/status/AppStatusStoreSuite.scala

+    }
+    val summary = new AppStatusStore(store).taskSummary(stageId, attemptId, uiQuantiles).get
+
+    val values = (0 to 2).map( i =>


2.0 is much clearer than 2.toDouble. But why not just write Array(0.0, 2.0, 4.0)?

shahidki31 · 2018-11-25T19:32:15Z

Thank you @srowen for the review. I will update the PR.

When did the behavior change? you said you found the commit.

https://issues.apache.org/jira/browse/SPARK-20657 After the PR correspond to the JIRA, this behavior change occurred. Also, In the PR discussion hasn't mentioned whether it is intentional or not.

SparkQA · 2018-11-25T19:42:21Z

Test build #99238 has finished for PR 23088 at commit 7edc835.

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

srowen

OK CC @vanzin on the intent of the original change

srowen · 2018-11-25T22:18:58Z

core/src/main/scala/org/apache/spark/status/AppStatusStore.scala

-            }
-          }
-        }.toIndexedSeq
+      val quantileTasks = store.view(classOf[TaskDataWrapper])


I get it, but, it seems like there was a particular reason for using an iterator and skipping, and I'm not sure we can just undo it. If this is only to filter for "SUCCESS", that much seems easy enough in the original code with an 'if' statement somewhere, no? similar code above still uses an iterator.

SparkQA · 2018-11-26T00:06:39Z

Test build #99244 has finished for PR 23088 at commit 6b376af.

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

srowen · 2018-11-26T12:08:33Z

core/src/main/scala/org/apache/spark/status/AppStatusStore.scala

-            }
-          }
-        }.toIndexedSeq
+      val quantileTasks = store.view(classOf[TaskDataWrapper])


Hm, I think this reintroduces the problem that this code fixed though. What about adding some kind of filter() method to KVStoreView? it could configure an optional Predicate<T> that is applied to iterator(). Commons Collections has a FilterIterator for this.

core/src/main/scala/org/apache/spark/status/AppStatusStore.scala

vanzin · 2018-11-26T19:40:15Z

core/src/main/scala/org/apache/spark/status/AppStatusStore.scala

+        .asScala
+        .filter(_.status == "SUCCESS") // Filter "SUCCESS" tasks
+        .zipWithIndex
+        .filter(x => indices.contains(x._2))


style for these is .filter { x => ... } (happens in a bunch of places)

I don't feel strongly about it but I have written it as above where a new block isn't needed. I don't think it makes any runtime difference.

It doesn't make a runtime difference. That's why this is a style nit.

Thanks. I have modified

vanzin · 2018-11-26T19:40:37Z

core/src/test/scala/org/apache/spark/status/AppStatusStoreSuite.scala

      i, i, i, i, i, i, i, i, i, i,
      i, i, i, i, i, i, i, i, i, i,
      i, i, i, i, stageId, attemptId)
  }

+  private def failedTaskData(i: Int): TaskDataWrapper = {


better to have the status be a parameter to the existing method (with a default value)

Thanks. done.

SparkQA · 2018-11-27T00:15:24Z

Test build #99287 has finished for PR 23088 at commit b29a150.

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

SparkQA · 2018-11-27T00:33:32Z

Test build #99289 has finished for PR 23088 at commit c81fcf3.

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

srowen

Looks like the only outstanding question here is whether we can use this code only when there's a memory-backed store.

While it makes some sense I have two concerns: different answers based on disk vs memory store which shouldn't really affect things. But would a user ever have both and see both side by side and be confused? Second is, that seems like it should still entail pushing down all the quantile logic into the KVStore, to be clean, right? and that's a bigger change.

I'd either punt on this part of the change, or design a change that involves pushing this down to the keystore I think?

SparkQA · 2018-12-02T07:13:04Z

Test build #99564 has finished for PR 23088 at commit 7956b27.

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

SparkQA · 2018-12-02T08:05:02Z

Test build #99566 has finished for PR 23088 at commit 2235967.

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

shahidki31 · 2018-12-02T15:11:33Z

Jenkins, retest this please

SparkQA · 2018-12-02T19:05:33Z

Test build #99577 has finished for PR 23088 at commit 2235967.

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

srowen · 2018-12-02T19:18:55Z

core/src/main/scala/org/apache/spark/status/AppStatusStore.scala

-          .index(TaskIndexNames.EXEC_RUN_TIME)
-          .first(0L)
-          .closeableIterator()
+        if (store.isInstanceOf[LevelDB]) {


Does this code path need to be different for disk vs memory? this part seemed like it could work efficiently either way.

Yes. Now, for diskStore case, it finds "total tasks count" and inMemory case only "successful tasks count".

This 'count' is used to find quantileIndices for all the tasks metrics.

spark/core/src/main/scala/org/apache/spark/status/AppStatusStore.scala

Line 222 in 676bbb2

val indices = quantiles.map { q => math.min((q * count).toLong, count - 1) }

For eg: Assume 200 tasks, out of which 100 success and 100 failed.

For diskStore case => indices = [0, 50, 100, 150, 199], count = 200
For InMemory case => indices = [0, 25, 50, 75, 99], count = 100

srowen · 2018-12-02T19:19:23Z

core/src/main/scala/org/apache/spark/status/AppStatusStore.scala

@@ -221,29 +230,49 @@ private[spark] class AppStatusStore(
    // stabilize once the stage finishes. It's also slow, especially with disk stores.
    val indices = quantiles.map { q => math.min((q * count).toLong, count - 1) }

+    // TODO Summary metrics needs to display all the successful tasks' metrics (SPARK-26119).


It's not ideal but it's a reasonable solution. Are you OK with it @vanzin ?

pgandhi999 · 2018-12-03T04:53:03Z

Hi @shahidki31 ,thank you for pointing that out. Will put up a fix for it shortly.

shahidki31 · 2018-12-03T05:48:57Z

Thanks @pgandhi999

pgandhi999 · 2018-12-03T08:31:05Z

PR : #23205 for displaying the message on empty data in task summary metrics table.

vanzin

I think this is ok for now, while the bigger fix is not tackled, Just a couple of minor nits.

vanzin · 2018-12-03T19:33:07Z

core/src/main/scala/org/apache/spark/status/AppStatusStore.scala

-          .index(TaskIndexNames.EXEC_RUN_TIME)
-          .first(0L)
-          .closeableIterator()
+        if (store.isInstanceOf[LevelDB]) {


Could you invert the check so you don't need to import LevelDB? Just to avoid importing more implementation details of the kvstore module into this class...

Done. Thanks

vanzin · 2018-12-03T19:35:01Z

core/src/test/scala/org/apache/spark/status/AppStatusStoreSuite.scala

@@ -77,6 +77,34 @@ class AppStatusStoreSuite extends SparkFunSuite {
    assert(store.count(classOf[CachedQuantile]) === 2)
  }

+  test("only successfull task have taskSummary") {
+    val store = new InMemoryStore()
+    (0 until 5).foreach { i => store.write(newTaskData(i, "FAILED")) }


nit: status = "FAILED" when param has a default value

vanzin · 2018-12-03T19:39:16Z

Also it would be good to open a separate bug to address the fix for SHS / disk store.

shahidki31 · 2018-12-04T00:40:55Z

Hi @vanzin , I have opened a JIRA for disk store case https://issues.apache.org/jira/browse/SPARK-26260. I will try to work on the same.

SparkQA · 2018-12-04T04:04:47Z

Test build #99635 has finished for PR 23088 at commit f8cfb54.

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

shahidki31 · 2018-12-04T04:31:28Z

Retest this please

SparkQA · 2018-12-04T07:52:47Z

Test build #99645 has finished for PR 23088 at commit f8cfb54.

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

SparkQA · 2018-12-04T08:05:02Z

Test build #99653 has finished for PR 23088 at commit 41cfe80.

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

shahidki31 · 2018-12-04T08:09:05Z

retest this please

SparkQA · 2018-12-04T12:32:59Z

Test build #99654 has finished for PR 23088 at commit 41cfe80.

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

vanzin · 2018-12-04T19:00:25Z

Merging to master / 2.4.

…cessful tasks' metrics ## What changes were proposed in this pull request? Task summary table in the stage page currently displays the summary of all the tasks. However, we should display the task summary of only successful tasks, to follow the behavior of previous versions of spark. ## How was this patch tested? Added UT. attached screenshot Before patch: ![screenshot from 2018-11-20 00-36-18](https://user-images.githubusercontent.com/23054875/48729339-62e3a580-ec5d-11e8-81f0-0d191a234ffe.png) ![screenshot from 2018-11-20 01-18-37](https://user-images.githubusercontent.com/23054875/48731112-41d18380-ec62-11e8-8c31-1ffbfa04e746.png) Closes #23088 from shahidki31/summaryMetrics. Authored-by: Shahid <shahidki31@gmail.com> Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com> (cherry picked from commit 35f9163) Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com>

shahidki31 · 2018-12-05T02:40:50Z

Thanks @vanzin @srowen

…cessful tasks' metrics ## What changes were proposed in this pull request? Task summary table in the stage page currently displays the summary of all the tasks. However, we should display the task summary of only successful tasks, to follow the behavior of previous versions of spark. ## How was this patch tested? Added UT. attached screenshot Before patch: ![screenshot from 2018-11-20 00-36-18](https://user-images.githubusercontent.com/23054875/48729339-62e3a580-ec5d-11e8-81f0-0d191a234ffe.png) ![screenshot from 2018-11-20 01-18-37](https://user-images.githubusercontent.com/23054875/48731112-41d18380-ec62-11e8-8c31-1ffbfa04e746.png) Closes apache#23088 from shahidki31/summaryMetrics. Authored-by: Shahid <shahidki31@gmail.com> Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com>

…cessful tasks' metrics ## What changes were proposed in this pull request? Task summary table in the stage page currently displays the summary of all the tasks. However, we should display the task summary of only successful tasks, to follow the behavior of previous versions of spark. ## How was this patch tested? Added UT. attached screenshot Before patch: ![screenshot from 2018-11-20 00-36-18](https://user-images.githubusercontent.com/23054875/48729339-62e3a580-ec5d-11e8-81f0-0d191a234ffe.png) ![screenshot from 2018-11-20 01-18-37](https://user-images.githubusercontent.com/23054875/48731112-41d18380-ec62-11e8-8c31-1ffbfa04e746.png) Closes apache#23088 from shahidki31/summaryMetrics. Authored-by: Shahid <shahidki31@gmail.com> Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com> (cherry picked from commit 35f9163) Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com>

…ly successful tasks summary …sks metrics for disk store ### What changes were proposed in this pull request? After #23088 task Summary table in the stage page shows successful tasks metrics for lnMemory store. In this PR, it added for disk store also. ### Why are the changes needed? Now both InMemory and disk store will be consistent in showing the task summary table in the UI, if there are non successful tasks ### Does this PR introduce any user-facing change? no ### How was this patch tested? Added UT. Manually verified Test steps: 1. add the config in spark-defaults.conf -> **spark.history.store.path /tmp/store** 2. sbin/start-hitoryserver 3. bin/spark-shell 4. `sc.parallelize(1 to 1000, 2).map(x => throw new Exception("fail")).count` ![Screenshot 2019-11-14 at 3 51 39 AM](https://user-images.githubusercontent.com/23054875/68809546-268d2e80-0692-11ea-8b2c-bee767478135.png) Closes #26508 from shahidki31/task. Authored-by: shahid <shahidki31@gmail.com> Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com>

shahidki31 closed this Nov 20, 2018

gengliangwang reviewed Nov 20, 2018

View reviewed changes

shahidki31 reopened this Nov 21, 2018

shahidki31 changed the title ~~[SPARK-26119][CORE][WEBUI]Task summary table should contain only successful tasks' metrics~~ [WIP][SPARK-26119][CORE][WEBUI]Task summary table should contain only successful tasks' metrics Nov 21, 2018

shahidki31 force-pushed the summaryMetrics branch from 8f4498e to 96fd281 Compare November 21, 2018 18:53

shahidki31 changed the title ~~[WIP][SPARK-26119][CORE][WEBUI]Task summary table should contain only successful tasks' metrics~~ [SPARK-26119][CORE][WEBUI]Task summary table should contain only successful tasks' metrics Nov 25, 2018

srowen requested changes Nov 25, 2018

View reviewed changes

srowen requested changes Nov 26, 2018

View reviewed changes

vanzin reviewed Nov 26, 2018

View reviewed changes

shahidki31 force-pushed the summaryMetrics branch from b29a150 to c81fcf3 Compare November 26, 2018 20:17

srowen reviewed Dec 1, 2018

View reviewed changes

shahidki31 force-pushed the summaryMetrics branch from c81fcf3 to dc95355 Compare December 2, 2018 02:33

srowen reviewed Dec 2, 2018

View reviewed changes

vanzin reviewed Dec 3, 2018

View reviewed changes

address comments

f8cfb54

nit

41cfe80

asfgit closed this in 35f9163 Dec 4, 2018

gengliangwang mentioned this pull request Aug 7, 2019

[SPARK-28638][WebUI] Task summary should only contain successful tasks' metrics #25369

Closed

shahidki31 mentioned this pull request Nov 13, 2019

[SPARK-26260][Core]For disk store tasks summary table should show only successful tasks summary #26508

Closed

		@@ -238,8 +239,16 @@ private[spark] class AppStatusStore(
		val diff = idx - currentIdx

[SPARK-26119][CORE][WEBUI]Task summary table should contain only successful tasks' metrics #23088

[SPARK-26119][CORE][WEBUI]Task summary table should contain only successful tasks' metrics #23088

Conversation

shahidki31 commented Nov 19, 2018

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Nov 20, 2018

shahidki31 commented Nov 20, 2018

shahidki31 commented Nov 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gengliangwang commented Nov 20, 2018

shahidki31 commented Nov 20, 2018 • edited Loading

srowen commented Nov 21, 2018

shahidki31 commented Nov 21, 2018 • edited Loading

shahidki31 commented Nov 21, 2018

SparkQA commented Nov 21, 2018

SparkQA commented Nov 21, 2018

shahidki31 commented Nov 25, 2018

srowen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahidki31 Nov 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahidki31 Nov 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahidki31 Nov 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahidki31 Nov 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahidki31 commented Nov 25, 2018

SparkQA commented Nov 25, 2018

srowen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Nov 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vanzin Nov 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Nov 27, 2018

SparkQA commented Nov 27, 2018

srowen left a comment

Choose a reason for hiding this comment

SparkQA commented Dec 2, 2018

SparkQA commented Dec 2, 2018

shahidki31 commented Dec 2, 2018

SparkQA commented Dec 2, 2018

Choose a reason for hiding this comment

shahidki31 Dec 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgandhi999 commented Dec 3, 2018

shahidki31 commented Dec 3, 2018

pgandhi999 commented Dec 3, 2018

vanzin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahidki31 commented Nov 20, 2018 •

edited

Loading

shahidki31 commented Nov 21, 2018 •

edited

Loading

shahidki31 Nov 25, 2018 •

edited

Loading

shahidki31 Nov 26, 2018 •

edited

Loading

shahidki31 Nov 26, 2018 •

edited

Loading

shahidki31 Nov 28, 2018 •

edited

Loading

vanzin Nov 26, 2018 •

edited

Loading

shahidki31 Dec 3, 2018 •

edited

Loading