adding new post aggregators for test statistics to druid-stats extension by chunghochen · Pull Request #4532 · apache/druid

chunghochen · 2017-07-11T19:34:11Z

Please refer to https://www.paypal-engineering.com/2017/06/29/democratizing-experimentation-data-for-product-innovations/ for more context and details.

jihoonson

Hi @chunghochen, thanks for the nice work!
I left a couple of comments. Also, please merge the master branch and check the build failure due to invalid formats.

jihoonson · 2017-07-28T06:37:21Z

+{
+  "type": "zscore2sample",
+  "name": "<output_name>",
+  "fields": [<count 1 (post_aggregator1)>, <sample size 1 (post_aggregator2)>, <count 2 (post_aggregator3)>, <sample size 2 (post_aggregator4)>]


It looks that the number of arguments is always 4 and their order matters. If so, I think it would be better to receive 4 individual arguments instead of a fields list because it is less error-prone and intuitive which arguments are needed. What do you think?

Yes, fields in this case required 4 arguments and the order matters.
However, I don't think it is more error prone or less intuitive, than using individual arguments.
I think at this juncture, fields as input parameters fit well since

It is aligned and consistent with the way fields are used in Druid. Pl see http://druid.io/docs/latest/querying/aggregations.html and http://druid.io/docs/latest/querying/post-aggregations.html

As we might fine-tune, or evolve the input parameters so that the stat functions can be most useful and powerful, keep them in fields at this juncture should be good, so it can be continuously evolved / fine-tuned without dramatically changing the input signature.

However, I don't think it is more error prone or less intuitive, than using individual arguments.

With the current design, users should remember the meaning of arguments according to their appearance order in the fields list or see the document every time. What's worse, users are difficult to figure out even though they input the arguments in wrong order (e.g., [<count 2 (post_aggregator3)>, <sample size 2 (post_aggregator4)>, <count 1 (post_aggregator1)>, <sample size 1 (post_aggregator2)>]).

If they should be specified individually, users, at least, cannot input arguments of wrong number or in wrong order.

It is aligned and consistent with the way fields are used in Druid.

In this case, I think the usability (stated above) matters rather than consistency. There is no point for arbitrary number of arguments.

As we might fine-tune, or evolve the input parameters so that the stat functions can be most useful and powerful, keep them in fields at this juncture should be good, so it can be continuously evolved / fine-tuned without dramatically changing the input signature.

If you have any plan, please let us know. I guess even changing input signature is not a big problem because arguments can be easily added and removed (thanks to json).

I agree with @jihoonson here -- a fields array makes sense if the fields are all treated basically the same, and if arbitrary numbers of fields are allowed, but in this case neither is true. The fields mean different things semantically, and the number is fixed, so the API would be nicer if the four arguments are given meaningful names.

It would still be easy to evolve the API with named parameters, and in fact I think it's easier, since the parameters are named rather than positional.

jihoonson · 2017-07-28T06:40:21Z

+        <dependency>
+            <groupId>org.apache.commons</groupId>
+            <artifactId>commons-math3</artifactId>
+            <version>3.6.1</version>


Version is not needed here because the parent module specifies it.

Great catch! I overlooked it when porting to the latest version of Druid.

jihoonson · 2017-08-11T03:12:13Z

Also, please add a doc for new post aggregators.

gianm

Thank you for the contribution @chunghochen. I left some comments on the code.

I didn't review the math, just reviewed the Druid aspects of the code.

gianm · 2017-08-10T03:54:16Z

+{
+  "type": "zscore2sample",
+  "name": "<output_name>",
+  "fields": [<count 1 (post_aggregator1)>, <sample size 1 (post_aggregator2)>, <count 2 (post_aggregator3)>, <sample size 2 (post_aggregator4)>]


I agree with @jihoonson here -- a fields array makes sense if the fields are all treated basically the same, and if arbitrary numbers of fields are allowed, but in this case neither is true. The fields mean different things semantically, and the number is fixed, so the API would be nicer if the four arguments are given meaningful names.

It would still be easy to evolve the API with named parameters, and in fact I think it's easier, since the parameters are named rather than positional.

gianm · 2017-08-10T03:56:13Z

+import java.util.Set;
+
+/**
+ * Created by chunchen on 4/5/17.


Please don't include author information in the code. Git will remember it.

gianm · 2017-08-10T03:57:00Z

+  public PvaluefromZscorePostAggregator(
+      @JsonProperty("name") String name,
+      @JsonProperty("field") PostAggregator field
+  ) {


Brace style isn't quite right here (and elsewhere in this file) so please apply Druid code style linked off https://github.com/druid-io/druid/blob/master/CONTRIBUTING.md.

gianm · 2017-08-11T00:39:38Z

+  }
+
+  @Override
+  public String toString() {


Consider overriding equals and hashCode as well.

gianm · 2017-08-11T00:43:09Z

+
+  @Override
+  public PostAggregator decorate(Map<String, AggregatorFactory> aggregators) {
+    return this;


This should recursively decorate the PostAggregator field, for example:

return new PvaluefromZscorePostAggregator(name, Iterables.getOnlyElement(Queries.decoratePostAggregator(Collections.singletonList(field), aggregators));

That way, any nested post-aggregators that need decoration will receive it.

gianm · 2017-08-11T06:27:43Z

+   * @param the success count of population 2
+   * @param sample size of population 2
+   */
+  private double zScoreTwoSamples(Double s1count, Double p1count, Double s2count, Double p2count) {


These might as well be double rather than Double as they are assumed to be non-null.

gianm · 2017-08-11T06:29:12Z

+      return (convertRate1 - convertRate2) /
+          Math.sqrt((convertRate1 * (1 - convertRate1) / p1count) +
+              (convertRate2 * (1 - convertRate2) / p2count));
+    } catch (Exception ex) {


What kind of Exception can be thrown here? Could this be made more specific?

Also, is it possible to use some other mechanism of detecting whatever problem this exception is detecting? Exceptions are quite slow compared to other forms of flow control and so I'm concerned about using them in a post-aggregator compute function.

gianm · 2017-08-11T06:29:41Z

+import java.util.Map;
+
+/**
+ * Created by chunchen on 4/23/17.


Please don't include author tags.

gianm · 2017-08-11T06:40:26Z

+    try {
+      NormalDistribution normDist = new NormalDistribution();
+      return normDist.cumulativeProbability(x);
+    } catch (Exception ex) {


What kind of Exception can be thrown here? Could this be made more specific?

Also, is it possible to use some other mechanism of detecting whatever problem this exception is detecting? Exceptions are quite slow compared to other forms of flow control and so I'm concerned about using them in a post-aggregator compute function.

gianm · 2017-08-11T06:52:49Z

+  }
+
+  @Override
+  public String toString() {


Consider overriding equals and hashCode as well.

chunghochen · 2017-08-22T05:58:09Z

Thank you @gianm and @jihoonson for the reviews and comments!!
I've made changes per review comments.
In my env, it appears passing checkstyle, as from mvn log:

[INFO] --- maven-checkstyle-plugin:2.17:check (validate) @ druid ---
[INFO] Starting audit...
Audit done.
Not sure why it behaves differently from druid CI env?
Thank you both for all the help!!

jihoonson · 2017-08-22T06:35:32Z

Some new rules (e.g., #4651, #4564, ...) have been added to the master branch. Please merge the master branch first and test again.

update with latest changes from upstream

…erge upstream/master

chunghochen · 2017-08-27T00:34:05Z

I can't see the errors when clicking the Details link of
continuous-integration/travis-ci/pr; other than #12696.3 canceled.
BTW, some anecdotal data points:

If my memory serves right, this pull request never fails on this test before.
It also passed all mvn tests before I committed. However, I ran into Transient failure of DruidCoordinatorBalancerTest #4389 every so often. Not sure if the error causing "Mid-level service client and updated high-level clients. #12696.3 canceled" is also transient failures of some sort?
Please help/advise on how to move forward this pull request.
Many thanks!!

jihoonson · 2017-08-29T00:00:40Z

@chunghochen, hmm i'm not sure why the job was canceled. Maybe there was a problem in travis. I restarted the job.

jihoonson · 2017-09-09T06:20:32Z

+p1 = (successCount1) / (sample size 1)

-For example,
+p2 = (successCount2) / (sample size 2)


It would be better to change sample size 1 and sample size 2 to camel case for consistency and better readability.

As we are doing two sample z-test to calculate the zscore, from sample 1 and sample 2.
Therefore, sample 1 size and sample 2 size are more meaningful in this context.
Will change sample2size to sample2Size for consistency.

jihoonson · 2017-09-09T06:22:03Z

  "name": "<output_name>",
-  "fields": [<count 1 (post_aggregator1)>, <sample size 1 (post_aggregator2)>, <count 2 (post_aggregator3)>, <sample size 2 (post_aggregator4)>]
+  "successCount1": <post_aggregator> success count of sample 1,
+  "sample1Size": <post_aggregaror> sample 1 size,


Is sampleSize1 better? Because successCount is postfixed by a number.

As described
successCount1: success count of sample 1
sample1size: sample 1 size
In short, we are doing two sample z-test to calculating the zscore, from sample 1 and sample2.
It appears sampleSize1 deviates from its original statistical meaning.

I think people won't be confused if the sampleSize is postfixed by the number and we give a proper description. Two different types of numbering for arguments make people confused and being prone to mistakes.

jihoonson · 2017-09-09T06:29:02Z

 # Test Stats Aggregators

-Incorporates test statistics related aggregators, including z-score and p-value. Please refer to [https://www.paypal-engineering.com/2017/06/29/democratizing-experimentation-data-for-product-innovations/](https://www.paypal-engineering.com/2017/06/29/democratizing-experimentation-data-for-product-innovations/) for background and details.
+Incorporates test statistics related aggregators, including z-score and p-value. Please refer to [https://www.paypal-engineering.com/2017/06/29/democratizing-experimentation-data-for-product-innovations/](https://www.paypal-engineering.com/2017/06/29/democratizing-experimentation-data-for-product-innovations/) for math background and details, although its input spec and example are out of date.


although its input spec and example are out of date.

I'm not sure this is a necessary statement because if the linked document is really out of date and irrelevant to the description here, we usually change it to more proper link.

Also, I think it's better to add links describing the definitions of z-score and p-value like wikipedia links because that blog post describes a sort of bigger concept how these new post aggregators are used in your case. Even the blog post links wikipedia for the definitions of z-score and p-value.

Hi Jihoon,

The purpose of the blog post is to explain and connect all the related concepts to derive how post aggregators do the calculations. IOW, it is to bridge the gap between what wikipedia states and what the post aggregators do.

Not sure if you're asking to remove "although its input spec and example are out of date" in test-stats.md?
Please advise. Many thanks!

The purpose of the blog post is to explain and connect all the related concepts to derive how post aggregators do the calculations. IOW, it is to bridge the gap between what wikipedia states and what the post aggregators do.

If so, I think it's better to remove "although its input spec and example are out of date." statement.

okay, if you insist.

jihoonson · 2017-09-09T06:32:39Z

+     "successCount1" :
       { "type"   : "constant",
-         "name"   : "successCountPopulation1",
+         "name"   : "successCountFromPopulation1Sample",


Same here. Looks better to put the number at last rather than in the middle.

We are calculating two-tailed p-value from zscore.

I think it is important to have the naming represents its statistical meaning.
In this case, zscore2sample really means we are doing two sample z-test to calculate the zscore.
Further, we are calculating two-tailed P-value from the z-score.

Naming calls for compressing a few statistical concepts together. It is more than mechanically placing the number in the middle or at the end of the name. We need to first describe the name in statistical terms and then try to compress it to a name which makes sense.

Ok, thanks for explanation.

jihoonson · 2017-09-09T06:37:32Z

 */
 @JsonTypeName("zscore2sample")
-public class ZtestPostAggregator implements PostAggregator {
+public class ZtestPostAggregator implements PostAggregator


Just ouf of curiosity, why is the class name different from its type name?

The class name follows the naming convention of all Druid PostAggregators.
The Json Type Name just tried to be brief and meaningful.

The class name tries to be brief while following the naming convention of all Druid postAggregators.
The Type Name just tries to be meaningful in the context of test statistics.

I mean, why the class name can't be simply ZScore2SamplePostAggregator? This is maybe a silly question because I'm not familiar with stats.

There are various test stats can be calculated based on z-test. It' not clear whether we should have one class for every variation of z-test based stats. For example, it might make more sense to have 1-sample and 2-sample zscore in one class, rather than in two different classes. In other words, I'm not thinking in terms of naming convention, but rather more along the line of extensibility and componentization.

It sounds good to me, but if so, the json type name should also be an extendable name, right?

The JsonTypeName (zscore2sample) is intended to be brief and meaningful to the user, so it is different from the class name ZtestPostAggregator. Not sure what you mean by "the json type name should also be an extendable name"? But if you mean we should make the type name in a way so that we can easily add a new type in a consistent manner; then yes. JsonTypeName is designed from the user perspective and is exposed to the user, while the class name is to do with implementations. The rationales behind these namings are different.

jihoonson · 2017-09-09T06:49:38Z

+  private final PostAggregator successCount1;
+  private final PostAggregator sample1Size;
+  private final PostAggregator successCount2;
+  private final PostAggregator sample2Size;


get() method for these methods are missing.

Also please add a json serde test.

These two postAggregators implement PostAggregator Interface. Get() is only needed for Aggregator Interface. I failed ti see any postAggregator implementing get() method.

Not sure why json serde is needed in this case? These two postAggregators didn’t persist into Druid by per se. And there is no custom Serde either.
One can think of them of glorified arithmetic post aggregators to calculate some test statistics.

These two postAggregators implement PostAggregator Interface. Get() is only needed for Aggregator Interface. I failed to see any postAggregator implementing get() method.

Not sure why json serde is needed in this case? These two postAggregators didn’t persist into Druid by per se. And there is no custom Serde either.
One can think of them of glorified arithmetic post aggregators to calculate some test statistics.

Looks that my comments were confused. Sorry for lack of details. I meant, the series of methods of getSuccessCount1(), getSample1Size(), getSuccessCount2(), and getSample2Size() should be added and annotated with @JsonProperty because they are needed for JSON serialization. Unit tests are needed to avoid such mistakes.

Thanks for clarifying this! will do as requested.

jihoonson · 2017-09-09T07:03:53Z

@@ -46,79 +48,92 @@
   http://facweb.cs.depaul.edu/sjost/csc423/documents/test-descriptions/indep-z.pdf


This comment should be javadoc.

will do :-)

jihoonson · 2017-09-09T07:05:14Z

+      @JsonProperty("sample1Size") PostAggregator sample1Size,
+      @JsonProperty("successCount2") PostAggregator successCount2,
+      @JsonProperty("sample2Size") PostAggregator sample2Size
+


Unnecessary whitespace

Do you mean to remove line 67?
I'm using Druid 11 checkstyle.xml and IntelliJ to do reformatting. And it passes the stylecheck.
According to IntelliJ's interpretation continuation indent is 4.
Just like to make sure I understand what you mean. :-)

Do you mean line#67? Or if there are more unnecessary whitespace you're referring to?
Just like to make sure we are on the same page. :-)

Yes, please remove this line.

will do! 👍

jihoonson · 2017-09-09T07:06:08Z

-        + fields
-        + "}";
+           "name='" + name + '\'' +
+           ", successCount1" + successCount1 +


Please insert '=' between string and value.

jihoonson · 2017-09-09T07:12:24Z

+  )
+  {
    Preconditions.checkNotNull(name, "Must have a valid, non-null post-aggregator");
+    Preconditions.checkNotNull(zScore, "Must have a valid, non-null post-aggregator");


Also it's better to check zScore is an intance of ZtestPostAggregator.

One can calculate the z-score oneself without using ZtestPostAggregator. Didn't check whether it is an instance of ZtestPostAggregator as we should keep it flexible and customizable if the user opts for it.

Actually, we want to keep it flexible and open.
For example, one can calculate it's own z-score for example if needed, and just feed its zscore to calculate p-value.

Ah, ok. It makes sense.

jihoonson · 2017-09-19T19:37:44Z

+  }
+
+  @Override
+  public int hashCode()


