KAFKA-4730: Streams does not have an in-memory windowed store #6239

ableegoldman · 2019-02-08T01:30:57Z

Implemented an in-memory window store allowing for range queries. A finite retention period defines how long records will be kept, ie the window of time for fetching, and the grace period defines the window within which late-arriving data may still be written to the store.

Unit tests were written to test the functionality of the window store, including its insert/update/delete and fetch operations. Single-record, all records, and range fetch were tested, for both time ranges and key ranges. The logging and metrics for late-arriving (dropped)records were tested as well as the ability to restore from a changelog.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

ableegoldman · 2019-02-08T01:31:27Z

@guozhangwang @mjsax @vvcephei @bbejeck

vvcephei

Hi @ableegoldman ,

Thanks for this PR!

I've made a first pass on API-level concerns. I haven't looked at the implementation in detail, yet.

It looks really good overall; I just had a few concerns pop out.

Thanks,
-John

streams/src/main/java/org/apache/kafka/streams/state/Stores.java

vvcephei · 2019-02-08T20:32:33Z

streams/src/main/java/org/apache/kafka/streams/state/Stores.java

@@ -166,7 +222,7 @@ public static WindowBytesStoreSupplier persistentWindowStore(final String name,
                                                                 final long windowSize,
                                                                 final boolean retainDuplicates) {
        if (numSegments < 2) {
-            throw new IllegalArgumentException("numSegments cannot must smaller than 2");
+            throw new IllegalArgumentException("numSegments cannot be smaller than 2");


Oof. Thanks.

streams/src/main/java/org/apache/kafka/streams/state/internals/InMemoryWindowStore.java

vvcephei · 2019-02-08T20:37:15Z

streams/src/main/java/org/apache/kafka/streams/state/internals/InMemoryWindowStore.java

+import static org.apache.kafka.streams.state.internals.WindowKeySchema.extractStoreKey;
+import static org.apache.kafka.streams.state.internals.WindowKeySchema.extractStoreTimestamp;
+
+public class InMemoryWindowStore<K, V> implements WindowStore<K, V> {


If I read the code correctly, K is always Bytes and V is always byte[]. IMHO, it would be better to just use these types directly.

I agree although the in-memory KV store just uses generic K, V. Is there any particular reason for that or would we want to change that as well?

InMemoryKeyValue uses generics, because this allows users to use it any type (for the case they don't want to use it with caching/logging wrappers) what seems to be ok. There is no reason to restrict the type IMHO.

For RocksDB it's different, because RocksDB does need to serialize -- thus, removing generics simplifies the code as the type must be plain bytes.

Actually, InMemoryKeyValue also doesn't need the generics. It's constructed in exactly one place, in Stores:

public static KeyValueBytesStoreSupplier inMemoryKeyValueStore(final String name) { Objects.requireNonNull(name, "name cannot be null"); return new KeyValueBytesStoreSupplier() { @Override public String name() { return name; } @Override public KeyValueStore<Bytes, byte[]> get() { return new InMemoryKeyValueStore<>(name, Serdes.Bytes(), Serdes.ByteArray()); } @Override public String metricsScope() { return "in-memory-state"; } }; }

Thus, we could (and should) go ahead an inline those types as well.

The generics are a historical artifact, if I understand the history. The lowest-level store used to be generic, but at some point we decided to go ahead and pin it down to just store bytes, but we left the actual generic parameters in place.

streams/src/main/java/org/apache/kafka/streams/state/internals/InMemoryWindowStore.java

...src/main/java/org/apache/kafka/streams/state/internals/InMemoryWindowBytesStoreSupplier.java

streams/src/main/java/org/apache/kafka/streams/state/internals/InMemoryWindowStore.java

ableegoldman · 2019-02-12T00:11:28Z

streams/src/main/java/org/apache/kafka/streams/state/internals/InMemoryWindowStore.java

@@ -48,7 +48,7 @@
 import static org.apache.kafka.streams.state.internals.WindowKeySchema.extractStoreKey;
 import static org.apache.kafka.streams.state.internals.WindowKeySchema.extractStoreTimestamp;

-public class InMemoryWindowStore<K, V> implements WindowStore<K, V> {
+public class InMemoryWindowStore<K extends Comparable<K>, V> implements WindowStore<K, V> {


Temporary fix, will likely make the switch from generics to <Bytes, byte[]> in separate PR

👍 Maybe we should just consider that to be within the scope of https://issues.apache.org/jira/browse/KAFKA-7918 . (If you agree, can you leave a comment on the ticket to that effect?)

ableegoldman · 2019-02-12T00:20:29Z

...src/main/java/org/apache/kafka/streams/state/internals/InMemoryWindowBytesStoreSupplier.java

+
+    @Override
+    public boolean retainDuplicates() {
+        return false;


The current implementation does not support duplicates, will add this

Yes, we need this for a full solution. The solution should be exactly the same as in the Rocks store (using seqnum for time windows).

bbejeck

Thanks for this PR @ableegoldman, sorry for jumping in a little late.
I've made an initial pass, and overall looks good. I have a couple of minor comments.

Also, I'm wondering if we should have an integration test for this as well.

bbejeck · 2019-02-12T21:56:47Z

streams/src/main/java/org/apache/kafka/streams/state/internals/InMemoryWindowStore.java

+        final List<KeyValue<Windowed<K>, V>> returnSet = new LinkedList<>();
+
+        for (final Entry<Long, NavigableMap<K, V>> segmentMapEntry : this.segmentMap.entrySet()) {
+            for (final Entry<K, V> kvMapEntry : segmentMapEntry.getValue().entrySet()) {


minor - no test coverage for lines 221 - 225 in the unit test

bbejeck · 2019-02-12T22:00:39Z

streams/src/main/java/org/apache/kafka/streams/state/internals/InMemoryWindowStore.java

+        }
+
+        @Override
+        public Long peekNextKey() {


minor - peekNextKey not covered in the unit test

bbejeck · 2019-02-12T22:12:00Z

Failure unrelated - kafka.api.AdminClientIntegrationTest.testMinimumRequestTimeouts
retest this please

vvcephei · 2019-02-12T22:35:03Z

streams/src/main/java/org/apache/kafka/streams/state/internals/InMemoryWindowStore.java

+    private final long retentionPeriod;
+    private final long windowSize;
+
+    private final NavigableMap<Long, NavigableMap<K, V>> segmentMap;


This is currently a mapping windowStartTime -> key -> value, which is fine, but I wonder if it would simplify the code at all to use the equivalent mapping:
(windowStartTime, key) -> value by making it a NavigableMap<Windowed<K>, V>.

You'd have to provide a Comparator to the TreeMap constructor to determine order based on timestamp first, then on key.

ableegoldman · 2019-02-13T06:22:55Z

docs/streams/developer-guide/processor-api.html

@@ -226,8 +226,9 @@ <h2>
                                application instance, but must fit into the available local disk
                                space.</li>
                            <li>RocksDB settings can be fine-tuned, see
-                                <a class="reference internal" href="config-streams.html#streams-developer-guide-rocksdb-config"><span class="std std-ref">RocksDB configuration</span></a>.</li>
-                            <li>Available <a class="reference external" href="/{{version}}/javadoc/org/apache/kafka/streams/state/Stores.PersistentKeyValueFactory.html">store variants</a>:


This link is broken so I updated it to where I think it was intended to point, please check if it makes sense or not

Another doc change we need to make is in docs/ops.html, under

<h5><a id="kafka_streams_store_monitoring" href="#kafka_streams_store_monitoring">State Store Metrics</a></h5>

we need to add the store-scope: <li><code>in-memory-window-state</code></li>.

And in docs/streams/upgrade-guide.html we can add a new section

<h3><a id="streams_api_changes_230" href="#streams_api_changes_220">Streams API changes in 2.3.0</a></h3>

and add a one-line paragraph saying that we added a new interface in Stores that provides a built-in in-memory window store, etc etc.

ableegoldman · 2019-02-13T23:37:38Z

With the current implementation, fetch and, in particular, all() are inefficient as they construct new objects for each record. This has the advantage of providing a snapshot, but might not be worth the cost since we make no guarantees about that anyways. We can refactor things to improve efficiency but is probably worth a separate JIRA/PR after this one is merged?

guozhangwang

Thanks @ableegoldman . Made a pass over the PR.

guozhangwang · 2019-02-14T06:37:25Z

docs/streams/developer-guide/processor-api.html

@@ -226,8 +226,9 @@ <h2>
                                application instance, but must fit into the available local disk
                                space.</li>
                            <li>RocksDB settings can be fine-tuned, see
-                                <a class="reference internal" href="config-streams.html#streams-developer-guide-rocksdb-config"><span class="std std-ref">RocksDB configuration</span></a>.</li>
-                            <li>Available <a class="reference external" href="/{{version}}/javadoc/org/apache/kafka/streams/state/Stores.PersistentKeyValueFactory.html">store variants</a>:


guozhangwang · 2019-02-14T06:41:57Z

docs/streams/developer-guide/processor-api.html

@@ -226,8 +226,9 @@ <h2>
                                application instance, but must fit into the available local disk
                                space.</li>
                            <li>RocksDB settings can be fine-tuned, see
-                                <a class="reference internal" href="config-streams.html#streams-developer-guide-rocksdb-config"><span class="std std-ref">RocksDB configuration</span></a>.</li>
-                            <li>Available <a class="reference external" href="/{{version}}/javadoc/org/apache/kafka/streams/state/Stores.PersistentKeyValueFactory.html">store variants</a>:


Another doc change we need to make is in docs/ops.html, under

<h5><a id="kafka_streams_store_monitoring" href="#kafka_streams_store_monitoring">State Store Metrics</a></h5>

we need to add the store-scope: <li><code>in-memory-window-state</code></li>.

guozhangwang · 2019-02-14T06:43:40Z

docs/streams/developer-guide/processor-api.html

@@ -226,8 +226,9 @@ <h2>
                                application instance, but must fit into the available local disk
                                space.</li>
                            <li>RocksDB settings can be fine-tuned, see
-                                <a class="reference internal" href="config-streams.html#streams-developer-guide-rocksdb-config"><span class="std std-ref">RocksDB configuration</span></a>.</li>
-                            <li>Available <a class="reference external" href="/{{version}}/javadoc/org/apache/kafka/streams/state/Stores.PersistentKeyValueFactory.html">store variants</a>:


And in docs/streams/upgrade-guide.html we can add a new section

<h3><a id="streams_api_changes_230" href="#streams_api_changes_220">Streams API changes in 2.3.0</a></h3>

and add a one-line paragraph saying that we added a new interface in Stores that provides a built-in in-memory window store, etc etc.

guozhangwang · 2019-02-14T06:47:41Z

...src/main/java/org/apache/kafka/streams/state/internals/InMemoryWindowBytesStoreSupplier.java

+    @Deprecated
+    @Override
+    public int segments() {
+        return (int) retentionPeriod;


I'd suggest we just throw exception in this function, instead of returning retentionPeriod here. Since this is a deprecated API and should not be called anymore inside Streams library. Returning some garbage values instead of throwing an exception is vulnerable to future bugs that calls this function by mistake.

guozhangwang · 2019-02-14T06:49:49Z

...src/main/java/org/apache/kafka/streams/state/internals/InMemoryWindowBytesStoreSupplier.java

+    // In-memory window store is not *really* segmented, so just say size is 1 ms
+    @Override
+    public long segmentIntervalMs() {
+        return 1;


Hmm.. if users decides to have a caching layer on top of this in-memory window store (which would be a weird usage, but we cannot forbid it) then the SegmentedCacheFunction would use 1ms as segments, causing millions of segments. Would that be okay?

I'm not sure I follow, SegmentedCacheFunction only seems to use this for comparison, why would that be a problem?

I think you're right, it is only logical, not physically impacting the data layout. With a long segmentId we should be fine with the range of segment ids with 1ms interval only.

guozhangwang · 2019-02-14T06:57:28Z

streams/src/main/java/org/apache/kafka/streams/state/internals/InMemoryWindowStore.java

+
+    private void removeExpiredSegments() {
+        final long minLiveTime = this.context.streamTime() - this.retentionPeriod;
+        final NavigableMap<Long, NavigableMap<WrappedK<K>, V>> expiredSegments = this.segmentMap.headMap(minLiveTime, true);


Why not just map.headMap(...).clear(); ?

guozhangwang · 2019-02-14T07:15:25Z

streams/src/main/java/org/apache/kafka/streams/state/internals/InMemoryWindowStore.java

+        } else {
+            if (value != null) {
+                this.segmentMap.computeIfAbsent(windowStartTimestamp, t -> new TreeMap<>());
+                this.segmentMap.get(windowStartTimestamp).put(new WrappedK<>(key, seqnum), value);


If retainDuplicates is false, could we not storing the sequence suffix at all to save 4 bytes per record?

I think this will be more straightforward after we remove the generics since we can just tack on the seqnum Bytes or not

Sounds good.

guozhangwang · 2019-02-14T07:22:48Z

streams/src/main/java/org/apache/kafka/streams/state/internals/InMemoryWindowStore.java

+        }
+    }
+
+    private KeyValue<Windowed<K>, V> getWindowedKeyValue(final K key, final long startTimestamp, final V value) {


Not a comment for this PR: when we remove the generics, this function can be replaced with WindowStoreIteratorWrapper since the inner iterator would always be bytes.

guozhangwang · 2019-02-14T07:30:23Z

streams/src/main/java/org/apache/kafka/streams/state/internals/InMemoryWindowStore.java

+        }
+    }
+
+    private static class InMemoryWindowStoreIterator<V> implements WindowStoreIterator<V> {


I think we can consolidate these two iterators and the InMemoryKeyValueStore#InMemoryKeyValueIterator as a single wrapper iterator (note WindowStoreIterator<V> is just a KeyValueStoreIterator<Long, V>.), that takes an iteratoras inner. Then for supporting "peekNextKey" we do not really need to rely on list iterator'spreviousbut just need to maintain the next element, and update it whenevernext()` is called and this element on-hold is returned.

And when we removed the generics we can even consolidate all of them in WindowStoreIteratorWrapper as well.

The problem with using the WindowStoreIteratorWrapper here is that expects the key Bytes to actually be a windowed key ie to contain the timestamp, however with the in-memory version we want to avoid having to wrap the timestamp with the key as well so the iterator cant just extract it from the Bytes

When we removed the generics and allow have bytes in InMemoryWindowStore, when we return the inner ListIterator aren't we going to have each key be concatenated with key-bytes followed with timestamp-bytes anyways?

Currently the key Bytes are wrapped up with timestamp into a windowed type directly in the fetch call, whereas the WindowStoreIteratorWrapper is given an iterator over Bytes and it does the extraction of key+timestamp -> windowed when accessed. We could rewrite the in-memory store as a single map with key+timestamp bytes as the key, but with the current implementation wrapping the key with the timestamp is a waste of space as we already map from each timestamp to a kv store

John actually raised that as a possibility to improve clarity. I wasn't entirely convinced that it would make things easier to read but if you both agree, I can be. Certainly I agree it would be good to share the iterator code

In retrospect, I'm not sure I'm convinced either ;)

One nice aspect of your current design is that when we need to drop a window, we can do it in one operation.

Okay, I think I'm convinced. We should just leave it as is in this PR at least, and after it is merged I will think about it twice when reviewing the other PR for inlining the byte stores.

guozhangwang · 2019-02-14T07:39:37Z

streams/src/main/java/org/apache/kafka/streams/state/internals/InMemoryWindowStore.java

+    @Override
+    public V fetch(final K key, final long windowStartTimestamp) {
+        removeExpiredSegments();
+        if (windowStartTimestamp <= this.context.streamTime() - this.retentionPeriod)


After the cleanup, can we just check if there's a bucket in the current map still (if windowStartTimestamp <= this.context.streamTime() - this.retentionPeriod then it should be cleared in removeExpiredSegments right?

guozhangwang · 2019-02-14T07:44:51Z

Jenkins failures seems consistent but should be irrelevant to this PR, maybe you need to rebase on the latest trunk?

Also I'm wondering if making a 1ms bucket is most efficient: for hopping windows that maybe okay since the window-start-timestamp should be sparse in the time-span; but if it is a sliding window then we will end up with tons of buckets and hence headMap etc could be very costly, and in that case maybe having a wider bucket like 100ms is a better trade-off. WDYT @ableegoldman @vvcephei @bbejeck @mjsax ?

ableegoldman · 2019-02-14T19:06:37Z

Also could be refactored as a map: key-> (map: timestamp -> value), maybe that makes more sense?

guozhangwang · 2019-02-14T23:33:40Z

Also could be refactored as a map: key-> (map: timestamp -> value), maybe that makes more sense?

Thinking about it a bit more, I think the sliding window is relatively rare compare with the hopping windows, plus we may want to have a totally different implementation for stream-stream join moving forward anyways. So shredding on time space may still make more sense than shredding on key space.

ableegoldman · 2019-02-15T00:06:15Z

+1 makes sense to have a different implementation for stream-stream joins in the future, we can split the duplicates support out as well then

vvcephei

Sorry, I had a pending comment I never submitted.

To answer your question about potentially optimizing performance, I agree we can consider that a separate scope. Correctness (and ergonomics) generally trumps performance, but if it's shockingly bad, we should consider refactoring it.

Of course, we won't know if it's poor performance or not without benchmarking it.

So I think we're justified in writing it so it behaves well (which I think you've done), and filing a Jira to do performance testing and consider optimizations.

docs/streams/developer-guide/processor-api.html

vvcephei · 2019-02-15T22:54:02Z

To weigh in on the latter question, it's not immediately obvious to me if it's better to use bigger buckets, even for large numbers of windows. It could actually be worse, as it adds overhead to other operations.

Even if we have a million windows in the map, it's still only log_2(1M)~=20 hops to the head, or any other window. Intuitively, it seems like this would be fast.

Conversely, if we were to "segment" this store, we still need to uniquely identify windows, so we'd wind up with a map: segmentId -> windowId -> key -> value or maybe segmentId -> (key, windowId) -> value . There are still the same number of elements in the whole data structure, though, so while finding the right segment would require fewer hops, finding the right key within a window is still the same number (eg, instead of log_2(1M), it's log_2(10,000) + log_2(100) = log_2(1M)). (I think)

So, it doesn't optimize seeks to specific keys, only seeks to whole segments/windows, which I think is only for expiration (headMap, as you mentioned). This should be comparatively rare.

Plus, it's also a performance optimization, which IMHO, we should back up with a benchmark or some other performance evaluation before adding complexity to the code.

ableegoldman · 2019-02-19T18:22:06Z

Both checks failed (although one indicates 0 failed), results not available.

Retest this, please

…y with removal of streamTime from PAPI

ableegoldman · 2019-02-20T00:49:21Z

JDK 8 passed, JDK 11 had two unrelated failures:

kafka.admin.DeleteConsumerGroupsTest.testDeleteCmdWithShortInitialization

kafka.api.ConsumerBounceTest.testRollingBrokerRestartsWithSmallerMaxGroupSizeConfigDisruptsBigGroup

retest this, please

vvcephei

The changes LGTM. I think there are a few hanging conversations, though.

vvcephei · 2019-02-20T16:34:49Z

streams/src/main/java/org/apache/kafka/streams/state/internals/InMemoryWindowStore.java

+            "expired-window-record-drop",
+            Sensor.RecordingLevel.INFO
+        );
+        addInvocationRateAndCount(


👍 I agree. We've seen some profiling results that suggest the widespread string literals from these metrics actually do cause memory pressure.

guozhangwang · 2019-02-20T17:27:24Z

LGTM modulo the one comment about filing a follow-up PR inlining the metrics name constants.

We still need to wait for the KIP to be officially accepted before merging the PR.

ableegoldman · 2019-02-20T20:00:50Z

JDK 8 test failed this time but JDK 11 passed, so both checks have passed at some point. KIP has also been accepted so we are ready to merge (I filed a JIRA Re: string constants hard-coded in metrics)

guozhangwang

There is a minor typo in the docs, I've pushed a commit to your branch to fix it.

guozhangwang · 2019-02-21T03:02:58Z

docs/streams/upgrade-guide.html

@@ -68,6 +68,11 @@ <h1>Upgrade Guide and API Changes</h1>
        More details about the new config <code>StreamsConfig#TOPOLOGY_OPTIMIZATION</code> can be found in <a href="https://cwiki.apache.org/confluence/display/KAFKA/KIP-295%3A+Add+Streams+Configuration+Allowing+for+Optional+Topology+Optimization">KIP-295</a>.
    </p>

+    <h3><a id="streams_api_changes_230" href="#streams_api_changes_220">Streams API changes in 2.3.0</a></h3>


nit: this should be 230 not 220 (my previous comment had a typo here)

…#6239) Implemented an in-memory window store allowing for range queries. A finite retention period defines how long records will be kept, ie the window of time for fetching, and the grace period defines the window within which late-arriving data may still be written to the store. Unit tests were written to test the functionality of the window store, including its insert/update/delete and fetch operations. Single-record, all records, and range fetch were tested, for both time ranges and key ranges. The logging and metrics for late-arriving (dropped)records were tested as well as the ability to restore from a changelog. Reviewers: John Roesler <john@confluent.io>, Matthias J. Sax <matthias@confluent.io>, Bill Bejeck <bill@confluent.io>, Guozhang Wang <wangguoz@gmail.com>

vvcephei reviewed Feb 8, 2019

View reviewed changes

ableegoldman commented Feb 12, 2019

View reviewed changes

bbejeck reviewed Feb 12, 2019

View reviewed changes

vvcephei reviewed Feb 12, 2019

View reviewed changes

ableegoldman commented Feb 13, 2019

View reviewed changes

guozhangwang reviewed Feb 14, 2019

View reviewed changes

vvcephei reviewed Feb 15, 2019

View reviewed changes

docs/streams/developer-guide/processor-api.html Show resolved Hide resolved

ableegoldman added 10 commits February 19, 2019 11:35

Implemented in-memory window store and test

f95b264

Minor fix to use correct function for in-memory window store restore

09cea4a

Minor fixes from PR review

931c20e

Added support for duplicates and missing tests

ec45c5b

Updated docs regarding available variants of in-memory store

083785d

Fixed style errors in docs

441ea10

Updated docs and minor changes from PR review

88c315a

Removed unused import

7707d99

Throw exception in segments()

201833c

Updated window store to track stream time on its own for compatabilit…

eabe346

…y with removal of streamTime from PAPI

ableegoldman force-pushed the InMemoryWindowStore branch from f457d64 to eabe346 Compare February 19, 2019 20:12

vvcephei approved these changes Feb 20, 2019

View reviewed changes

minor fix

8284985

guozhangwang approved these changes Feb 21, 2019

View reviewed changes

guozhangwang merged commit ff603c6 into apache:trunk Feb 21, 2019

KAFKA-4730: Streams does not have an in-memory windowed store #6239

KAFKA-4730: Streams does not have an in-memory windowed store #6239

Conversation

ableegoldman commented Feb 8, 2019

Committer Checklist (excluded from commit message)

ableegoldman commented Feb 8, 2019

vvcephei 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

bbejeck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbejeck commented Feb 12, 2019

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

ableegoldman commented Feb 13, 2019

guozhangwang 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

guozhangwang commented Feb 14, 2019

ableegoldman commented Feb 14, 2019

guozhangwang commented Feb 14, 2019

ableegoldman commented Feb 15, 2019

vvcephei left a comment • edited

Choose a reason for hiding this comment

vvcephei commented Feb 15, 2019

ableegoldman commented Feb 19, 2019

ableegoldman commented Feb 20, 2019

vvcephei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guozhangwang commented Feb 20, 2019

ableegoldman commented Feb 20, 2019

guozhangwang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vvcephei left a comment •

edited