KAFKA-6036: Local Materialization for Source KTable by guozhangwang · Pull Request #5779 · apache/kafka

guozhangwang · 2018-10-11T02:02:49Z

Refactor the materialization for source KTables in the way that:

If Materialized.as(queryableName) is specified, materialize;
If the downstream operator requires to fetch from this KTable via ValueGetters, materialize;
If the downstream operator requires to send old values, materialize.

Otherwise do not materialize the KTable. E.g. builder.table("topic").filter().toStream().to("topic") would not create any state stores.

There's a couple of minor changes along with PR as well:

KTableImpl's queryableStoreName and isQueryable are merged into queryableStoreName only, and if it is null it means not queryable. As long as it is not null, it should be queryable (i.e. internally generated names will not be used any more).

To achieve this, splitted MaterializedInternal.storeName() and MaterializedInternal.queryableName(). The former can be internally generated and will not be exposed to users. QueryableName can be modified to set to the internal store name if we decide to materialize it during the DSL parsing / physical topology generation phase. And only if queryableName is specified the corresponding KTable is determined to be materialized.

Found some overlapping unit tests among KTableImplTest, and KTableXXTest, removed them.
There are a few typing bugs found along the way, fixed them as well.

This PR is an illustration of experimenting a poc towards logical materializations.

Today we've logically materialized the KTable for filter / mapValues / transformValues if queryableName is not specified via Materialized, but whenever users specify queryableName we will still always materialize. My original goal is to also consider logically materialize for queryable stores, but when implementing it via a wrapped store to apply the transformations on the fly I realized it is tougher than I thought, because we not only need to support fetch or get, but also needs to support range queries, approximateNumEntries, and isOpen etc as well, which are not efficient to support. So in the end I'd suggest we still stick with the rule of always materializing if queryableName is specified, and only consider logical materialization otherwise.

Committer Checklist (excluded from commit message)

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

…cal-materialization

…cal-materialization t

guozhangwang · 2018-10-11T02:06:34Z

