[issue-48] Builder API #125

EronWright · 2018-04-28T05:15:22Z

Signed-off-by: Eron Wright [email protected]

Change log description

introduced PravegaConfig to capture high-level client configuration
general stream cut support
support for environment-provided default Pravega scope
builder for FlinkPravegaInputFormat
builder for FlinkPravegaReader/FlinkPravegaWriter
rename FlinkPravegaReaderTest to FlinkPravegaReaderITCase
unit test for FlinkPravegaReader/FlinkPravegaWriter
implement batch table API support
builder for FlinkPravegaTableSource/FlinkPravegaTableSink
introduce FlinkPravegaJsonTableSource/FlinkPravegaJsonTableSink
treat FlinkPravegaTableSource/FlinkPravegaTableSink as an internal class
introduce FlinkPravegaTableSinkTest
removed StreamId in favor of io.pravega.client.stream.Stream
removed FlinkPravegaParams in favor of PravegaConfig, builder classes
moved IntegerDeserializationSchema to utils
implemented StreamSourceOperatorTestHarness

Purpose of the change
Closes #48 (builder API), #68 (reader unit test), and #62 (TableSource batch API)

What the code does
This PR introduces a new builder API for the stream, batch, and table connectors, and (trivially) completes the table batch API integration. The builder API expands the capabilities of the connector to include support for credentials and stream cuts across all connectors. It also supports environment variables/system properties for configuring the controller URI and the default scope.

Note this is a breaking change.

How to verify it
New and revised unit tests, notably:

FlinkPravegaReaderTest
FlinkPravegaWriterTest
PravegaConfigTest
FlinkPravegaTableSourceTest
FlinkPravegaTableSinkTest

Documentation (Draft)

Here's some details about the proposed API. Broadly speaking, there's an API for Pravega context (e.g. controller URI, credentials), an API for the various types of readers, and an API for the various types of writers.

For readers, we're providing a builder() for FlinkPravegaReader (streaming), FlinkPravegaInputFormat (batch), and FlinkPravegaTableSource (unified), in lieu of the constructors provided today on those classes. The builders have an inheritance hierarchy, to unify the common elements.

Connecting to Pravega

PravegaConfig

A top-level config object, PravegaConfig, is provided to establish a Pravega context. It synthesizes the context from environment variables, system properties, command-line arguments, and explicit settings.

Note the configuration of a default scope, which allows unqualified stream names to be used later. An environment variable is also supported - PRAVEGA_SCOPE - to make it easy for Nautilus to apply a default scope per project.

// minimal
PravegaConfig config = PravegaConfig.fromDefaults();

// with command-line parameters parsed by Flink's ParameterTool class:
ParameterTool params = ParameterTool.fromArgs(args);
PravegaConfig config = PravegaConfig.fromParams(params);

// explicit settings
PravegaConfig config = PravegaConfig.fromDefaults()
    .withControllerURI("tcp://...")
    .withDefaultScope("scope-1")
    .withCredentials(credentials)
    .withHostnameValidation(false);

Reading a Stream

Reader-Builder (Common)

All the reader-builders across batch, streaming, and table extend the AbstractReaderBuilder, which focuses on which streams will be read (with optional cuts). Unqualified stream names are resolved against the PravegaConfig.

AbstractReaderBuilder.builder()
    .forStream("stream-1")    // unbounded
    .forStream("stream-1", fromCut)    // bounded
    .forStream("scope-1/stream-1")    // qualified stream name
    .forStream(Stream.of("scope-1/stream-1"))   // typed stream identifier
    .withPravegaConfig(...)    // use a config from above (default: PravegaConfig.fromDefaults())
    .build();

Batch API

Create a FlinkPravegaInputFormat for the Batch API:

FlinkPravegaInputFormat.<Integer> inputFormat = FlinkPravegaInputFormat.<Integer>builder()
    .forStream(...)
    .withPravegaConfig(config)
    .withDeserializationSchema(...)
    .build();

Streaming API

The streaming reader (based on a Pravega reader group and a Flink checkpoint hook) is actually used in both the DataStream API and the Table API. To support this, another abstract reader-builder, AbstractStreamingReaderBuilder, is introduced that extends AbstractReaderBuilder:

AbstractStreamingReaderBuilder.builder()
    .uid(...)   // hook state identifier (default: generated)
    .withReaderGroupScope(...)   // override of RG scope (default: PravegaConfig's default scope)
    .withReaderGroupName(...)   // RG stream name
    .withReaderGroupRefreshTime(...)  
    .withCheckpointInitiateTimeout(...) 
    .withEventReadTimeout(...)

Each concrete reader has a builder that extends the appropriate abstract builder.

Create a FlinkPravegaReader for the DataStream API:

FlinkPravegaReader<Integer> pravegaSource = FlinkPravegaReader.<Integer>builder()
    .forStream(...)
    .withPravegaConfig(config)
    .withDeserializationSchema(...)
    .build();

StreamExecutionEnvironment env = StreamExecutionEnvironment.getExecutionEnvironment();
env.addSource(pravegaSource);

Table API

The Table API provides a unified API for both the Flink streaming and batch environment. As such, a TableSource implements StreamTableSource and BatchTableSource. Our builder's API must handle both scenarios.

The table API is oriented around Flink's TableSchema classes which describe the columns. Concrete subclasses of FlinkPravegaTableSource must be provided to parse a serialized event as an instance of that schema. We provide JSON support out-of-box with FlinkPravegaJsonTableSource.

Create a FlinkPravegaTableSource for the Table API:

TableSchema schema = ...;
FlinkPravegaTableSource source = FlinkPravegaJsonTableSource.builder()
    .forStream(...)
    .withPravegaConfig(config)
    .withSchema(...)
    .build();

ExecutionEnvironment batchEnv = ...;
// OR
StreamingExecutionEnvironment streamEnv = ...;

Writing a Stream

Writer-Builder (Common)

All the writer-builders share a base builder, which focuses on which stream will be written to.

AbstractWriterBuilder.builder()
    .forStream(...)   // unqualified, qualified, or typed stream identifier
    .withPravegaConfig(config)

Streaming API

The streaming writer is used for both the DataStream API and the Table API. To support this, another abstract writer-builder, AbstractStreamingWriterBuilder, is introduced that extends AbstractWriterBuilder to cover writer semantics and timeouts:

AbstractStreamingWriterBuilder.builder()
    .withWriterMode(AT_LEAST_ONCE | EXACTLY_ONCE)
    .withTxnTimeout(Time.seconds(30))
    .withTxnGracePeriod(Time.seconds(30))

For the DataStream API, construct a concrete sink function, FlinkPravegaWriter:

FlinkPravegaWriter<Integer> pravegaSink = FlinkPravegaWriter.<Integer>builder()
   .forStream(...)
   .withPravegaConfig(...)
   .withWriterMode(EXACTLY_ONCE)
   .build();

StreamExecutionEnvironment env = StreamExecutionEnvironment.getExecutionEnvironment();
DataStream<Integer> stream = ...
stream.addSink(pravegaSink);

Table API

Note: the Pravega table sink works only in the streaming environment at this time.

Create a FlinkPravegaTableSink for the Table API using JSON format:

FlinkPravegaTableSink pravegaSink = FlinkPravegaJsonTableSink.builder()
    .forStream(...)
    .withPravegaConfig(...)
    .withRoutingKeyField("field")
    .build();

StreamExecutionEnvironment env = StreamExecutionEnvironment.getExecutionEnvironment();
StreamTableEnvironment tableEnv = TableEnvironment.getTableEnvironment(env);
Table table = ...
table.writeToSink(pravegaSink);

…t, FlinkPravegaTableSource - introduced PravegaConfig to capture high-level client configuration - general stream cut support - support for environment-provided default Pravega scope - builder for FlinkPravegaInputFormat - builder for FlinkPravegaReader - rename FlinkPravegaReaderTest to FlinkPravegaReaderITCase - unit test for FlinkPravegaReader - implement batch table API support - builder for FlinkPravegaTableSource - introduce FlinkPravegaJsonTableSource - treat FlinkPravegaTableSource as an internal class - deprecated StreamId in favor of io.pravega.client.stream.Stream - deprecated FlinkPravegaParams in favor of PravegaConfig, builder classes - moved IntegerDeserializationSchema to utils - implemented StreamSourceOperatorTestHarness Signed-off-by: Eron Wright <[email protected]>

