STORM-2406 [Storm SQL] Change underlying API to Streams API #2443

HeartSaVioR · 2017-12-04T12:01:31Z

This will enable us to provide windowed aggregation, join, etc later.
- Tuple-to-tuple is making more sense than micro-batch in these cases
Tested with several sql cases
Also bump Calcite version to 1.14.0

Even the patch looks like huge diff, most of things are rename classes/move packages.
I've also fixed some checkstyle issues.

HeartSaVioR · 2017-12-04T12:10:17Z

While working with Streams API, I feel the needs for Source (typed Spout) and Sink (typed Sink Bolt) so have been thinking about interfaces/abstract classes, but due to ACK mechanism and various approaches of implementing external sink bolts, hiding concept of Tuple doesn't look like easy.

I have another wondering case to windowing with timestamp field: Streams API doesn't expose field name, but Streams API still receive field name for timestamp field. It may need to be addressed before STORM-2405 (support windowing aggregation in SQL).

HeartSaVioR · 2017-12-04T12:15:10Z

sql/storm-sql-core/src/jvm/org/apache/storm/sql/planner/streams/rel/StreamsStreamInsertRel.java

-                .partitionPersist(consumer.getStateFactory(), new Fields(inputFields), consumer.getStateUpdater(),
-                        new Fields(outputFields))
-                .newValuesStream().name(stageName);
+        inputStream.mapToPair(new StreamInsertMapToPairFunction(primaryKeyIndex)).to(consumer);


To make logic simple, it assumes that all the tables have one PK (which it should be extended to support composed key), and provides PairStream(KeyedStream) to consumer bolt.

HeartSaVioR · 2017-12-04T12:20:03Z

sql/storm-sql-core/src/jvm/org/apache/storm/sql/planner/streams/rel/StreamsStreamInsertRel.java


        Preconditions.checkArgument(isInsert(), "Only INSERT statement is supported.");

-        List<String> inputFields = this.input.getRowType().getFieldNames();
-        List<String> outputFields = getRowType().getFieldNames();
+        // Calcite ensures that the value is structurized to the table definition


To elaborate, if table BAR is defined as ID INTEGER PK, NAME VARCHAR, DEPTID INTEGER and query like INSERT INTO BAR SELECT NAME, ID FROM FOO is executed, Calcite makes the projection ($1 <- ID, $0 <- NAME, null) to the value before INSERT.

srdo

I'm not really familiar with Storm SQL, so I think someone more familiar with it should also review, but I left a few suggestions.

srdo · 2017-12-04T18:46:51Z

sql/storm-sql-core/pom.xml

@@ -66,10 +66,34 @@
            <artifactId>calcite-core</artifactId>
            <version>${calcite.version}</version>
            <exclusions>
+                <exclusion>


Why the exclusions?

They're for supporting another parts of Calcite and unneeded dependencies for us. Some dependencies are something already we have, so also good to avoid the version conflict.

Makes sense if we don't need them.

If you're trying to avoid version conflicts for transitive dependencies I think adding a <dependencyManagement> section to set the versions is better. It lets you set which version of the dependency you want, and then all transitive includes of that dependency will be the specified version. I think we do this with e.g. the Jackson dependencies in the Storm parent pom.

srdo · 2017-12-06T18:28:10Z

sql/storm-sql-core/src/jvm/org/apache/storm/sql/calcite/ParallelTable.java

