Introduce SystemSchema tables (#5989) #6094

surekhasaharan · 2018-08-01T22:15:57Z

SYSTEM_TABLES provide visibility into the druid segments, servers and tasks.

SEGMENTS table provides details on served and published segments
SERVERS table provides details on data servers
SERVERSEGMETS table is the JOIN of SEGMENTS and SERVERS
TASKS table provides details on tasks from overlord

Sample sql queries to retrieve data from these tables:

SELECT * FROM sys.segments where datasource='wikipedia';
SELECT * FROM sys.servers;
SELECT * FROM sys.tasks where status='FAILED';

* SEGMENTS table provides details on served and published segments * SERVERS table provides details on data servers * SERVERSEGMETS table is the JOIN of SEGMENTS and SERVERS * TASKS table provides details on tasks

vogievetsky · 2018-08-02T19:20:04Z

so excited about this one!

fjy · 2018-08-13T21:18:36Z

@surekhasaharan can you fix the merge conflicts?

gianm · 2018-08-06T14:56:42Z

docs/content/querying/sql.md

@@ -481,6 +485,77 @@ SELECT * FROM INFORMATION_SCHEMA.COLUMNS WHERE TABLE_SCHEMA = 'druid' AND TABLE_
 |COLLATION_NAME||
 |JDBC_TYPE|Type code from java.sql.Types (Druid extension)|

+## SYSTEM SCHEMA
+
+SYSTEM_TABLES provide visibility into the druid segments, servers and tasks.


Please correct capitalization and naming:

The SYS schema provides visibility into Druid segments, servers and tasks.

gianm · 2018-08-06T14:57:18Z

docs/content/querying/sql.md

+SYSTEM_TABLES provide visibility into the druid segments, servers and tasks.
+For example to retrieve all segments for datasource "wikipedia", use the query:
+```sql
+select * from SYS.SEGMENTS where DATASOURCE='wikipedia';


Lowercase seems more Druid-y, so I think I'd prefer SELECT * FROM sys.segments WHERE dataSource = 'wikipedia'. The only reason INFORMATION_SCHEMA isn't like this is because it's a standard thing and uppercase seems more normal for it from looking at other databases.

ok, will make everything lowercase in docs and code. For the column names, do they need to be camelCase like dataSource, isPublished etc. or datasource, is_published or keeping them uppercase is fine ?

Hmm, good question. I think in SQL underscores are more normal, although data_source is very weird so let's not do that. Probably datasource is ok.

If anyone else has an opinion please go for it.

gianm · 2018-08-06T16:44:14Z

docs/content/querying/sql.md

+```
+
+### SEGMENTS table
+Segments tables provides details on all the segments, both published and served(but not published).


To me this reads a bit unclear, I'd suggest trying something like:

Segments tables provides details on all Druid segments, whether they are published yet or not.

gianm · 2018-08-06T19:33:10Z

docs/content/querying/sql.md

+
+|Column|Notes|
+|------|-----|
+|SEGMENT_ID||


Please include a description for all of these columns, and capitalize the first letter of each description.

Please include "size", "version", and "partition_num" too -- they are all useful. I'd also include "replicas" which should be the number of replicas currently being served.

gianm · 2018-08-06T19:33:38Z

docs/content/querying/sql.md

+|DATASOURCE||
+|START||
+|END||
+|IS_PUBLISHED|segment in metadata store|


It'd be clearer to expand this a bit: "True if this segment has been published to the metadata store."

Similar comment for the other ones.

added more description

gianm · 2018-08-16T16:39:11Z

sql/src/main/java/io/druid/sql/calcite/schema/SystemSchema.java

+          row[1] = segment.getDataSource();
+          row[2] = segment.getInterval().getStart();
+          row[3] = segment.getInterval().getEnd();
+          boolean is_available = false;


Avoid underscores in variable names: this should be isAvailable. Similar comment for other names.

gianm · 2018-08-16T16:57:12Z

sql/src/main/java/io/druid/sql/calcite/schema/SystemSchema.java

+      return Linq4j.asEnumerable(rows);
+    }
+
+    private List<ImmutableDruidDataSource> getMetadataSegments(


We should document that the coordinator must be online in order for querying this table to work properly. It's an important note since the other tables only require broker & data servers. Let's do a similar note for tasks / overlord too.

added a comment for segments and tasks table

gianm · 2018-08-16T17:17:21Z

sql/src/main/java/io/druid/sql/calcite/schema/SystemSchema.java

+    }
+
+    private List<ImmutableDruidDataSource> getMetadataSegments(
+        DruidLeaderClient druidLeaderClient,


Let's name this appropriately as to what it is (coordinator client).

gianm · 2018-08-16T17:25:12Z

sql/src/main/java/io/druid/sql/calcite/schema/SystemSchema.java

+        for (DataSegment segment : segmentMap.values()) {
+          row[0] = druidServer.getHost();
+          row[1] = segment.getIdentifier();
+          rows.add(row);


This has the same issue as the segments table above. It doesn't work to reuse arrays like this.

moved Object[] row = new Object[SERVERSEGMENTS_TABLE_SIZE]; inside the inner loop

gianm · 2018-08-16T17:25:28Z

sql/src/main/java/io/druid/sql/calcite/schema/SystemSchema.java

+    }
+
+    private List<TaskStatusPlus> getTasks(
+        DruidLeaderClient druidLeaderClient,


Let's name this what it is (indexing service client).

*Add unit tests

* Made tasks/segments stream to calcite instead of storing it in memory * Add num_rows to segments table * Refactor JsonParserIterator * Replace with closeable iterator

fix a compile error after merge, add TrafficCop param to InputStreamResponseHandler

gianm

Partial review - ended around DirectDruidClient.

gianm · 2018-08-22T17:05:04Z

docs/content/querying/sql.md

+|size|Size of segment in bytes|
+|version|Version number (generally an ISO8601 timestamp corresponding to when the segment set was first started)|
+|partition_num|Partition number (an integer, unique within a datasource+interval+version; may not necessarily be contiguous)|
+|num_replicas|Number replicas of this segment currently being served|


Number of replicas

gianm · 2018-08-22T17:06:22Z

docs/content/querying/sql.md

+
+|Column|Notes|
+|------|-----|
+|server|Server name in the form host:port|


It'd be useful to have another field for just the host. Imagine doing stuff like GROUP BY server_host to collect together all servers run on the same machine.

sure, added another column for server_host

Does SystemSchema support concatenating strings? Then, server isn't necessary. Users can just do select concat(host, cast(plaintext_port as VARCHAR)) from sys.servers.

It does support concatenating, but I think we should keep this here. It's basically the primary key of the server table, and is used for joins with the segment_servers table. The other fields (host, plaintext_port, etc) are provided too as conveniences. With system tables, since they're all generated dynamically anyway, it's ok to have some redundancy when it makes the user experience more convenient.

Just tried concat, it does not support that call cannot translate call CONCAT($t1, $t2). May be there is a way to support concat in future. But for now users can just do select server from sys.servers; :). Also server is the primary key of this table and is used in the join table segment_servers.

gianm · 2018-08-22T17:07:37Z

docs/content/querying/sql.md

+|------|-----|
+|server|Server name in the form host:port|
+|scheme|Server scheme http or https|
+|server_type|Type of druid service for example historical, realtime, bridge, indexer_executor|


Capitalize Druid, and this sentence needs a bit more punctuation. How about:

Type of Druid service. Possible values include: historical, realtime, bridge, and indexer_executor.

gianm · 2018-08-22T17:10:45Z

docs/content/querying/sql.md

+
+To retrieve information from segment_servers table, use the query:
+```sql
+SELECT * FROM sys.segment_servers;


Would be awesome to include an example here of a JOIN between "segments" and "servers". Maybe a query that shows the breakdown of number of segments for a specific datasource, by server. Something like grouping by server, filter by datasource, count segments.

Added an example of query using JOIN

gianm · 2018-08-22T17:11:07Z

docs/content/querying/sql.md

+
+### TASKS table
+
+The tasks table provides information about active and recently-completed indexing tasks. For more information 


"check out" not "checkout out"

gianm · 2018-08-27T08:41:38Z

sql/src/main/java/io/druid/sql/calcite/schema/SystemSchema.java

+    {
+      Object[] oldRow = null;
+      Object[] newRow = null;
+      for (Object[] row : rows) {


Danger! This is O(n^2): updateRow is called once per segment and must iterate over all rows (of which there is one per segment). We must use a different approach.

@surekhasaharan Was this comment addressed?

@jon-wei yes, the design has changed quite a lot, there is no updateRow method anymore. This was to update the num_replicas which is handled in DruidSchema now.

gianm · 2018-09-19T20:32:28Z

docs/content/querying/sql.md

@@ -468,6 +468,11 @@ plan SQL queries. This metadata is cached on broker startup and also updated per
 [SegmentMetadata queries](segmentmetadataquery.html). Background metadata refreshing is triggered by
 segments entering and exiting the cluster, and can also be throttled through configuration.

+Druid exposes system information through special system tables. There are two such schemas available: Information Schema and System Schema


Please use better grammar here: some punctuation is missing.

added the missing period, let me know if you want to restructure it further.

gianm · 2018-09-19T20:34:11Z

docs/content/querying/sql.md

@@ -519,6 +524,89 @@ SELECT * FROM INFORMATION_SCHEMA.COLUMNS WHERE TABLE_SCHEMA = 'druid' AND TABLE_
 |COLLATION_NAME||
 |JDBC_TYPE|Type code from java.sql.Types (Druid extension)|

+## SYSTEM SCHEMA
+
+The SYS schema provides visibility into Druid segments, servers and tasks.


It's "sys" now (not SYS).

gianm · 2018-09-19T23:20:43Z

server/src/main/java/org/apache/druid/discovery/DruidLeaderClient.java

+      final HttpResponseHandler<Intermediate, Final> handler
+  )
+  {
+    return httpClient.go(


No need to change this, but: IMO it would look better to write this on one line.

fixed the format

gianm · 2018-09-20T00:14:09Z

server/src/main/java/org/apache/druid/server/http/MetadataResource.java

+  @Path("/segments")
+  @Produces(MediaType.APPLICATION_JSON)
+  @ResourceFilters(DatasourceResourceFilter.class)
+  public Response getDatabaseSegmentSegments()


Maybe just getDatabaseSegments?

yes, that looks better

fjy · 2018-10-06T23:46:42Z

@jihoonson any more comments?

jihoonson · 2018-10-07T05:07:47Z

@fjy reviewing now.

jihoonson · 2018-10-07T05:17:28Z

benchmarks/src/main/java/org/apache/druid/benchmark/query/SqlBenchmark.java

+        "/simple/leader",
+        new ServerDiscoverySelector(EasyMock.createMock(ServiceProvider.class), "test")
+    )
+    {


This subclass doesn't mean empty. It just inherits all methods from the parent class.

Please check my comment below in PlannerFactory.

jihoonson · 2018-10-07T05:32:40Z

sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerFactory.java

-      final @Json ObjectMapper jsonMapper
+      final @Json ObjectMapper jsonMapper,
+      final @Coordinator DruidLeaderClient coordinatorDruidLeaderClient,
+      final @IndexingService DruidLeaderClient overlordDruidLeaderClient


Regarding my comment about mocking DruidLeaderClient in benchmark, I think the real issue is that PlannerFactory needs TimelineServerView, ObjectMapper, and DruidLeaderClients. Since this class is just a factory which is responsible for creating a new planner, it's not intuitive why it gets information from TimelineServerview or leaderClients. This makes me confused until tracking the whole change related to those parameters.

I think the same design for DruidSchema can be applied to here too. DruidSchema also needs TimelineServerView, but it's being injected and so TimelineServerView is not exposed here. Similarly, you can inject SystemSchema instead of these parameters. Actually, you've already made it injectable. Why don't you use it?

That makes sense, will pass in SystemSchema in PlannerFactory. Thanks.

jihoonson · 2018-10-07T05:34:51Z

docs/content/querying/sql.md

@@ -477,6 +477,11 @@ plan SQL queries. This metadata is cached on broker startup and also updated per
 [SegmentMetadata queries](segmentmetadataquery.html). Background metadata refreshing is triggered by
 segments entering and exiting the cluster, and can also be throttled through configuration.

+Druid exposes system information through special system tables. There are two such schemas available: Information Schema and Sys Schema.
+Information schema provides details about table and column types. The "sys" schema provides information about Druid internals like segments/tasks/servers.


Ah, sys is the real name. Got it.

jihoonson · 2018-10-07T05:35:45Z

docs/content/querying/sql.md

@@ -519,6 +524,101 @@ SELECT * FROM INFORMATION_SCHEMA.COLUMNS WHERE TABLE_SCHEMA = 'druid' AND TABLE_
 |COLLATION_NAME||
 |JDBC_TYPE|Type code from java.sql.Types (Druid extension)|

+## SYSTEM SCHEMA
+
+The sys schema provides visibility into Druid segments, servers and tasks.


Quotes look good. It makes clear that sys is the real name.

jihoonson · 2018-10-07T05:38:20Z

docs/content/querying/sql.md

+|num_rows|Number of rows in current segment, this value could be null if unkown to broker at query time|
+|is_published|True if this segment has been published to the metadata store|
+|is_available|True if this segment is currently being served by any server|
+|is_realtime|True if this segment is being served on a realtime server|


We have realtime node and realtime tasks (kafka index tasks, realtime index task). I believe you meant the all types of realtime tasks. So, True if this segment is being served on any type of realtime tasks.

jihoonson · 2018-10-07T05:38:51Z

docs/content/querying/sql.md

+|num_replicas|Number of replicas of this segment currently being served|
+|num_rows|Number of rows in current segment, this value could be null if unkown to broker at query time|
+|is_published|True if this segment has been published to the metadata store|
+|is_available|True if this segment is currently being served by any server|


I think server here is also ambiguous. Does it mean historicals and realtime tasks?

This means is_available is true for a segment, if it's being served by any server( either historical or realtime).

jihoonson · 2018-10-07T05:40:00Z

docs/content/querying/sql.md

+|host|Hostname of the server|
+|plaintext_port|Unsecured port of the server, or -1 if plaintext traffic is disabled|
+|tls_port|TLS port of the server, or -1 if TLS is disabled|
+|server_type|Type of Druid service. Possible values include: historical, realtime and indexer_executor.|


Ok. Then probably better to add a hint like indexer_executor(peon).

jihoonson · 2018-10-07T05:48:32Z

sql/src/main/java/org/apache/druid/sql/calcite/schema/SystemSchema.java

+          });
+
+      //auth check for published segments
+      final CloseableIterator<DataSegment> authorizedPublishedSegments = getAuthorizedPublishedSegments(


This should be closed when iterating is done.

jihoonson · 2018-10-07T05:53:45Z

sql/src/main/java/org/apache/druid/sql/calcite/schema/SystemSchema.java

+            }
+            catch (JsonProcessingException e) {
+              log.error(e, "Error getting segment payload for segment %s", val.getKey().getIdentifier());
+              throw new RuntimeException(e);


All exceptions should be logged, so the above logging is duplicate. Also, the exception message is more important because it's propagated to the user client if necessary as well as being logged. Please remove the log and improve the exception message.

jihoonson · 2018-10-07T06:01:34Z

sql/src/main/java/org/apache/druid/sql/calcite/schema/SystemSchema.java

+    };
+  }
+
+  static class BytesAccumulatingResponseHandler extends InputStreamResponseHandler


There are two more same classes. Please extract them to the public class and reuse it.

okay, created a common public class.

jihoonson · 2018-10-07T06:08:57Z

sql/src/main/java/org/apache/druid/sql/calcite/schema/SystemSchema.java

+        responseHandler
+    );
+    try {
+      future.get();


Ah, sorry. My question is why future needs to get here. get is called anyway in JsonParserIterator.

The query performance should be good because /druid/coordinator/v1/metadata/segments returns the StreamingOutput.

jihoonson · 2018-10-07T06:31:44Z

server/src/main/java/org/apache/druid/discovery/DruidLeaderClient.java

@@ -133,6 +134,14 @@ public FullResponseHolder go(Request request) throws IOException, InterruptedExc
    return go(request, new FullResponseHandler(StandardCharsets.UTF_8));
  }

+  public <Intermediate, Final> ListenableFuture<Final> goStream(


Please add that this method doesn't do retrying on errors or handle leader changes occurred during communication to javadoc.

jihoonson · 2018-10-07T06:49:47Z

sql/src/main/java/org/apache/druid/sql/calcite/schema/DruidSchema.java

    return rowSignatureBuilder.build();
  }
+
+  // note this is a mutable map accessed only by SystemSchema


Why does this need to be mutable? I don't see any places mutating this map.

this comment is obsolete now, this method returned segmentMetadataInfo before which is changing in this class.

jihoonson · 2018-10-07T06:55:02Z

sql/src/main/java/org/apache/druid/sql/calcite/schema/SegmentMetadataHolder.java

+    }
+  }
+
+  public void setRowSignature(RowSignature rowSignature)


Do you mean to remove the setters and always create a new SegmentMetadataHolder object when any of the member needs update. And replace the entry in segmentMetadataInfo map in DruidSchema every time update is required.

Yeah, this is what I meant. I think reducing the scope of concurrent access is more important here than what kind of lock types we use. As I commented here, the concurrency control algorithm is spanned over multiple places, which makes us difficult to understand and makes potential bugs even in the future.

jihoonson · 2018-10-09T12:29:34Z

docs/content/querying/sql.md

+|num_rows|Number of rows in current segment, this value could be null if unkown to broker at query time|
+|is_published|True if this segment has been published to the metadata store|
+|is_available|True if this segment is currently being served by any server(historical or realtime)|
+|is_realtime|True if this segment is being served on any type of realtime tasks|


Please check the doc for these variables. Probably they should long instead of boolean?

good point, changed the doc

jihoonson · 2018-10-09T12:42:10Z

sql/src/main/java/org/apache/druid/sql/calcite/schema/SystemSchema.java

+            }
+          });
+      try {
+        authorizedPublishedSegments.close();


authorizedPublishedSegments is an iterator and the transform of publishedSegments is executed lazily. So, closing authorizedPublishedSegments here means that authorizedPublishedSegments is closed even before starting the iteration.

authorizedPublishedSegments.close() should be called when the iteration on Linq4j.asEnumerable(allSegments).where(t -> t != null) is finished. But, I'm not sure what the best way is. @gianm any idea?

Yes, i was not totally sure where to close authorizedPublishedSegments. I'll remove it from here and check what is the right place.

surekhasaharan · 2018-10-10T16:46:50Z

Thanks so much @jihoonson for the review. I've addressed your comments, let me know if you have any further comments.

jihoonson

The latest change looks good to me. @surekhasaharan thanks!

fjy · 2018-10-11T00:17:25Z

Spoke with @gianm about if he has any further comments and he does not so I am moving forward with merging.

leventov · 2018-10-11T17:49:54Z

sql/src/test/java/org/apache/druid/sql/calcite/schema/SystemSchemaTest.java

+      new DruidServerMetadata("server2", "server2:1234", null, 5L, ServerType.HISTORICAL, DruidServer.DEFAULT_TIER, 0),
+      1L,
+      null,
+      ImmutableMap.of("segment2", segment2, "segment4", segment4, "segment5", segment5)


Shouldn't it use segment3

This is segment2 because in the test later, it asserts for 2 replicas for segment2. So I was trying to mimic the behavior that segment2 is served by both the servers druidServer1 and druidServer2.

It is expected that when I change the code here to "segment3", segment3, all tests in this class still pass?

Tests did pass with segment3, I checked again and the 2 replicas for segment2 are actually coming from this part , so yes it can be changed to segment3.Will do.

hm, I think this suite needs a test for SystemSchema.ServerSegmentsTable that checks expected server/segment mappings, this test would catch that segment2->segment3 change

you make a great point @jon-wei , I thought server_segments is not that interesting or complex, so skipped the test. Adding the test in this PR. Since I switched segment2->segment3, that seems more cleaner. This new test will now catch segment3->segment2 change.

PR comment from apache#6094 (review)

* Add check for nullable numRows * Make numRows long instead of Long type * Add check for numRows in unit test * small refactoring * Modify test PR comment from #6094 (review) * Add a test for serverSegments table * update tests

leventov · 2019-01-09T04:38:06Z

sql/src/test/java/org/apache/druid/sql/calcite/schema/SystemSchemaTest.java

+    walker = new SpecificSegmentsQuerySegmentWalker(conglomerate)
+        .add(segment1, index1)
+        .add(segment2, index2)
+        .add(segment2, index2)


Duplicate line

@leventov This is removed here It was because I thought about the replicas in a different way(wrong way), fixed in this new PR.

Surekha Saharan added 5 commits July 31, 2018 17:32

Added SystemSchema with following tables (apache#5989)

44c6337

* SEGMENTS table provides details on served and published segments * SERVERS table provides details on data servers * SERVERSEGMETS table is the JOIN of SEGMENTS and SERVERS * TASKS table provides details on tasks

Add documentation for system schema

52b6115

Merge branch 'master' of github.com:druid-io/druid into system-table

335fc2b

Fix static-analysis warnings

4f34202

Merge branch 'master' of github.com:druid-io/druid into system-table

59e996b

fjy added this to the 0.13.0 milestone Aug 13, 2018

Merge branch 'master' of github.com:druid-io/druid into system-table

7991720

gianm requested changes Aug 16, 2018

View reviewed changes

Surekha Saharan added 18 commits August 19, 2018 10:58

Merge branch 'master' of github.com:druid-io/druid into system-table

3fd41de

Address PR comments

816552e

*Add unit tests

Merge branch 'master' of github.com:druid-io/druid into system-table

d74040c

Fix a test

a8038ee

Try to fix a test

456a0ad

Fix a bug around replica count

5728364

Merge branch 'master' of github.com:druid-io/druid into system-table

05fd4ce

Merge branch 'master' of github.com:druid-io/druid into system-table

cec8737

Merge branch 'master' of github.com:druid-io/druid into system-table

54dd64c

Merge branch 'master' of github.com:druid-io/druid into system-table

c99f027

rename io.druid to org.apache.druid

7a57b1e

Merge branch 'master' of github.com:druid-io/druid into system-table

3cb0f52

Major change is to make tasks and segment queries streaming

cf18959

* Made tasks/segments stream to calcite instead of storing it in memory * Add num_rows to segments table * Refactor JsonParserIterator * Replace with closeable iterator

Fix docs, make num_rows column nullable, some unit test changes

68d45a0

Merge branch 'master' of github.com:druid-io/druid into system-table

0239f94

make num_rows column type long, allow it to be null

4e3b013

fix a compile error after merge, add TrafficCop param to InputStreamResponseHandler

Filter null rows for segments table from Linq4j enumerable

495883a

change num_replicas datatype to long in segments table

b6fe553

gianm reviewed Sep 20, 2018

View reviewed changes

Fix some tests and address comments

bab61c6

Surekha Saharan added 2 commits October 5, 2018 12:12

Fix style for tests

3fbbdc6

Merge branch 'master' of github.com:druid-io/druid into system-table

ccc7f18

jihoonson reviewed Oct 7, 2018

View reviewed changes

Surekha Saharan added 7 commits October 8, 2018 00:34

PR comments

23112a5

Merge branch 'master' of github.com:druid-io/druid into system-table

b84d728

Address PR comments

3cd1722

Merge branch 'master' of github.com:druid-io/druid into system-table

1022693

Add the missing BytesAccumulatingResponseHandler class

d63469d

Use Sequences.withBaggage in DruidPlanner

9f396aa

Merge branch 'master' of github.com:druid-io/druid into system-table

e0657e5

jihoonson reviewed Oct 9, 2018

View reviewed changes

Surekha Saharan added 3 commits October 9, 2018 11:17

Fix docs, add comments

83c74fe

Merge branch 'master' of github.com:druid-io/druid into system-table

1873c92

Close the iterator if hasNext returns false

892ee80

dclim mentioned this pull request Oct 10, 2018

Druid 0.13.0-incubating release notes #6442

Closed

jihoonson approved these changes Oct 11, 2018

View reviewed changes

fjy merged commit 3a0a667 into apache:master Oct 11, 2018

leventov reviewed Oct 11, 2018

View reviewed changes

surekhasaharan deleted the system-table branch October 12, 2018 17:35

surekhasaharan pushed a commit to surekhasaharan/druid that referenced this pull request Oct 13, 2018

Modify test

7a13dc9

PR comment from apache#6094 (review)

leventov reviewed Jan 9, 2019

View reviewed changes

clintropolis mentioned this pull request Jul 31, 2019

Fix is_realtime column behavior in sys.segments table #8154

Merged

4 tasks


		### TASKS table

		The tasks table provides information about active and recently-completed indexing tasks. For more information

Introduce SystemSchema tables (#5989) #6094

Introduce SystemSchema tables (#5989) #6094

Conversation

surekhasaharan commented Aug 1, 2018 • edited

vogievetsky commented Aug 2, 2018

fjy commented Aug 13, 2018

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

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

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

gianm 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

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

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

Choose a reason for hiding this comment

fjy commented Oct 6, 2018

jihoonson commented Oct 7, 2018

Choose a reason for hiding this comment

jihoonson Oct 7, 2018 • edited

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surekhasaharan commented Aug 1, 2018 •

edited

jihoonson Oct 7, 2018 •

edited