KAFKA-5358: Consumer perf tool should count rebalance time. by huxihx · Pull Request #3188 · apache/kafka

huxihx · 2017-06-01T09:18:30Z

Added 'join.group.ms' for new consumer to calculate the time of joining group.

@hachikuji Please review the PR. Thanks.

Added 'join.group.ms' for new consumer to calculate the time of joining group.

asfbot · 2017-06-01T10:22:14Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/4700/
Test PASSed (JDK 8 and Scala 2.12).

asfbot · 2017-06-01T10:41:30Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/4716/
Test PASSed (JDK 7 and Scala 2.11).

hachikuji

Thanks for picking this up so quickly. Left a couple comments.

hachikuji · 2017-06-02T02:52:39Z

core/src/main/scala/kafka/tools/ConsumerPerformance.scala

    if (!config.hideHeader) {
      if (!config.showDetailedStats)
-        println("start.time, end.time, data.consumed.in.MB, MB.sec, data.consumed.in.nMsg, nMsg.sec")
+        println(s"start.time, end.time, data.consumed.in.MB, MB.sec, data.consumed.in.nMsg, nMsg.sec${if (!config.useOldConsumer) ", join.group.ms" else ""}")


A couple suggestions:

Maybe the name could be total.rebalance.time or something like that?

Perhaps additionally we could report total.fetch.time?

Does total.fetch.time include total.rebalance.time ?

Let's say they are independent. We could also include total.time, but perhaps no need.

hachikuji · 2017-06-02T02:54:11Z

core/src/main/scala/kafka/tools/ConsumerPerformance.scala


    totalMessagesRead.set(messagesRead)
    totalBytesRead.set(bytesRead)
+    joinTime.set(joinEnd - joinStart)


Perhaps we should allow for the possibility that we have more than one rebalance?

Currently, this perf tool does not support multiple consumer instances in one consumer group for new consumer, so is it possible that there will be more than one rebalance?

The user can pass the groupId as a parameter, so seems possible, right?

And further rebalances may be possible anyway depending on the state of the brokers.

2. Added `total.fetch.time` 3. Add support to count total time for multiple rebalances

asfbot · 2017-06-02T08:39:54Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/4780/
Test FAILed (JDK 8 and Scala 2.12).

asfbot · 2017-06-02T08:46:57Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/4796/
Test PASSed (JDK 7 and Scala 2.11).

hachikuji · 2017-06-02T21:06:12Z

core/src/main/scala/kafka/tools/ConsumerPerformance.scala

    var lastBytesRead = 0L
    var lastMessagesRead = 0L
+    var joinStart = 0L
+    var totalRebalanceTimeExceptTheFirst = 0L


I'm not sure I understand what you're trying to do here. Why not just count all rebalances?

The time for the first rebalance happens before the fetch but the rest would happen during the fetch. That's why I removed the total time of other rebalances from the total fetch time. Does it make any sense?

@hachikuji What do you think of the comment above?

I still don't understand why you want to treat the first rebalance specially. Can you elaborate please? Intuitively, I expect that we count the total time for the test and subtract the total rebalance time to get the fetch time. Is there anything wrong with that?

…'s comments.

huxihx · 2017-06-06T00:45:35Z

@hachikuji yes, you are right. I complicated the issue. Addressed the comments and please review again. Thanks.

asfbot · 2017-06-06T01:40:36Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/4938/
Test PASSed (JDK 7 and Scala 2.11).

asfbot · 2017-06-06T02:09:31Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/4922/
Test PASSed (JDK 8 and Scala 2.12).

huxihx · 2017-06-07T00:06:32Z

@hachikuji Please take some time to review. Thanks

hachikuji · 2017-06-07T05:37:41Z

core/src/main/scala/kafka/tools/ConsumerPerformance.scala

      consumerConnector.shutdown()
    }
    val elapsedSecs = (endMs - startMs) / 1000.0
+    fetchTimeInMs.set((endMs - startMs) - fetchTimeInMs.get)


I think you probably mean to subtract joinGroupTimeInMs?

huxihx · 2017-06-07T06:33:08Z

Thanks for the comments and sorry for the typo. Please review again.

asfbot · 2017-06-07T07:26:39Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/5016/
Test PASSed (JDK 7 and Scala 2.11).

asfbot · 2017-06-07T07:42:42Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/5000/
Test PASSed (JDK 8 and Scala 2.12).

hachikuji · 2017-06-07T18:08:42Z

core/src/main/scala/kafka/tools/ConsumerPerformance.scala

@@ -98,10 +101,17 @@ object ConsumerPerformance {
      consumerConnector.shutdown()
    }
    val elapsedSecs = (endMs - startMs) / 1000.0