-                                                                               .withProcessorParameters(processorParameters)
-                                                                               .withTopic(topic)
-                                                                               .build();
+    public <K, V> KTable<K, V> table(final String topic,


Removed S template since we will always need a key-value store for this API (the MaterializedInternal indicates that already).

guozhangwang · 2018-10-11T02:07:02Z

+        final KTableSource<K, V> tableSource = new KTableSource<>(storeName, storeName);

-        final ProcessorParameters processorParameters = new ProcessorParameters(tableSource, processorName);
+        final ProcessorParameters<K, V> processorParameters = new ProcessorParameters<>(tableSource, processorName);


Minor fixes to remove unchecked warnings, ditto below.

guozhangwang · 2018-10-11T02:07:44Z

        final String sourceName = newProcessorName(KTableImpl.SOURCE_NAME);
        final String processorName = newProcessorName(KTableImpl.SOURCE_NAME);
-        final KTableSource<K, V> tableSource = new KTableSource<>(storeBuilder.name());
+        // enforce store name as queryable name to always materialize global table stores


As commented here, for GlobalKTables we should always materialize so we set the queryable name to store name at the first place to enforce that.

guozhangwang · 2018-10-11T02:08:23Z

        if (processorSupplier instanceof KTableSource) {
            final KTableSource<K, V> source = (KTableSource<K, V>) processorSupplier;
-            return new KTableSourceValueGetterSupplier<>(source.storeName);
+            // whenever a source ktable is required for getter, it should be materialized


This is the key change.

guozhangwang · 2018-10-11T02:08:30Z

+    // need to set the queryable name as the store name to enforce materialization
    public void enableSendingOldValues() {
-        sendOldValues = true;
+        this.sendOldValues = true;


This is the key change.

nit: remove this

guozhangwang · 2018-10-11T02:12:06Z

    }

-    public final void addGlobalStore(final StoreBuilder<KeyValueStore> storeBuilder,
+    public final void addGlobalStore(final StoreBuilder storeBuilder,


To cope with the typing.

guozhangwang · 2018-10-11T02:12:51Z

-        assertFalse(nodes.hasNext());
-        assertFalse(stores.hasNext());
-        assertFalse(subtopologies.hasNext());
+        builder.table(topic, Materialized.with(Serdes.Long(), Serdes.String()));


We do not need to use topology description here: only this test case is using it and other functions are relying on itb.

guozhangwang · 2018-10-11T02:13:11Z

            .count();
        final TopologyDescription describe = builder.build().describe();
+
+        System.out.println(describe);


This is for debugging: will remove.

guozhangwang · 2018-10-11T02:14:12Z

    }

-    @Test
-    public void testValueGetter() {


This can be merged with queryable value getter below, ditto in other test classes.

guozhangwang · 2018-10-11T02:15:03Z

        final String topic1 = "topic1";
-
-        final KTableImpl<String, String, String> table1 = (KTableImpl<String, String, String>) builder.table(topic1, stringConsumed);
+        final KTableImpl<String, String, String> table1 = (KTableImpl<String, String, String>) builder.table(topic1, stringConsumed, Materialized.as("store"));


This is to enforce materialization for the test. Ditto elsewhere.

guozhangwang · 2018-10-11T02:16:27Z

@bbejeck @vvcephei @mjsax

bbejeck

@guozhangwang made a quick pass looks good, I'll take a more detailed look tomorrow and I'll have some comments

bbejeck

Thanks @guozhangwang I've made a pass and overall looks good to me, I just have a couple of minor questions.

I also have one meta-question regarding the logic of store names and the ability to query a store. If a name is not provided up-front then queryable is set to false, but a store name is generated later on via the materializedInternal.generateStoreNameIfNeeded cf https://github.com/apache/kafka/blob/trunk/streams/src/main/java/org/apache/kafka/streams/StreamsBuilder.java#L229. I find it a little confusing that the store name exists but the boolean flag for queryable is false. Would we want to consider a follow-up PR at some point to explicity state the store isn't queryable becuase a name isn't provided up front? I realize that I may be splitting hairs here and overthinking things.

bbejeck · 2018-11-28T19:38:07Z

-            shouldMaterialize ? materializedInternal.storeName() : null
-        );
+        // only materialize if the state store has queryable name
+        final String queryableName = materializedInternal != null ? materializedInternal.queryableStoreName() : null;


Couldn't the MaterializedInternal#queryableStoreName() return null indicating not to materialize as well?

EDIT: I think the null check in KTableSource makes sure we never hit this condition, correct? If so, you can ignore this comment and the other 2 related ones below.

I guess it could return null -- but I also think that would be ok, just indicating that we don't materialize the result.

bbejeck · 2018-11-28T19:39:00Z

-            shouldMaterialize ? materializedInternal.storeName() : null
-        );
+        // only materialize if the state store has queryable name
+        final String queryableName = materializedInternal != null ? materializedInternal.queryableStoreName() : null;


bbejeck · 2018-11-28T19:39:12Z


-        final boolean shouldMaterialize = materialized != null && materialized.isQueryable();
+        // only materialize if users provide a specific queryable name
+        final String queryableStoreName = materialized != null ? materialized.queryableStoreName() : null;


bbejeck · 2018-11-28T20:19:34Z

-            }, Materialized.<String, Integer, KeyValueStore<Bytes, byte[]>>as("anyStoreNameFilter"));
+            }, Materialized.<String, Integer, KeyValueStore<Bytes, byte[]>>as("store2"));
        final KTableImpl<String, Integer, Integer> table3 = (KTableImpl<String, Integer, Integer>) table1.filterNot(
+            new Predicate<String, Integer>() {


nit: use lambda instead

bbejeck · 2018-11-28T20:27:56Z

-        final KTableImpl<String, Integer, Integer> table3 = (KTableImpl<String, Integer, Integer>) table2.filter(
-            new Predicate<String, Integer>() {
+        final KTableImpl<String, String, Integer> table3 = (KTableImpl<String, String, Integer>) table1.mapValues(
+            new ValueMapper<String, Integer>() {


nit: use lambda here and below

…cal-materialization

guozhangwang · 2018-11-30T01:13:10Z

Would we want to consider a follow-up PR at some point to explicity state the store isn't queryable becuase a name isn't provided up front? I realize that I may be splitting hairs here and overthinking things.

Yeah that's a good question that I'm scratching my head about when worked on this PR. For readability here's my reasoning: in order to merge the confusing two fields of KTableImpl into a single queryableStoreName, it is better to separate the case where "the KTable is still materialized with an internal name, but it is not queryable" and "the KTable is not materialized at all, hence no store name, hence not queryable". With that the semantics of storeName and isQueryable inside MaterializedInternal can be straight-forward, in fact, callers do not need to know their values but only need to call the queryableStoreName() function: if it returns null, never materialize; otherwise always materialize with the returned name.

There may be a few places that we can further remove the generateStoreNameIfNeeded call, I'll browse through them and try removing.

…cal-materialization

guozhangwang · 2018-11-30T20:57:07Z

@bbejeck I've addressed the comments, and for the generateStoreNameIfNeeded call on filter / mapValues I've also consolidated them into a single place. For compatibility I cannot remove them though (also added comments for it).

Could you take another look? Also cc @mjsax @vvcephei .

bbejeck · 2018-12-04T19:50:17Z

retest this please

bbejeck

Thanks, @guozhangwang LGTM. But I'm thinking do we need a unit test that compares the topology where the KTable is materialized and later it's not (or vice versa) to ensure compatibility? If we already have such a test then you can ignore this comment.

guozhangwang · 2018-12-05T00:25:34Z

Thanks, @guozhangwang LGTM. But I'm thinking do we need a unit test that compares the topology where the KTable is materialized and later it's not (or vice versa) to ensure compatibility? If we already have such a test then you can ignore this comment.

Yes, I looked at the TopologyTest and I think it is covered already (note all the modified ones): https://github.com/apache/kafka/pull/5779/files#diff-dc709dbcb4ca71e6bd10350ceb214a00L964

One thing worth noting though, is that we do this without optimization turned on at all since I felt we should just always do this as it does not break compatibility upgrading from any older version without it to the new version. But lmk if you feel differently

bbejeck · 2018-12-05T02:35:07Z

One thing worth noting though, is that we do this without optimization turned on at all since I felt we should just always do this as it does not break compatibility upgrading from any older version without it to the new version. But lmk if you feel differently

Yes I agree with this approach as well

mjsax

Some initial comments and questions.

mjsax · 2018-12-06T23:11:01Z

                                sourceName.equals(this.name) ? sourceNodes : Collections.singleton(sourceName),
-                                storeBuilder.name(),
-                                isQueryable,
+                                queryableStoreName,


I think we should make sure (to avoid bugs), that queryableStoreName == null || queryableStoreName.equals(storeBuilder.name()) -- can we add a check? Not sure if this class is the best place for it? But maybe it's also good to have multiple checks in place?

I thought about that, and I think it is fine since the call trace is only in two folds:

MaterializedInternal.queryableName() logic, which always based on storeName().

StoreBuilder#name()

But I can definitely add an assertion inside the call as it is not on the critical path either. We've never done that before but I cannot think of a good reason not to.

It's a matter of code style (I personally like assertions, but many people don't). Do we run test with assertions enabled? If not, it does not make sense to add one.

mjsax · 2018-12-06T23:22:20Z

+                                     final MaterializedInternal<K, V, KeyValueStore<Bytes, byte[]>> materialized) {
+        final String sourceName = newProcessorName(KStreamImpl.SOURCE_NAME);
+        final String tableSourceName = newProcessorName(KTableImpl.SOURCE_NAME);
+        // only materialize the source store if it is queryable, or it is required to be materialized from downstream


nit: this comment is confusion, because it's not clear how to map it to the code? I would remove it. What is the purpose?

Sounds good. I'll remove it.

mjsax · 2018-12-06T23:23:21Z

+        final KTableSource<K, V> tableSource = new KTableSource<>(materialized.storeName(), materialized.queryableStoreName());
+        final ProcessorParameters<K, V> processorParameters = new ProcessorParameters<>(tableSource, tableSourceName);
+
+        final TableSourceNode<K, V, KeyValueStore<K, V>> tableSourceNode = TableSourceNode.<K, V, KeyValueStore<K, V>>tableSourceNodeBuilder()


Seems like we could remove the store generic type from TableSourceNode, too?

Good call, will try.

mjsax · 2018-12-06T23:29:22Z

+    // need to set the queryable name as the store name to enforce materialization
    public void enableSendingOldValues() {
-        sendOldValues = true;
+        this.sendOldValues = true;


nit: remove this

mjsax · 2018-12-06T23:29:44Z

+    // when the source ktable requires materialization from downstream, we just
+    // need to set the queryable name as the store name to enforce materialization
+    public void materialize() {
+        this.queryableName = storeName;


nit: remove this

mjsax · 2018-12-06T23:32:09Z

+    private boolean sendOldValues;

-    private boolean sendOldValues = false;
+    public KTableSource(final String storeName, final String queryableName) {


It seems there are two cases here:
(1) the table is queryable, for this case storeName == queryableName
(2) the table is not queryable, for this case queryableName == null

Thus, it seems cleaner at this level, to keep storeName and isQueryable flag to avoid potential bugs that storeName != queryableName ?

At outer layers, we can still do it differently. Thoughts?

I personally like the distinguishment of queryableName and storeName for both this class and MaterializedInternal :) Let me try to see if I can persuade you:

storeName is the name for underlying store no matter if the store does exist or not (i.e. it may not be materialized), as long as the store is materialized the storeName would never be null.

queryableName is the name to query the store, it is set once and done. Logically it represents the logic of isQueryable ? return storeName : return null in a single place (otherwise there are a bunch of those places where this logic are needed and hence more error prone).

For KTableSource as an example, by the time we create this node we cannot tell yet whether we will materialize it at the end (e.g. if the downstream operators require this ancestor to send old values or materialize, we will then do it). So we need to generate the storeName as always but keep the queryableName as null.

I think the concern that queryableName != storeName is valid, for which case I would like to put generateStoreNameIfNeeded as part of the MaterializedInternal constructor, and hope that would cleanup a little bit.

mjsax · 2018-12-06T23:34:23Z

    }

+    public boolean isQueryable() {
+        return queriable;


Remove variable queriable and return storeName() != null instead?

mjsax · 2018-12-06T23:35:37Z

+        return queriable;
+    }
+
+    public String queryableStoreName() {


Why do we need this? Can't we call storeName() directly? If the code base is clean, storeName() should return null iff table is not queryable?

The issue is that we may need to create internal name via generateStoreNameIfNeeded which will set the storeName field. Ditto for the question around isQueryable.

Thinking about this once more, it seems inconsistent to use boolean flag isQueryable. Should we rather replace it with a second String queyableStoreName that is null if the store is not queryable?

Ack. I think we can just remove isQueryable actually since for whichever its caller, we can always use queryableName instead.

…cal-materialization

guozhangwang · 2018-12-07T03:04:28Z

@mjsax updated per comment.

mjsax

Overall LGTM.

Some more nits. Feel free to merge afterwards.

mjsax · 2018-12-07T23:13:03Z

-        } else {
-            return this.queryableStoreName;
-        }
+        return this.queryableStoreName;


nit: remove this

mjsax · 2018-12-07T23:14:54Z

                                  final MaterializedInternal<K, V, KeyValueStore<Bytes, byte[]>> materializedInternal,
                                  final boolean filterNot) {
-        final String name = builder.newProcessorName(FILTER_NAME);
+        // we actually do not need generate store names at all since if it is not specified, we will not


nit: [to] generate

mjsax · 2018-12-07T23:24:26Z

-            tupleForwarder.maybeForward(key, value, oldValue);
+
+            if (queryableName != null) {
+                final V oldValue = store.get(key);


Why do we unconditionally send old value? This was like this in the old code, but I am wondering why?

Hmm.. that's a good question! Right now we have two reasons that KTableSource needs to be materialized:

Its materialize() is called from the value getter (think: a join in downstream).

Its sendOldValues is called.

for 1), we actually dot actually need to send old values actually (and it is indeed the case since in the TupleForwarder if sendOldValues is false, we will actually ignore the old value from the caller; but we can of course save one store read as well! :) ). I will update this logic.

mjsax · 2018-12-07T23:36:28Z

-        doTestValueGetter(builder, topic1, table1, table2, table3, table4);
+        final KTableImpl<String, String, Integer> table2 = (KTableImpl<String, String, Integer>) table1.mapValues(Integer::new, Materialized.<String, Integer, KeyValueStore<Bytes, byte[]>>as(storeName2).withValueSerde(Serdes.Integer()));
+        final KTableImpl<String, String, Integer> table3 = (KTableImpl<String, String, Integer>) table1.mapValues(value -> new Integer(value) * (-1), Materialized.<String, Integer, KeyValueStore<Bytes, byte[]>>as(storeName3).withValueSerde(Serdes.Integer()));
+        final KTableImpl<String, String, Integer> table4 = (KTableImpl<String, String, Integer>) table1.mapValues(Integer::new);


nit: avoid long lines. hard to read.

mjsax · 2018-12-08T04:23:13Z

-            shouldMaterialize ? materializedInternal.storeName() : null
-        );
+        // only materialize if the state store has queryable name
+        final String queryableName = materializedInternal != null ? materializedInternal.queryableStoreName() : null;


I guess it could return null -- but I also think that would be ok, just indicating that we don't materialize the result.

mjsax · 2018-12-08T05:55:32Z

+        return queriable;
+    }
+
+    public String queryableStoreName() {


Thinking about this once more, it seems inconsistent to use boolean flag isQueryable. Should we rather replace it with a second String queyableStoreName that is null if the store is not queryable?

mjsax · 2018-12-08T06:16:13Z

 public class TableSourceNode<K, V, S extends StateStore> extends StreamSourceNode<K, V> {

-    private final StoreBuilder<S> storeBuilder;
+    private final MaterializedInternal<K, V, ?> materializedInternal;


If we assume it's key-value store only (for now), should we remove the generic type for now?

…cal-materialization

guozhangwang · 2018-12-09T03:17:51Z

| If we assume it's key-value store only (for now), should we remove the generic type for now?

Since other contributors are also working on https://cwiki.apache.org/confluence/display/KAFKA/KIP-300%3A+Add+Windowed+KTable+API+in+StreamsBuilder I'd suggest we keep it as is?

mjsax · 2018-12-09T04:30:03Z

Fine with me. Was just a thought.

…shListener (#6017) This is a follow-up PR from the previous PR #5779, where KTabeSource always get old values from the store even if sendOldValues. It gets me to make a pass over all the KTable/KStreamXXX processor to push the sendOldValues at the callers in order to avoid unnecessary store reads. More details: ForwardingCacheFlushListener and TupleForwarder both need sendOldValues as parameters. a. For ForwardingCacheFlushListener it is not needed at all, since its callers XXXCachedStore already use the sendOldValues values passed from TupleForwarder to avoid getting old values from underlying stores. b. For TupleForwarder, it actually only need to pass the boolean flag to the cached store; and then it does not need to keep it as its own variable since the cached store already respects the boolean to pass null or the actual value.. The only other minor bug I found from the pass in on KTableJoinMerge, where we always pass old values and ignores sendOldValues. Reviewers: Matthias J. Sax <mjsax@apache.org>

Refactor the materialization for source KTables in the way that: If Materialized.as(queryableName) is specified, materialize; If the downstream operator requires to fetch from this KTable via ValueGetters, materialize; If the downstream operator requires to send old values, materialize. Otherwise do not materialize the KTable. E.g. builder.table("topic").filter().toStream().to("topic") would not create any state stores. There's a couple of minor changes along with PR as well: KTableImpl's queryableStoreName and isQueryable are merged into queryableStoreName only, and if it is null it means not queryable. As long as it is not null, it should be queryable (i.e. internally generated names will not be used any more). To achieve this, splitted MaterializedInternal.storeName() and MaterializedInternal.queryableName(). The former can be internally generated and will not be exposed to users. QueryableName can be modified to set to the internal store name if we decide to materialize it during the DSL parsing / physical topology generation phase. And only if queryableName is specified the corresponding KTable is determined to be materialized. Found some overlapping unit tests among KTableImplTest, and KTableXXTest, removed them. There are a few typing bugs found along the way, fixed them as well. ----------------------- This PR is an illustration of experimenting a poc towards logical materializations. Today we've logically materialized the KTable for filter / mapValues / transformValues if queryableName is not specified via Materialized, but whenever users specify queryableName we will still always materialize. My original goal is to also consider logically materialize for queryable stores, but when implementing it via a wrapped store to apply the transformations on the fly I realized it is tougher than I thought, because we not only need to support fetch or get, but also needs to support range queries, approximateNumEntries, and isOpen etc as well, which are not efficient to support. So in the end I'd suggest we still stick with the rule of always materializing if queryableName is specified, and only consider logical materialization otherwise. Reviewers: Bill Bejeck <bill@confluent.io>, Matthias J. Sax <mjsax@apache.org>

…shListener (apache#6017) This is a follow-up PR from the previous PR apache#5779, where KTabeSource always get old values from the store even if sendOldValues. It gets me to make a pass over all the KTable/KStreamXXX processor to push the sendOldValues at the callers in order to avoid unnecessary store reads. More details: ForwardingCacheFlushListener and TupleForwarder both need sendOldValues as parameters. a. For ForwardingCacheFlushListener it is not needed at all, since its callers XXXCachedStore already use the sendOldValues values passed from TupleForwarder to avoid getting old values from underlying stores. b. For TupleForwarder, it actually only need to pass the boolean flag to the cached store; and then it does not need to keep it as its own variable since the cached store already respects the boolean to pass null or the actual value.. The only other minor bug I found from the pass in on KTableJoinMerge, where we always pass old values and ignores sendOldValues. Reviewers: Matthias J. Sax <mjsax@apache.org>

guozhangwang added 13 commits October 6, 2018 16:35

minor fixes

1434325

Merge branch 'trunk' of https://github.com/apache/kafka into KXXXX-lo…

fbf8c7b

…cal-materialization

minor fixes

d63bfcf

dummy

8f3e598

revert filter

a771267

revert KTableImpl

9b45dc3

dummy

dd5e6b7

fix minor tests

800fbce

fix minor tests

7e843e8

minor

164024b

remove imports

dd6712e

fix unit tests

89cfdb5

Merge branch 'trunk' of https://github.com/apache/kafka into KXXXX-lo…

dd13593

…cal-materialization t

guozhangwang commented Oct 11, 2018

View reviewed changes

bbejeck reviewed Nov 21, 2018

View reviewed changes

rebase from trunk

c141a3f

mjsax added the streams label Nov 28, 2018

bbejeck reviewed Nov 28, 2018

View reviewed changes

Merge branch 'trunk' of https://github.com/apache/kafka into KXXXX-lo…

ce88eab

…cal-materialization

guozhangwang added 3 commits November 29, 2018 17:25

github comments

7827b13

Merge branch 'trunk' of https://github.com/apache/kafka into KXXXX-lo…

f913fea

…cal-materialization

refactor unit tests further

37e4cba

bbejeck approved these changes Dec 4, 2018

View reviewed changes

mjsax reviewed Dec 6, 2018

View reviewed changes

guozhangwang added 4 commits December 6, 2018 17:04

Merge branch 'trunk' of https://github.com/apache/kafka into KXXXX-lo…

ecd70f8

…cal-materialization

github comments

be64fdc

unit tests

fa2294c

one more fix

d5d358b

mjsax approved these changes Dec 8, 2018

View reviewed changes

Merge branch 'trunk' of https://github.com/apache/kafka into KXXXX-lo…

12caef4

…cal-materialization

guozhangwang added 2 commits December 8, 2018 21:35

github comments

0c11db0

minor fix

7d02ad5

guozhangwang merged commit c0353d8 into apache:trunk Dec 9, 2018

guozhangwang mentioned this pull request Dec 9, 2018

KAFKA-6036: Follow-up; cleanup sendOldValues logic ForwardingCacheFlushListener #6017

Merged

3 tasks

mjsax mentioned this pull request Mar 14, 2019

KAFKA-7075: Allow Topology#addGlobalStore to add a window store #5725

Closed

3 tasks

guozhangwang deleted the KXXXX-local-materialization branch April 25, 2020 00:06

Conversation

guozhangwang commented Oct 11, 2018

Committer Checklist (excluded from commit message)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guozhangwang commented Oct 11, 2018

Uh oh!

bbejeck left a comment

Choose a reason for hiding this comment

Uh oh!

bbejeck left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guozhangwang commented Nov 30, 2018

Uh oh!

guozhangwang commented Nov 30, 2018

Uh oh!

bbejeck commented Dec 4, 2018

Uh oh!

bbejeck left a comment

Choose a reason for hiding this comment

Uh oh!

guozhangwang commented Dec 5, 2018

Uh oh!

bbejeck commented Dec 5, 2018

Uh oh!

mjsax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!