+public interface ParallelTable extends StormTable {
+
+    /**
+     * Returns parallelism hint of this table. Returns null if don't know.


nit: Returns null if not known.

srdo · 2017-12-06T18:29:57Z

sql/storm-sql-core/pom.xml

@@ -66,10 +66,34 @@
            <artifactId>calcite-core</artifactId>


You might want to reduce the allowed checkstyle errors in the poms if you reduced the violations.

I'll see how the number is changed. It might be remain the same if I fixed checkstyle issue from A and violated from B.
Actually I'm focusing on planning and addressing window aggregation and join, so TBH I'd like to address the checkstyle issue later altogether.

Sure. I didn't mean to say that you should do any extra work on checkstyle in this, just wanted to make sure reducing the counter wasn't being forgotten by accident.

Good. Thanks for the reminder. :)

srdo · 2017-12-06T18:30:50Z

sql/storm-sql-core/src/jvm/org/apache/storm/sql/compiler/CompilerUtil.java

@@ -117,9 +138,21 @@ public TableBuilderInfo parallelismHint(int parallelismHint) {
      return this;
    }

+    // FIXME: we may want to separate Stream and Table, and let output table be Table instead of Stream


Can you put this in an issue instead? I think it will be forgotten if left as a TODO in the code.

OK I'll file an issue and remove the comment.

https://issues.apache.org/jira/browse/STORM-2848

srdo · 2017-12-06T18:32:14Z

sql/storm-sql-core/src/jvm/org/apache/storm/sql/compiler/CompilerUtil.java

+      return this;
+    }
+
+    private void interpretConstraint(ColumnConstraint constraint, int fieldIdx) {
      if (constraint instanceof ColumnConstraint.PrimaryKey) {
        ColumnConstraint.PrimaryKey pk = (ColumnConstraint.PrimaryKey) constraint;
        Preconditions.checkState(primaryKey == -1, "There are more than one primary key in the table");


Nit: There are -> There is

srdo · 2017-12-07T13:08:30Z

...ternal/storm-sql-kafka/src/test/org/apache/storm/sql/kafka/TestKafkaDataSourcesProvider.java

-            ByteBuffer b = SERIALIZER.write(tuple.getValues(), null);
-            return b.equals(buf);
-        }
+        Assert.assertEquals(KafkaBolt.class, consumer.getClass());


Same question as for the HDFS test

Same answer.

srdo · 2017-12-07T13:10:02Z

...xternal/storm-sql-mongodb/src/jvm/org/apache/storm/sql/mongodb/MongoDataSourcesProvider.java

+            String serField;
+            if (props.contains("ser.field")) {
+                serField = props.getProperty("ser.field");
+            } else if (props.contains("trident.ser.field")) {


Nit: Are there constants somewhere that could be used instead of literals?

I think I missed it. Nice finding. Will address. Btw it is specific to mongo config, so constants will be put in same class.

srdo · 2017-12-07T13:15:07Z

...torm-sql-runtime/src/jvm/org/apache/storm/sql/runtime/datasource/socket/bolt/SocketBolt.java

+import org.slf4j.LoggerFactory;
+
+/**
+ * The Bolt implementation for Socket. The class is for test purpose and doesn't handle reconnection or so.


What is meant by "or so"?

Also if this class is only for tests, can it be moved to test scope? And if not, can it be moved into a package for test classes so it's clear that it's only for tests?

I think removing "or so" is clearer. What I meant for test is not unit test, just for users to test query before attaching external source. Maybe we can just remove mentioning about test purpose and mention that it doesn't handle reconnection.

srdo · 2017-12-07T13:16:52Z

...rm-sql-runtime/src/jvm/org/apache/storm/sql/runtime/datasource/socket/spout/SocketSpout.java

 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;

 /**
- * Trident Spout for Socket data. Only available for Storm SQL, and only use for test purposes.
+ * Spout for Socket data. Only available for Storm SQL. This doesn't guarantee at-least-once.


Is it now intended for non-test use?

Same thing for spout. I think I need to add same comment from bolt and also mentioning that it doesn't support replaying so at-most-once semantic.

I found myself dumb, I already implemented replay of messages.

srdo · 2017-12-07T13:31:16Z

sql/storm-sql-runtime/src/test/org/apache/storm/sql/TestUtils.java

    }
  }

-  public static class MockStateUpdater implements StateUpdater<MockState> {
+  public static class MockBolt extends BaseRichBolt {


I think it could be helpful to use a JUnit TestRule (e.g. by subclassing http://junit.org/junit4/javadoc/4.12/org/junit/rules/ExternalResource.html) to help remember to clear VALUES after the test. It's easy to forget to clean up, and then the tests leak state.

Same for the other test utils that keep static state.

Thanks for the information. I'll try to apply on that.

HeartSaVioR · 2017-12-07T23:56:31Z

@srdo Thanks again for the detailed review. Addressed review comments.

HeartSaVioR · 2017-12-15T13:38:01Z

Rebased to apply dropping standalone mode.

srdo · 2018-07-06T16:08:13Z

@HeartSaVioR Are you still planning on getting this in? I could probably put some time in to trying this out if no one else is willing to test/use storm-sql, so we can get a +1.

HeartSaVioR · 2018-07-09T08:12:15Z

@srdo
Yeah, thanks for reminding this.

Applying the change manually again would be easier than rebasing, since there were nice efforts on storm-sql (checkstyle violation reduction, test config). It would take some efforts to apply the change without messing up current effort.

I'll mention you when it's ready. Thanks again!

HeartSaVioR · 2018-07-13T08:38:31Z

@srdo
Rebased. I'm seeing intermittent test failure like below, but not consistent failure. Will try to take a look at once I have time to, but let's move it out of this PR.

17:32:06.845 [SLOT_1027] INFO  o.a.s.m.StormMetricRegistry - Starting metrics reporters...
17:32:06.845 [SLOT_1027] INFO  o.a.s.s.a.ClientAuthUtils - Got AutoCreds []
17:32:06.846 [SLOT_1027] INFO  o.a.s.d.w.WorkerState - Reading assignments
17:32:06.846 [SLOT_1027] ERROR o.a.s.d.s.Slot - Error when processing event
java.io.IOException: java.lang.NullPointerException
	at org.apache.storm.daemon.supervisor.LocalContainer.launch(LocalContainer.java:54) ~[storm-server-2.0.0-SNAPSHOT.jar:2.0.0-SNAPSHOT]
	at org.apache.storm.daemon.supervisor.LocalContainerLauncher.launchContainer(LocalContainerLauncher.java:42) ~[storm-server-2.0.0-SNAPSHOT.jar:2.0.0-SNAPSHOT]
	at org.apache.storm.daemon.supervisor.Slot.handleWaitingForBlobUpdate(Slot.java:528) ~[storm-server-2.0.0-SNAPSHOT.jar:2.0.0-SNAPSHOT]
	at org.apache.storm.daemon.supervisor.Slot.stateMachineStep(Slot.java:232) ~[storm-server-2.0.0-SNAPSHOT.jar:2.0.0-SNAPSHOT]
	at org.apache.storm.daemon.supervisor.Slot.run(Slot.java:902) [storm-server-2.0.0-SNAPSHOT.jar:2.0.0-SNAPSHOT]
Caused by: java.lang.NullPointerException
	at org.apache.storm.daemon.worker.WorkerState.readWorkerExecutors(WorkerState.java:630) ~[storm-client-2.0.0-SNAPSHOT.jar:2.0.0-SNAPSHOT]
	at org.apache.storm.daemon.worker.WorkerState.<init>(WorkerState.java:153) ~[storm-client-2.0.0-SNAPSHOT.jar:2.0.0-SNAPSHOT]
	at org.apache.storm.daemon.worker.Worker.loadWorker(Worker.java:172) ~[storm-client-2.0.0-SNAPSHOT.jar:2.0.0-SNAPSHOT]
	at org.apache.storm.daemon.worker.Worker.lambda$start$39(Worker.java:164) ~[storm-client-2.0.0-SNAPSHOT.jar:2.0.0-SNAPSHOT]
	at java.security.AccessController.doPrivileged(Native Method) ~[?:1.8.0_66]
	at javax.security.auth.Subject.doAs(Subject.java:422) ~[?:1.8.0_66]
	at org.apache.storm.daemon.worker.Worker.start(Worker.java:163) ~[storm-client-2.0.0-SNAPSHOT.jar:2.0.0-SNAPSHOT]
	at org.apache.storm.daemon.supervisor.LocalContainer.launch(LocalContainer.java:52) ~[storm-server-2.0.0-SNAPSHOT.jar:2.0.0-SNAPSHOT]
	... 4 more
17:32:06.847 [SLOT_1027] ERROR o.a.s.u.Utils - Halting process: Error when processing an event
java.lang.RuntimeException: Halting process: Error when processing an event
	at org.apache.storm.utils.Utils.exitProcess(Utils.java:473) [storm-client-2.0.0-SNAPSHOT.jar:2.0.0-SNAPSHOT]
	at org.apache.storm.daemon.supervisor.Slot.run(Slot.java:949) [storm-server-2.0.0-SNAPSHOT.jar:2.0.0-SNAPSHOT]

HeartSaVioR · 2018-07-13T08:43:04Z

docs/storm-sql-internal.md

@@ -1,59 +0,0 @@
---


I removed the document since it is documented based on Trident implementation. There're other docs for SQL example or SQL reference.

I'm wondering if it would be worth updating this rather than removing it, or maybe merging parts of it into one of the other storm-sql docs (e.g. the overview doc). I think the page provides a nice overview of what storm-sql is, and how it works.

@srdo
Agreed. Regarding updating the doc, I guess most parts of content are correct, but unfortunately I don't have resource for diagram so required to redraw one. I'd rather explain in content to avoid redraw one (like "Now Trident topology in a diagram is replaced to normal Storm topology"). What do you think?

We could even do some photo editing but not sure it is easy to do since I don't know which font is used for storm-sql-internal-workflow.png.

It would probably be pretty quick to duplicate with e.g. https://www.draw.io/, since it's just some boxes.

Yeah right. I didn't think about that. Drawing a new one.

Just finished it.

Thanks, looks great. I think the dotted line box is missing the "StormSQL" label. Also maybe we should put the draw.io source file in the docs directory too, so we don't have to remake the diagram again if we need to change it later.

The second diagram on the page also mentions Trident. I don't know if it makes sense to update, or just remove it.

Nice finding. Will fix it and also will attach source file of draw.io, but not sure where to add.
I'll draw for second diagram as well.

srdo

Looks pretty good to me. My only concern is the reduction in test coverage because the bolts in e.g. storm-sql-hdfs can't be stubbed, but as mentioned it's up to you whether you think it makes sense to try to preserve the tests or not.

I'll try checking this out and try out an example or two.

srdo · 2018-07-13T12:59:39Z

docs/storm-sql-internal.md

@@ -1,59 +0,0 @@
---


I'm wondering if it would be worth updating this rather than removing it, or maybe merging parts of it into one of the other storm-sql docs (e.g. the overview doc). I think the page provides a nice overview of what storm-sql is, and how it works.

srdo · 2018-07-13T13:14:07Z

docs/storm-sql.md

@@ -6,14 +6,14 @@ documentation: true

 The Storm SQL integration allows users to run SQL queries over streaming data in Storm. Not only the SQL interface allows faster development cycles on streaming analytics, but also opens up the opportunities to unify batch data processing like [Apache Hive](///hive.apache.org) and real-time streaming data analytics.

-At a very high level StormSQL compiles the SQL queries to [Trident](Trident-API-Overview.html) topologies and executes them in Storm clusters. This document provides information of how to use StormSQL as end users. For people that are interested in more details in the design and the implementation of StormSQL please refer to the [this](storm-sql-internal.html) page.
+At a very high level StormSQL compiles the SQL queries to Storm topologies leveraging Streams API and executes them in Storm clusters. This document provides information of how to use StormSQL as end users. For people that are interested in more details in the design and the implementation of StormSQL please refer to the [this](storm-sql-internal.html) page.


There's a link to storm-sql-internal in this line

srdo · 2018-07-13T13:16:03Z

sql/README.md

-  - Not across batches.
-  - Limitation came from `join` feature of Trident.
-  - Please refer this doc: `Trident API Overview` for details.
+Please see [here](../../docs/storm-sql.md) for details on how to use it.


Isn't this link going down by too many directories?

Yeah my bad. Will fix.

srdo · 2018-07-13T13:25:37Z

sql/storm-sql-core/src/jvm/org/apache/storm/sql/compiler/CompilerUtil.java

                }

                @Override
                public Schema.TableType getJdbcTableType() {
                    return Schema.TableType.STREAM;
                }
+
+                @Override
+                public boolean isRolledUp(String s) {


Nit: s doesn't really say much as a variable name, can we replace it with one that says what this string is? Same for the other methods here.

OK will find origin methods and restore its origin names.

srdo · 2018-07-13T13:29:13Z

sql/storm-sql-core/src/jvm/org/apache/storm/sql/planner/streams/StreamsStormRuleSets.java

+            StreamsScanRule.INSTANCE,
+            StreamsFilterRule.INSTANCE,
+            StreamsProjectRule.INSTANCE,
+            StreamsAggregateRule.INSTANCE,


Are we holding on to aggregate and join so they'll be easy to implement later, or why are the aggregate and join rules here?

This is to have control to warn to the end users when aggregate or join is used in users' query. I didn't test if we don't have our own rule and user query trigger the feature. Yeah it is also good to not forget about missing features.

Makes sense.

srdo · 2018-07-13T13:49:46Z

...-external/storm-sql-hdfs/src/test/org/apache/storm/sql/hdfs/TestHdfsDataSourcesProvider.java

-        tupleList.add(t0);
-        tupleList.add(t1);
-        return tupleList;
+        Assert.assertEquals(HdfsBolt.class, consumer.getClass());


Looking at this again, I think the way to do it would be to use a factory like https://github.com/apache/storm/blob/master/external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/internal/ConsumerFactory.java to instantiate the HdfsBolt. You could pass the factory class name in through a property and instantiate it when the data source is created. The default factory would just call new HdfsBolt().

I'm not sure if it's worth it. Up to you.

Thanks for the information! I'll think about applying it but let me address other comments first.

srdo · 2018-07-13T13:53:56Z

...xternal/storm-sql-mongodb/src/jvm/org/apache/storm/sql/mongodb/MongoDataSourcesProvider.java

+                serField = props.getProperty(VALUE_SERIALIZED_FIELD);
+            } else if (props.contains(TRIDENT_VALUE_SERIALIZED_FIELD)) {
+                // backward compatibility
+                serField = props.getProperty(TRIDENT_VALUE_SERIALIZED_FIELD);


Nit: Since this is targeting 2.0.0, it might be okay not to provide backward compatibility.

Yeah right, but it has been existing one and easy to support backward compatibility, so doesn't matter if we support it. We didn't deprecate it either.

Sure, that makes sense.

srdo · 2018-07-13T13:57:02Z

...torm-sql-runtime/src/jvm/org/apache/storm/sql/runtime/datasource/socket/bolt/SocketBolt.java

+ * The Bolt implementation for Socket. Only available for Storm SQL.
+ * The class doesn't handle reconnection so you may not want to use this for production.
+ */
+public class SocketBolt implements IRichBolt {


Nit: Consider extending BaseRichBolt instead, so you don't have to implement the methods you don't want to customize

Yeah nice finding! Will fix.

srdo · 2018-07-13T13:58:26Z

...torm-sql-runtime/src/jvm/org/apache/storm/sql/runtime/datasource/socket/bolt/SocketBolt.java

+
+/**
+ * The Bolt implementation for Socket. Only available for Storm SQL.
+ * The class doesn't handle reconnection so you may not want to use this for production.


Does it make sense for us to include code like this if we know it isn't suitable for production use?

You could do some experiments with your production query via replacing input table and/or output table to socket.

We may also want to have console output table (it can't be input table for sure) but we could address it from follow-up issue.

Please refer others' cases: Spark has couple of input sources and output sinks for testing, whereas Flink doesn't look like providing table sources and table sinks for testing. I'd rather feel better on Spark for this case.

http://spark.apache.org/docs/latest/structured-streaming-programming-guide.html#input-sources
http://spark.apache.org/docs/latest/structured-streaming-programming-guide.html#output-sinks

https://ci.apache.org/projects/flink/flink-docs-release-1.5/dev/table/sourceSinks.html

Okay. It's fine to keep it, just wanted to make sure we didn't want to e.g. move it to /test

srdo · 2018-07-13T14:02:55Z

sql/storm-sql-runtime/src/test/org/apache/storm/sql/TestUtils.java

+
+    @Override
+    protected void after() {
+      // no-op


Nit: I think you can leave out this method entirely

srdo · 2018-07-14T14:12:29Z

Tried out this branch with a storm-sql-kafka example, it seems to work fine. I hit a couple of newbie traps, will make sure to update the docs to help other people avoid them.

The only change in behavior I noticed is that certain debug logs (e.g.

storm/sql/storm-sql-runtime/src/jvm/org/apache/storm/sql/runtime/trident/functions/EvaluationFilter.java

Line 56 in c9e9a7c

    
           LOG.info("Expression code for filter: \n{}", ((DebuggableExecutableExpression) filterInstance).getDelegateCode());

) are being printed to console during topology submit, rather than to the worker log. I don't think it's an issue, it was just a little surprising.

HeartSaVioR · 2018-07-16T10:30:45Z

@srdo
Yes, since Streams API doesn't have rich function like executor side initialization for now. We could address it but maybe better to file a new issue and handle it independently. What do you think?

srdo · 2018-07-16T11:35:29Z

I don't think we need to change it. I just wanted to make a note of it. When this has been merged, we might want to add a couple of notes about debugging to the docs, we can mention the log line there.

srdo · 2018-07-16T11:43:36Z

The test failures look unrelated, not sure why it's suddenly failing. If they don't go away by rebasing to master, we might want to raise issues to track them.

I don't know if you're looking at #2443 (comment), but we can probably make do without the tests for now. If so, it would be good to raise an issue to reintroduce them, the current tests are very minimal.

+1

HeartSaVioR · 2018-07-16T12:43:55Z

Yes test failures look unrelated, one is from server, another one is from cassandra.

I'd like to address #2443 (comment) but OK to address it with new issue, since smaller patch would be easier to merge.

srdo · 2018-07-16T12:56:17Z

Sounds good.

HeartSaVioR · 2018-07-16T13:42:39Z

Raised https://issues.apache.org/jira/browse/STORM-3153 for addressing restoring tests.

HeartSaVioR · 2018-07-16T14:35:39Z

@srdo
I have exported these diagrams into xml : please download and rename below files to remove .txt (github doesn't allow uploading xml file directly) and see they're properly imported in your account of draw.io.

storm-sql-internal-example.xml.txt
storm-sql-internal-workflow.xml.txt

If it works, I'll add these files into image directory or other directory (not sure) as well.

srdo · 2018-07-16T14:51:59Z

storm-sql-internal-example.xml(1).txt

They work fine for me. The example diagram had a typo (Fliter).

HeartSaVioR · 2018-07-16T15:18:16Z

@srdo Nice finding. Fixed. Also exported to xml once again. Please check again. Thanks for your patience!
storm-sql-internal-example.xml.txt

srdo · 2018-07-16T15:29:03Z

Looks great.

HeartSaVioR · 2018-07-16T20:58:16Z

@srdo I'd like to get final review and approval from you to check if I miss anything before merging. Could you please help to do this? Thanks!

srdo · 2018-07-16T21:41:34Z

I think all my comments were addressed, thanks for your patience. LGTM, +1.

HeartSaVioR · 2018-07-16T21:48:26Z

@srdo Thanks for the detailed review and nice suggestions!

* This will enable us to provide windowed aggregation, join, etc. * Tuple-to-tuple is making more sense than micro-batch in these cases * Tested with several sql cases * Also bump Calcite version to 1.14.0 * Fix checkstyle issues a bit: not doing exhaustively * Update diagrams in storm-sql internal doc * Also add XML type of exported diagrams as well to restore diagram from draw.io

HeartSaVioR commented Dec 4, 2017

View reviewed changes

HeartSaVioR changed the title ~~ISSUE-2406 [Storm SQL] Change underlying API to Streams API~~ STORM-2406 [Storm SQL] Change underlying API to Streams API Dec 5, 2017

HeartSaVioR force-pushed the STORM-2406 branch from e9f7ff6 to 42b6a50 Compare December 5, 2017 06:51

srdo reviewed Dec 7, 2017

View reviewed changes

HeartSaVioR force-pushed the STORM-2406 branch from 0f85cf7 to b31d63e Compare December 8, 2017 00:12

HeartSaVioR force-pushed the STORM-2406 branch 2 times, most recently from c320c49 to 8fe6beb Compare December 15, 2017 13:36

HeartSaVioR force-pushed the STORM-2406 branch from 8fe6beb to 93f9142 Compare July 13, 2018 08:07

HeartSaVioR commented Jul 13, 2018

View reviewed changes

srdo reviewed Jul 13, 2018

View reviewed changes

HeartSaVioR force-pushed the STORM-2406 branch from fb0eb3f to c69a23c Compare July 16, 2018 21:59

asfgit merged commit c69a23c into apache:master Jul 16, 2018

STORM-2406 [Storm SQL] Change underlying API to Streams API #2443

STORM-2406 [Storm SQL] Change underlying API to Streams API #2443

Conversation

HeartSaVioR commented Dec 4, 2017

HeartSaVioR commented Dec 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srdo 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

HeartSaVioR Dec 7, 2017 • 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

HeartSaVioR commented Dec 7, 2017

HeartSaVioR commented Dec 15, 2017

srdo commented Jul 6, 2018

HeartSaVioR commented Jul 9, 2018

HeartSaVioR commented Jul 13, 2018

HeartSaVioR Jul 13, 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

srdo 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

srdo commented Jul 14, 2018 • edited

HeartSaVioR commented Jul 16, 2018

srdo commented Jul 16, 2018

srdo commented Jul 16, 2018

HeartSaVioR commented Jul 16, 2018

HeartSaVioR Dec 7, 2017 •

edited

HeartSaVioR Jul 13, 2018 •

edited

srdo commented Jul 14, 2018 •

edited

HeartSaVioR commented Jul 16, 2018 •

edited