I think the main point of this patch is to compute the throughput statistics based only on the time spent fetching, so maybe we can move this below and just use fetchTimeInMs.get / 1000.0? We could also report MB.sec as two separate values: total.MB.sec which uses the total time, and fetch.MB.sec which uses only the fetch time. Similarly for nMsg.sec.

2. Ditto for `nMsg.sec` 3. Refined output format

huxihx · 2017-06-08T01:34:49Z

@hachikuji Refined the code to address your comments. A sample output is as below:

Does it look good now?

asfbot · 2017-06-08T02:28:48Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/5035/
Test PASSed (JDK 8 and Scala 2.12).

asfbot · 2017-06-08T02:36:45Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/5051/
Test PASSed (JDK 7 and Scala 2.11).

huxihx · 2017-06-11T01:05:07Z

@hachikuji Could you take some time to review this PR? Thanks.

hachikuji · 2017-06-12T06:50:12Z

@huxihx Yes, will get to it early this week. Sorry for the delay

huxihx · 2017-06-16T00:45:43Z

@hachikuji Hello, did you get any chances to review this PR? Thanks.

asfgit · 2017-06-16T01:44:17Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/5366/
Test PASSed (JDK 8 and Scala 2.12).

asfgit · 2017-06-16T01:49:05Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/5381/
Test PASSed (JDK 7 and Scala 2.11).

hachikuji · 2017-06-20T20:21:52Z

core/src/main/scala/kafka/tools/ConsumerPerformance.scala