- remove FlinkPravegaParams in favor of PravegaConfig - remove StreamId Signed-off-by: Eron Wright <[email protected]>

- implement builders for sinks - FlinkPravegaTabkeSink, FlinkPravegaWriter Signed-off-by: Eron Wright <[email protected]>

- update pravega submodule (w/ client-side serialization changes) Signed-off-by: Eron Wright <[email protected]>

fpj · 2018-05-08T18:28:48Z

There is an unused import that is causing checkstyle to fail:

:checkstyleMain[ant:checkstyle] [ERROR] /home/travis/build/pravega/flink-
connectors/src/main/java/io/pravega/connectors/flink/CheckpointSerializer.java:13:8: Unused import - 
org.apache.commons.lang3.SerializationUtils. [UnusedImports]

Signed-off-by: Eron Wright <[email protected]>

- suppress findbugs warnings due to Pravega issue Signed-off-by: Eron Wright <[email protected]>

vijikarthi · 2018-05-08T21:26:21Z

src/main/java/io/pravega/connectors/flink/util/StreamWithBoundaries.java

+    public static StreamWithBoundaries of(Stream stream, StreamCut from, StreamCut to) {
+        Preconditions.checkNotNull(stream, "stream");
+        Preconditions.checkNotNull(stream, "from");
+        Preconditions.checkNotNull(stream, "to");


36:37 - please change the variable name stream to from and to

vijikarthi · 2018-05-08T22:41:40Z

src/main/java/io/pravega/connectors/flink/FlinkPravegaReader.java

+            if (this.readerGroupRefreshTime != null) {
+                rgConfigBuilder.groupRefreshTimeMillis(this.readerGroupRefreshTime.toMilliseconds());
+            }
+            resolveStreams().forEach(s -> rgConfigBuilder.stream(s.getStream(), s.getFrom() /*, s.getTo() */));


Why is the to stream cut commented and left to default UNBOUNDED?

vijikarthi · 2018-05-08T22:50:02Z

src/test/java/io/pravega/connectors/flink/FlinkPravegaJsonTableSourceTest.java

+import org.apache.flink.types.Row;
+import org.junit.Test;
+
+import static org.junit.Assert.*;


Please replace the bulk import statement with specific package imports.

vijikarthi · 2018-05-08T23:01:40Z

Since this patch introduces breaking API changes, we need to fix the connector dependent projects (nautilus-samples, pravega-samples etc.,) before merging this code.

- workaround for pravega/pravega#2519 Signed-off-by: Eron Wright <[email protected]>

- update Pravega submodule Signed-off-by: Eron Wright <[email protected]>

Signed-off-by: Eron Wright <[email protected]>

- support for end cut Signed-off-by: Eron Wright <[email protected]>

- cleanup import statements Signed-off-by: Eron Wright <[email protected]>

EronWright · 2018-05-09T17:51:14Z

@vijikarthi OK this is ready for another review.

fpj

Really nice work, @EronWright . I have left a few comments for your consideration.

fpj · 2018-05-10T13:43:02Z

src/main/java/io/pravega/connectors/flink/AbstractReaderBuilder.java

+        private final StreamCut from;
+        private final StreamCut to;
+
+        public static StreamSpec of(String streamSpec, StreamCut from, StreamCut to) {


Minor: the name of the first variable is a bit confusing. The name of the class is StreamSpec and it contains a stream name + frm/to cuts. Consequently, the streamSpec string isn't a full stream spec according to the class definition. Perhaps call the variable streamName?

Well, the main idea here is that the 'streamSpec' is an unprocessed user input that will be parsed later (out of necessity). I avoided 'streamName' because I don't want anyone to treat the value as an actual stream name. Also it is a private class.

fpj · 2018-05-10T13:48:14Z

src/main/java/io/pravega/connectors/flink/CheckpointSerializer.java

@@ -32,15 +32,17 @@ public int getVersion() {

    @Override
    public byte[] serialize(Checkpoint checkpoint) throws IOException {
-        return SerializationUtils.serialize(checkpoint);
+        ByteBuffer buf = checkpoint.toBytes();
+        byte[] b = new byte[buf.remaining()];


Should we be flipping the byte buffer instead of doing this?

This is the way to convert an opaque ByteBuffer to a byte[]. I would argue that toBytes should return a byte[].

OK, this is expecting the buffer to be in the right position, makes sense.

As for checkpoint.toBytes() returning a ByteBuffer, I agree that typically it returns a byte[]. We should consider changing it in Pravega.

fpj · 2018-05-10T13:50:07Z

src/main/java/io/pravega/connectors/flink/FlinkPravegaInputFormat.java

@@ -183,4 +170,45 @@ public T nextRecord(T t) throws IOException {
    public void close() throws IOException {
        this.segmentIterator.close();
    }
+
+    /**
+     * Gets a builder {@link FlinkPravegaInputFormat} to read Pravega streams using the Flink batch API.


This PR is not consistently adding a new line between description and first @param. Having the extra line is good for readability.

fpj · 2018-05-10T13:51:33Z

src/main/java/io/pravega/connectors/flink/FlinkPravegaReader.java

    // ----- configuration fields -----

-    // The supplied event deserializer.
-    private final DeserializationSchema<T> deserializationSchema;


Why are these variables not private any longer?

For the sake of the new unit test that makes assertions against them.

fpj · 2018-05-10T14:00:41Z

src/main/java/io/pravega/connectors/flink/FlinkPravegaReader.java

-        return eventReadTimeout;
+    void initialize() {
+        // TODO: This will require the client to have access to the pravega controller and handle any temporary errors.
+        //       See https://github.com/pravega/pravega/issues/553.


This issue is actually closed. If this is still a concern, then we need an issue against this repository.

I'm wondering if this is an actual concern, though. On the one hand, if the controller is not accessible when this environment is being constructed, then it is a good idea to fail early. On the other hand, this is just setting up configuration for submission, and creating a reader group is really part of runtime.

It also depends on when this initialize call is actually invoked. If at job submission or job execution time, then we are fine and we don't need another issue.

I will be attempting to solving this in #89. Note that this block of code is simply moving around. So, let it wait a bit OK?

As for the rationale, in Flink it is considered a best practice to defer source/sink communication until the job executes. I did some prep for this in the hook enhancement that was merged recently in Flink 1.5. It is thought that the init code can move to where the hook is instantiated.

That's fine, but let's at least replace the issue from the pravega queue, which is currently closed, with one in the flink-connector queue. I have created this one:

#130

fpj · 2018-05-10T14:18:26Z

src/main/java/io/pravega/connectors/flink/FlinkPravegaWriter.java

+     * @param <T> the element type.
+     * @param <B> the builder type.
+     */
+    public abstract static class AbstractStreamingWriterBuilder<T, B extends AbstractStreamingWriterBuilder> extends AbstractWriterBuilder<B> {


Same question about being in a separate file.

fpj · 2018-05-10T14:20:21Z

src/main/java/io/pravega/connectors/flink/util/StreamId.java

- * Captures the fully qualified name of a stream. The convention to represent this as a
- * single string is using [scope]/[stream].
- */
-public class StreamId {


The description says that this is being deprecated, but the class is actually being removed rather than marked as deprecated.

fpj · 2018-05-10T14:24:10Z

src/test/java/io/pravega/connectors/flink/FlinkPravegaReaderTest.java

-
-    //Ensure each test completes within 120 seconds.
-    @Rule
-    public final Timeout globalTimeout = new Timeout(120, TimeUnit.SECONDS);


It looks like these test cases have been left timeout-less.

The unit tests don't wait for anything in an unbounded way that would necessitate a timeout. I'll review whether any of integration tests (which I agree should use a timeout) are lacking a timeout.
Update: reviewed, all integration tests have a timeout.

fpj · 2018-05-10T14:26:04Z

src/test/java/io/pravega/connectors/flink/FlinkTableITCase.java

@@ -146,6 +155,63 @@ public void testEndToEnd() throws Exception {
        }
    }

+
+    /**
+     * Tests the end-to-end functionality of a batch table source & sink.


fpj · 2018-05-10T14:27:22Z

src/test/java/io/pravega/connectors/flink/FlinkTableITCase.java

+     * @throws Exception on exception
+     */
+    @Test
+    @Ignore("[issue-124] FlinkPravegaTableSink doesn't support BatchTableSink")


Instead of ignoring the test case, should we remove and add it back once 124 is done? It can go in the same PR.

I would prefer we leave it, since these integration tests are illustrative and we don't have any other samples. It will be fixed soon in #124

shrids · 2018-05-11T09:36:45Z

src/main/java/io/pravega/connectors/flink/FlinkPravegaReader.java

+    protected ReaderGroup createReaderGroup() {
+        ReaderGroupManager readerGroupManager = createReaderGroupManager();
+        readerGroupManager.createReaderGroup(this.readerGroupName, readerGroupConfig);
+        return readerGroupManager.getReaderGroup(this.readerGroupName);


We should invoke readerGroupManager.close() (since closing the connectionFactory does not close the connection to controller)

Yeah, good point. The reader group object needs to be closed after this merge:

https://github.com/pravega/pravega/pull/2383/files#diff-67be2af2da7593612509f695490a147bR150

Looking at the checkpoint hook, it is not clear to me where it needs to happen, though.

I too have been worried about the cleanup issue, so I added a close method to Flink's hook interface in 1.5.

https://github.com/apache/flink/blob/af29172bf555558e579a92f0ee3e56ed76fd7a1a/flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/MasterTriggerRestoreHook.java#L80

fpj · 2018-05-11T12:39:31Z

src/main/java/io/pravega/connectors/flink/CheckpointSerializer.java

@@ -32,15 +32,17 @@ public int getVersion() {

    @Override
    public byte[] serialize(Checkpoint checkpoint) throws IOException {
-        return SerializationUtils.serialize(checkpoint);
+        ByteBuffer buf = checkpoint.toBytes();
+        byte[] b = new byte[buf.remaining()];


OK, this is expecting the buffer to be in the right position, makes sense.

As for checkpoint.toBytes() returning a ByteBuffer, I agree that typically it returns a byte[]. We should consider changing it in Pravega.

fpj · 2018-05-11T13:08:54Z

src/main/java/io/pravega/connectors/flink/FlinkPravegaReader.java

-        return eventReadTimeout;
+    void initialize() {
+        // TODO: This will require the client to have access to the pravega controller and handle any temporary errors.
+        //       See https://github.com/pravega/pravega/issues/553.


That's fine, but let's at least replace the issue from the pravega queue, which is currently closed, with one in the flink-connector queue. I have created this one:

#130

fpj · 2018-05-11T13:29:36Z

src/main/java/io/pravega/connectors/flink/FlinkPravegaReader.java

+    protected ReaderGroup createReaderGroup() {
+        ReaderGroupManager readerGroupManager = createReaderGroupManager();
+        readerGroupManager.createReaderGroup(this.readerGroupName, readerGroupConfig);
+        return readerGroupManager.getReaderGroup(this.readerGroupName);


Yeah, good point. The reader group object needs to be closed after this merge:

https://github.com/pravega/pravega/pull/2383/files#diff-67be2af2da7593612509f695490a147bR150

Looking at the checkpoint hook, it is not clear to me where it needs to happen, though.

- move abstract builders to separate files - update reference to issue pertaining to client-to-controller communication - add whitespace in comments Signed-off-by: Eron Wright <[email protected]>

EronWright · 2018-05-11T17:09:52Z

@fpj and @shrids I think I've addressed all feedback.

All feedback addressed

EronWright changed the title ~~[issue-48] Builder API~~ [issue-48] Builder API (WIP) Apr 28, 2018

[issue-48] builder API

2739df7

- remove FlinkPravegaParams in favor of PravegaConfig - remove StreamId Signed-off-by: Eron Wright <[email protected]>

EronWright requested review from tzulitai and vijikarthi May 1, 2018 22:50

[issue-48] builder API

b1a1669

- implement builders for sinks - FlinkPravegaTabkeSink, FlinkPravegaWriter Signed-off-by: Eron Wright <[email protected]>

EronWright changed the title ~~[issue-48] Builder API (WIP)~~ [issue-48] Builder API May 3, 2018

hldnova mentioned this pull request May 7, 2018

Refactor flink examples to use flink connector builder API pravega/pravega-samples#76

Closed

[issue-48] builder API

7a9d4ed

- update pravega submodule (w/ client-side serialization changes) Signed-off-by: Eron Wright <[email protected]>

EronWright mentioned this pull request May 8, 2018

Fix breaking API changes introduced in Pravega as part of the serialization API changes #128

Closed

EronWright added 2 commits May 8, 2018 11:40

[issue-48] builder API

29fdc42

Signed-off-by: Eron Wright <[email protected]>

[issue-48] builder API

9b9e3c7

- suppress findbugs warnings due to Pravega issue Signed-off-by: Eron Wright <[email protected]>

vijikarthi reviewed May 8, 2018

View reviewed changes

EronWright added 5 commits May 8, 2018 17:11

[issue-48] builder API

659f626

- workaround for pravega/pravega#2519 Signed-off-by: Eron Wright <[email protected]>

[issue-48] builder API

19af4eb

- update Pravega submodule Signed-off-by: Eron Wright <[email protected]>

[issue-48] support end cuts

e9f7fb4

Signed-off-by: Eron Wright <[email protected]>

[issue-48]

b23152c

- support for end cut Signed-off-by: Eron Wright <[email protected]>

[issue-48] builder API

6257c54

- cleanup import statements Signed-off-by: Eron Wright <[email protected]>

EronWright mentioned this pull request May 9, 2018

Request for newWriter method to take transaction timeout and grace period in FlinkPravegaParam #123

Closed

vijikarthi approved these changes May 9, 2018

View reviewed changes

fpj added the breaking changes label May 10, 2018

fpj requested changes May 10, 2018

View reviewed changes

shrids reviewed May 11, 2018

View reviewed changes

fpj previously requested changes May 11, 2018

View reviewed changes

[issue-28] builder API

0967fff

- move abstract builders to separate files - update reference to issue pertaining to client-to-controller communication - add whitespace in comments Signed-off-by: Eron Wright <[email protected]>

EronWright merged commit a60cfac into master May 14, 2018

EronWright mentioned this pull request Jul 25, 2018

Implement table source (batch) #62

Closed

EronWright deleted the issue-48-builder branch May 31, 2019 17:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[issue-48] Builder API #125

[issue-48] Builder API #125

EronWright commented Apr 28, 2018 •

edited

Loading

fpj commented May 8, 2018 •

edited

Loading

vijikarthi May 8, 2018

vijikarthi May 8, 2018

vijikarthi May 8, 2018

vijikarthi commented May 8, 2018

EronWright commented May 9, 2018

fpj left a comment

fpj May 10, 2018

EronWright May 10, 2018

fpj May 10, 2018

EronWright May 10, 2018

fpj May 11, 2018

fpj May 10, 2018

fpj May 10, 2018

EronWright May 10, 2018

fpj May 10, 2018

EronWright May 10, 2018

fpj May 11, 2018

fpj May 10, 2018

fpj May 10, 2018

fpj May 10, 2018

EronWright May 11, 2018 •

edited

Loading

fpj May 10, 2018

fpj May 10, 2018

EronWright May 10, 2018

shrids May 11, 2018

fpj May 11, 2018

EronWright May 11, 2018

fpj May 11, 2018

fpj May 11, 2018

fpj May 11, 2018

EronWright commented May 11, 2018

[issue-48] Builder API #125

[issue-48] Builder API #125

Conversation

EronWright commented Apr 28, 2018 • edited Loading

Documentation (Draft)

Connecting to Pravega

PravegaConfig

Reading a Stream

Reader-Builder (Common)

Batch API

Streaming API

Table API

Writing a Stream

Writer-Builder (Common)

Streaming API

Table API

fpj commented May 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vijikarthi commented May 8, 2018

EronWright commented May 9, 2018

fpj 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

EronWright May 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EronWright commented May 11, 2018

EronWright commented Apr 28, 2018 •

edited

Loading

fpj commented May 8, 2018 •

edited

Loading

EronWright May 11, 2018 •

edited

Loading