Please implement equals() method either. This will remove the potential problem when working with collections.

jihoonson · 2017-09-19T19:37:57Z

+  }
+
+  @Override
+  public int hashCode()


Please implement equals() method either. This will remove the potential problem when working with collections.

chunghochen · 2017-09-20T19:15:21Z

Not sure why the checks failed?
My mvn regressions passed all tests in my env. I can provide the log if needed.

can't click into details of Inspections: pull requests (Druid) — TeamCity build failed. It asked me to sign up and but even I signed up, I can't see any thing meaningful.
continuous-integration/travis-ci/pr — The Travis CI build failed
It showed failure with java.util.concurrent.locks.AbstractQueuedSynchronizer.tryAcquireSharedNanos(AbstractQueuedSynchronizer.java:1328)
I don't think my change will cause this issue. It looks like intermittent issues I ran into before.

Also I can't see ihoonson requested changes, as I clicked "See review" link but failed to see new review comment.

Please help/advise.
Thanks so much!

jon-wei · 2017-09-20T22:36:43Z

@chunghochen

We've been having issues with Travis CI recently, the third test there often fails with a timeout, I've reset that test.

To see the TeamCity report, you can click on "Login as Guest". I checked the error report there and they don't seem related to this patch.

jon-wei · 2017-09-20T22:37:44Z