+          config.dateFormat.format(startMs), config.dateFormat.format(endMs), totalMBRead,
+          totalMBRead / elapsedSecs, totalMessagesRead.get, totalMessagesRead.get / elapsedSecs))
+      } else {
+        println("---- performance test result ----")


It seems unnecessary to make major changes to the output. Why don't we just keep the current format and add the additional fields? I thought the old patch did this and it seemed fine.

asfgit · 2017-08-09T01:55:42Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/6654/
Test FAILed (JDK 7 and Scala 2.11).

asfgit · 2017-08-09T03:14:08Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/6655/
Test PASSed (JDK 7 and Scala 2.11).

asfgit · 2017-08-09T03:42:23Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/6640/
Test PASSed (JDK 8 and Scala 2.12).

huxihx · 2017-08-09T03:50:28Z

@hachikuji Please review the patch. Thanks.

hachikuji

Left a couple more comments.

hachikuji · 2017-08-10T23:19:34Z

core/src/test/scala/unit/kafka/tools/ConsumerPerformanceTest.scala

  def testHeaderMatchBody(): Unit = {
    Console.withOut(outContent) {
-      ConsumerPerformance.printHeader(true)
+      ConsumerPerformance.printHeader(true, false)


Please provide parameter names. Also, let's add additional test cases so that we are covering all of the variations: detailed=true/false, consumer=old/new.

Please address this comment.

hachikuji · 2017-08-10T23:24:47Z

core/src/main/scala/kafka/tools/ConsumerPerformance.scala

+          s"${if (!useOldConsumer) ", rebalance.time.ms, fetch.time.ms, fetch.MB.sec, fetch.nMsg.sec" else ""}"
+        )
+      } else {
+        println("time, threadId, data.consumed.in.MB, MB.sec, data.consumed.in.nMsg, nMsg.sec")


Is there any reason we don't include the new fields in the "detailed" view?

In the detailed view, can we safely think fetch.time.ms is always equal to elapsedMs in printProgressMessage? If yes, then the new fields fetch.MB.sec and fetch.nMsg.sec will always be equal to the existing ones MB.sec and nMsg.sec respectively, so that's why I did not have those new fields included in the detailed view. Does it make any sense?

I'm not sure I follow. The fetch time is always equal to the total time minus the join group time. Can we not compute that periodically?

hachikuji · 2017-08-16T22:05:47Z

core/src/main/scala/kafka/tools/ConsumerPerformance.scala

    val consumerTimeout = new AtomicBoolean(false)
    var metrics: mutable.Map[MetricName, _ <: Metric] = null
+    val joinGroupTimeInMs = new AtomicLong(0)
+    val fetchTimeInMs = new AtomicLong(0)


There doesn't appear to be any reason for fetchTimeInMs to be an AtomicLong. We can use a regular long and just initialize it below.

huxihx · 2017-08-17T02:45:56Z

@hachikuji Thanks for the comments. Please review again.

asfgit · 2017-08-17T03:42:46Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/6836/
Test PASSed (JDK 7 and Scala 2.11).

asfgit · 2017-08-17T04:07:43Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/6822/
Test PASSed (JDK 8 and Scala 2.12).

hachikuji

Thanks for the updates. A few more comments.

hachikuji · 2017-08-19T00:18:09Z

core/src/main/scala/kafka/tools/ConsumerPerformance.scala

    var metrics: mutable.Map[MetricName, _ <: Metric] = null
    val joinGroupTimeInMs = new AtomicLong(0)
-    val fetchTimeInMs = new AtomicLong(0)
+    var fetchTimeInMs = 0L


This doesn't need to be a var. We can just initialize it below, right?

hachikuji · 2017-08-19T00:18:46Z

core/src/test/scala/unit/kafka/tools/ConsumerPerformanceTest.scala

  @Test
-  def testHeaderMatchBody(): Unit = {
+  def testDetailedHeaderMatchBody(): Unit = {
+    testHeaderMatchContent(true, false, 2, () => ConsumerPerformance.printProgressMessage(1, 1024 * 1024, 0, 1, 0, 0, 1,


Can you add parameter names for the two booleans?

hachikuji · 2017-08-19T00:21:09Z

core/src/main/scala/kafka/tools/ConsumerPerformance.scala

+    joinTimeOpt match {
+      case Some(joinTime) =>
+        val fetchTimeMs = endMs - beginTime.getOrElse(startMs) - joinTime
+        println(", %d, %d, %.4f, %.4f".format(joinTime, fetchTimeMs, (bytesRead * 1.0 / (1024 * 1024) / (fetchTimeMs / 1000.0)),


Now we're printing two lines in every call to printProgressMessage? Shouldn't we just include the additional information into one message?

@hachikuji The new-added fields are within the same line since I changed the first println to print. Am I doing right?

… Removed useless initialization for

huxihx · 2017-08-21T01:21:03Z

@hachikuji Thanks for your comments. Already addressed your comment 1 and 3. About comment 2, seems I already changed println to print so the new-added fields are also within the same line. Please review again.

hachikuji

Thanks for the updates. Two more comments.

hachikuji · 2017-08-22T23:27:36Z

core/src/main/scala/kafka/tools/ConsumerPerformance.scala

          if (config.showDetailedStats)
-            printProgressMessage(0, bytesRead, lastBytesRead, messagesRead, lastMessagesRead, lastReportTime, currentTimeMillis, config.dateFormat)
+            printProgressMessage(0, bytesRead, lastBytesRead, messagesRead, lastMessagesRead,
+              lastReportTime, currentTimeMillis, config.dateFormat, Some(testStartTime), Some(joinTime.get))


There seems to be no reason for testStartTime to be an Option.

Why do we call get on joinTime and rewrap it instead of just passing it through?

Since joinTime is an AtomicLong, and printProgressMessage expects an Option, that's why I used Some(joinTime.get) here.
For old consumers, there is no join time, so printProgressMessage exposes an Option here. Does it make any senses?

hachikuji · 2017-08-22T23:35:54Z

core/src/main/scala/kafka/tools/ConsumerPerformance.scala

                           endMs: Long,
-                           dateFormat: SimpleDateFormat) = {
+                           dateFormat: SimpleDateFormat,
+                           beginTime: Option[Long] = None,


It's a little weird that we have both a startTime and a beginTime. I think the reason is that beginTime is computed from the start of the test, but startTime is computed after the first rebalance. However, now that we are calculating the rebalance time separately, I wonder if we still need to maintain this distinction. Maybe we can just use the test start time for both cases.

startTime changes in each round of printProgressMessage but beginTime does not change after a test begins. printProgressMessage must use startTime to calculate TPS in a single call.

However, we also need to pass through beginTime which does not change after perf test gets started. It is used to calculate the total fetch time and fetch-time-related measures.

What do you think?

Ok, that's fair. In that case, perhaps we should do something similar for the join time. Maybe we can have one field which tracks periodic join time and is reset after each reporting interval, and another field which tracks the total join time.

added periodicJoinTimeInMs to track join time for a single interval.

huxihx · 2017-09-06T02:48:02Z

retest it please.

huxihx · 2017-09-06T05:49:04Z

@hachikuji please review it again. Thanks

hachikuji · 2017-09-08T17:44:29Z

retest this please

hachikuji

Some more comments.

hachikuji · 2017-09-08T17:54:52Z

core/src/main/scala/kafka/tools/ConsumerPerformance.scala

-    else
-      println("time, threadId, data.consumed.in.MB, MB.sec, data.consumed.in.nMsg, nMsg.sec")
+  private[tools] def printHeader(showDetailedStats: Boolean, useOldConsumer: Boolean): Unit = {
+    val newFieldsInHeader = s"${if (!useOldConsumer) ", total.rebalance.time.ms, fetch.time.ms, fetch.MB.sec, fetch.nMsg.sec" else ""}"


nit: it's more straightforward to pull the if out of the string, eg:

if (!useOldConsumer) ", rebalance.time.ms, fetch.time.ms, fetch.MB.sec, fetch.nMsg.sec" else ""

hachikuji · 2017-09-08T17:56:01Z

core/src/main/scala/kafka/tools/ConsumerPerformance.scala

+        println("start.time, end.time, data.consumed.in.MB, MB.sec, data.consumed.in.nMsg, nMsg.sec" + newFieldsInHeader)
+      } else {
+        println("time, threadId, data.consumed.in.MB, MB.sec, data.consumed.in.nMsg, nMsg.sec" +
+          s"${if (useOldConsumer) "" else ", rebalance.time.ms" + newFieldsInHeader}")


Isn't this redundant?

You mean all these lines are redundant? Isn't it printing the header based on whether using old or new consumer and whether printing in detailed?

I meant that the check for useOldConsumer is redundant give how we built newFieldsInHeader.

Checking useOldConsumer here is because new consumer should show the rebalance time for each interval as you suggest. However, the old consumer does not get this field displayed. Does it make sense?

hachikuji · 2017-09-08T18:06:47Z

core/src/main/scala/kafka/tools/ConsumerPerformance.scala

      def onPartitionsAssigned(partitions: util.Collection[TopicPartition]) {
        isAssigned.set(true)
+        joinTime.addAndGet(System.currentTimeMillis - joinStart)
+        joinTimeMsInSingleRound = System.currentTimeMillis - joinStart


There could be multiple joins within each reporting period, so this should be a +=, right?

yes, you are right. Thanks.

hachikuji · 2017-09-08T18:10:08Z

core/src/main/scala/kafka/tools/ConsumerPerformance.scala

+        joinStart = System.currentTimeMillis
      }})
-    val joinStart = System.currentTimeMillis()
+    val joinStartForFirstTime = System.currentTimeMillis()


Since we are now reporting the join time separately, I don't think we need the loop below anymore. It was initially intended to discount the rebalance overhead, but now we account for that transparently.

hachikuji · 2017-09-08T18:13:54Z

core/src/main/scala/kafka/tools/ConsumerPerformance.scala

-        1000.0 * (mbRead / elapsedMs), messagesRead, ((messagesRead - lastMessagesRead) / elapsedMs) * 1000.0))
+    print("%s, %d, %.4f, %.4f, %d, %.4f".format(dateFormat.format(endMs), id, totalMBRead,
+      1000.0 * (mbRead / elapsedMs), messagesRead, ((messagesRead - lastMessagesRead) / elapsedMs) * 1000.0))
+    joinTimeOpt match {


Maybe we should just move this block to a separate function so that we don't need all of the Option parameters?

Sorry, I am not sure if I fully understand this comment. Do you mean create a new function to handle all the printing things whereas printProgressMessage is that type of function?

Yes, it's a minor issue, but it's kind of annoying that we have three optional parameters which are all expected in the case of the new consumer. I think a cleaner approach is to use a separate method. Something approximately like this:

def printBasicProgress ... def printExtraProgress ... def printNewConsumerProgress { printBasicProgress() printExtraProgress() println() } def printOldConsumerProgress() { printBasicProgress() println() }

Then we shouldn't need the optional parameters.

hachikuji · 2017-09-08T18:14:28Z

core/src/main/scala/kafka/tools/ConsumerPerformance.scala

          if (config.showDetailedStats)
-            printProgressMessage(0, bytesRead, lastBytesRead, messagesRead, lastMessagesRead, lastReportTime, currentTimeMillis, config.dateFormat)
+            printProgressMessage(0, bytesRead, lastBytesRead, messagesRead, lastMessagesRead,
+              lastReportTime, currentTimeMillis, config.dateFormat, Some(testStartTime), Some(joinTime.get), Some(joinTimeMsInSingleRound))


Is testStartTime correct? Shouldn't this be the beginning of the interval?

testStartTime means the exact time where the entire consumer perf tool begins. Beginning of the interval for each round is 6th parameter of printProgressMessage. Does it make sense?

What I am not understanding is why we use the test start time when the other periodic statistics are computed based on the the interval start time. Shouldn't we be consistent?

huxihx · 2017-09-15T00:57:21Z

@hachikuji Could you address my replied comments to see if they make any senses? Thanks.

hachikuji · 2017-09-15T16:15:11Z

core/src/main/scala/kafka/tools/ConsumerPerformance.scala

+      case Some(joinTime) =>
+        val fetchTimeMs = endMs - beginTime.getOrElse(startMs) - joinTime
+        println(", %d, %d, %d, %.4f, %.4f".format(
+          periodicJoinTimeInMs.getOrElse(0L), joinTime, fetchTimeMs, (bytesRead * 1.0 / (1024 * 1024) / (fetchTimeMs / 1000.0)),


Why do we have two join times here? We only have one join time in the header, right? The periodic time should be enough I think.

1. refactoring printing to common functions 2. removed redundant codes

huxihx · 2017-09-18T01:59:40Z

@hachikuji Thanks for the comments. Please review again.

hachikuji

LGTM. Merging with a few fixes

hachikuji · 2017-09-20T16:44:10Z

@huxihx In the future, the review will go much faster if you are a bit more thorough with your testing. In this case, there were still some problems which would have been obvious had you executed the performance tool yourself manually.

There was no newline printed after the normal (non-detailed) output.
The detailed output was still computed inconsistently. If the join time for the interval was 0, then the fetch mb/sec (for example) should have exactly matched the total mb/sec. Same for messages/sec.

I fixed these problems during merging.

KAFKA-5358: Consumer perf tool should count rebalance time.

be43bf3

Added 'join.group.ms' for new consumer to calculate the time of joining group.

hachikuji reviewed Jun 2, 2017

View reviewed changes

1. Refined the name to total.rebalance.time

cbdf6c1

2. Added `total.fetch.time` 3. Add support to count total time for multiple rebalances

hachikuji reviewed Jun 2, 2017

View reviewed changes

Refined time counting for both fetcher and rebalance as per hachikuji…

7600e6b

…'s comments.

hachikuji reviewed Jun 7, 2017

View reviewed changes

Correct the counting for total fetch time.

08ff452

hachikuji reviewed Jun 7, 2017

View reviewed changes

1. Split MB.sec into two parts: total.MB.sec and fetch.MB.sec

8c80f13

2. Ditto for `nMsg.sec` 3. Refined output format

hachikuji reviewed Jun 20, 2017

View reviewed changes

returned back to the original output format for new consumer

edf1d08

correct printHeader invoking to have testHeaderMatchBody passed

bbba9b0

hachikuji reviewed Aug 10, 2017

View reviewed changes

hachikuji reviewed Aug 16, 2017

View reviewed changes

Added newly-created fields for detailed views.

3a1ce0f

hachikuji reviewed Aug 19, 2017

View reviewed changes

1. Added parameter name when invoking testDetailedHeaderMatchBody; 2.…

c517815

… Removed useless initialization for

hachikuji reviewed Aug 22, 2017

View reviewed changes

huxihx closed this Aug 23, 2017

added a field that tracks periodic join time.

9adda74

huxihx reopened this Sep 6, 2017

hachikuji reviewed Sep 8, 2017

View reviewed changes

hachikuji reviewed Sep 15, 2017

View reviewed changes

addressed Jason's last comments

dfa9422

1. refactoring printing to common functions 2. removed redundant codes

hachikuji approved these changes Sep 20, 2017

View reviewed changes

asfgit closed this in 239dad1 Sep 20, 2017

Comments

Conversation

huxihx commented Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asfbot commented Jun 1, 2017

Uh oh!

asfbot commented Jun 1, 2017

Uh oh!

hachikuji 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!

asfbot commented Jun 2, 2017

Uh oh!

asfbot commented Jun 2, 2017

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!

huxihx commented Jun 6, 2017

Uh oh!

asfbot commented Jun 6, 2017

Uh oh!

asfbot commented Jun 6, 2017

Uh oh!

huxihx commented Jun 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huxihx commented Jun 7, 2017

Uh oh!

asfbot commented Jun 7, 2017

Uh oh!

asfbot commented Jun 7, 2017

Uh oh!

hachikuji Jun 7, 2017 • 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!

huxihx commented Jun 8, 2017

Uh oh!

asfbot commented Jun 8, 2017

Uh oh!

asfbot commented Jun 8, 2017

Uh oh!

huxihx commented Jun 11, 2017

Uh oh!

hachikuji commented Jun 12, 2017

Uh oh!

huxihx commented Jun 16, 2017

Uh oh!

asfgit commented Jun 16, 2017

Uh oh!

asfgit commented Jun 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfgit commented Aug 9, 2017

Uh oh!

asfgit commented Aug 9, 2017

Uh oh!

huxihx commented Jun 1, 2017 •

edited

Loading

hachikuji Jun 7, 2017 •

edited

Loading

huxihx commented Aug 21, 2017 •

edited

Loading