Moving this to 0.11.1, as 0.11.0 is feature frozen now

jon-wei · 2017-09-20T23:26:08Z

@chunghochen Can you try merging master to your branch? The TeamCity failures shown in the report were fixed by #4809

chunghochen · 2017-09-21T21:37:59Z

@jon-wei
Thanks so much for your help!
Here is what I see from my terminal:
...@...<druid-...> git status
On branch stats-pvalue
Your branch is up-to-date with 'origin/stats-pvalue'.
...
...@...<druid-...> git remote -v
origin https://github.com/chunghochen/druid.git (fetch)
origin https://github.com/chunghochen/druid.git (push)
upstream https://github.com/druid-io/druid.git (fetch)
upstream https://github.com/druid-io/druid.git (push)
...@...<druid-...> git merge upstream/master
Already up-to-date.

Can you try merging master to your branch?

Not sure what git command I should do to merge master to my branch?
Thanks so much for all the help!

jon-wei · 2017-09-21T22:37:01Z

@chunghochen

Not sure what git command I should do to merge master to my branch?

You can do:

git fetch upstream
git merge upstream/master

chunghochen · 2017-09-21T22:43:26Z

@jon-wei
Yeap, just realized that after sending out the question.
CI is running for ~ an hour now.

chunghochen · 2017-09-24T17:40:23Z

Hopefully this pull request can be merged in when the codeline is open? :-)

chunghochen · 2017-09-26T19:22:45Z

@jihoonson Not sure where we are on this pull request?
It passed all CI regressions 5 days ago.
Thanks so much for all the help!

jihoonson

@chunghochen I think it's almost done. Left some trivial comments. In addition, please comment on #4532 (comment).

jihoonson · 2017-09-28T06:57:11Z

+
+    pvaluefromZscorePostAggregator = new PvaluefromZscorePostAggregator("pvalue", zscore);
+
+    System.out.print("zscore = " + zscore + "\n");


Please remove debug code.

jihoonson · 2017-09-28T06:57:17Z

+        -1783.8762354220219
+    ))).doubleValue();
+
+    System.out.print("pvalue = " + pvalue + "\n");


Please remove debug code.

jihoonson · 2017-09-28T06:57:22Z

+    /* Assert P-value is positive and very small */
+    Assert.assertTrue(pvalue >= 0 && pvalue < 0.00001);
+
+    System.out.print(pvaluefromZscorePostAggregator.toString());


Please remove debug code.

jihoonson · 2017-09-28T06:57:33Z

+                        zscore, 0.0001
+    );
+
+    System.out.print(ztestPostAggregator.toString());


Please remove debug code.

jihoonson · 2017-09-28T07:00:01Z

+  @Override
+  public int hashCode()
+  {
+    int result = name != null ? name.hashCode() : 0;


Please remove unnecessary null check for name. It's already tested in the constructor.

jihoonson · 2017-09-28T07:00:52Z

+  @Override
+  public int hashCode()
+  {
+    int result = name != null ? name.hashCode() : 0;


Please remove unnecessary null check for name. It's already checked in the constructor.

chunghochen · 2017-10-06T03:39:54Z

@jihoonson and @jon-wei, can you guys please help?

Not sure why it fails on both checks?
Inspections: pull requests (Druid) — TeamCity build failed, it shows the following
server/src/main/java/io/druid/client/cache
CaffeineCacheConfig.java (3)
server/src/main/java/io/druid/segment/realtime/firehose
ServiceAnnouncingChatHandlerProvider.java (1)
However, the above is not my code.

continuous-integration/travis-ci/pr — The Travis CI build failed
I can't read the log to notice how druid-server fails.
In any event, my mvn regressions passed in my laptop environment before merging with master, as well as after merging with master.

Please help!!
BTW, my change here is very trivial - simply removing some blocks per review comments.

jon-wei · 2017-10-06T04:50:37Z

@chunghochen

The CI test failed again because of an unrelated test timeout (happens to a lot of PRs), I've restarted the build for that test suite.

I believe the TeamCity build is failing because some PRs that introduced failures were committed without fixing the TeamCity issues, future builds for new PRs are still picking up the unaddressed inspection issues, your PR is fine and we can ignore them since none of the files are related to your changes.

chunghochen · 2017-10-06T22:01:52Z

@jon-wei,

Thanks so much for looking into this! And confirming my PR is fine. :-)

chunghochen · 2017-10-07T00:05:55Z

@jihoonson

@chunghochen I think it's almost done. Left some trivial comments. In addition, please comment on #4532 (comment).

Great! Hope my latest commits address your concerns? I also commented on #4532 (comment).
Not sure why this pull request still shows "Changes requested"?
Hopefully, this pull request can be quickly closed. And it won't be dragged on forever. :-)
Thanks so much for your help!!

jihoonson

Thanks @chunghochen!

chunghochen · 2017-10-14T18:45:51Z

@jihoonson @jon-wei
Cool! Thanks so much for all the help!
Look forward to see this commit in 0.11.1 release. :-)

adding new post aggregators of test stats to druid-stats extension

a7321e8

fjy added the Improvement label Jul 13, 2017

fjy added this to the 0.11.0 milestone Jul 13, 2017

jihoonson requested changes Jul 28, 2017

View reviewed changes

gianm reviewed Aug 11, 2017

View reviewed changes

changes to address code review comments

ccdf21e

ChungHo Chen added 3 commits August 25, 2017 12:23

Merge remote-tracking branch 'upstream/master' into stats-pvalue

a1a5065

update with latest changes from upstream

fix checkstyle violations using druid_intellij_formatting.xml after m…

d8c16b2

…erge upstream/master

add @OverRide annotation per CI log

b937e78

jihoonson requested changes Sep 9, 2017

View reviewed changes

jihoonson reviewed Sep 19, 2017

View reviewed changes

make changes per review comments/discussions

2604aee

jon-wei modified the milestones: 0.11.0, 0.11.1 Sep 20, 2017

Merge remote-tracking branch 'upstream/master' into stats-pvalue

f4a20b2

jihoonson reviewed Sep 28, 2017

View reviewed changes

ChungHo Chen added 2 commits October 5, 2017 15:25

remove some blocks per review comments

f89472c

Merge remote-tracking branch 'upstream/master' into stats-pvalue

eb87c2c

jihoonson approved these changes Oct 10, 2017

View reviewed changes

jon-wei approved these changes Oct 10, 2017

View reviewed changes

jon-wei merged commit 0614b92 into apache:master Oct 10, 2017

jon-wei mentioned this pull request Jan 5, 2018

[WIP] Druid 0.12.0 release notes #5211

Closed

		@@ -46,79 +48,92 @@
		http://facweb.cs.depaul.edu/sjost/csc423/documents/test-descriptions/indep-z.pdf


		pvaluefromZscorePostAggregator = new PvaluefromZscorePostAggregator("pvalue", zscore);

		System.out.print("zscore = " + zscore + "\n");

Conversation

chunghochen commented Jul 11, 2017

Uh oh!

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

jihoonson commented Aug 11, 2017

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunghochen commented Aug 22, 2017

Uh oh!

jihoonson commented Aug 22, 2017

Uh oh!

chunghochen commented Aug 27, 2017

Uh oh!

jihoonson commented Aug 29, 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!

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!

chunghochen Sep 15, 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!

Choose a reason for hiding this comment

chunghochen Sep 15, 2017 •

edited

Loading