Implement bundle features in local store. #3200

wu-hui · 2020-06-10T14:40:29Z

No description provided.

# Conflicts: # packages/firestore/src/util/bundle_reader.ts # packages/firestore/test/unit/util/bundle.test.ts

Fix change buffer bug

# Conflicts: # packages/firestore/src/core/bundle.ts # packages/firestore/src/local/bundle_cache.ts # packages/firestore/src/local/indexeddb_bundle_cache.ts # packages/firestore/src/local/local_serializer.ts # packages/firestore/src/local/memory_bundle_cache.ts # packages/firestore/src/platform/platform.ts # packages/firestore/src/platform_browser/browser_platform.ts # packages/firestore/src/platform_node/node_platform.ts # packages/firestore/src/util/bundle_reader.ts # packages/firestore/test/unit/local/bundle_cache.test.ts # packages/firestore/test/unit/platform/platform.test.ts # packages/firestore/test/unit/specs/spec_test_components.ts # packages/firestore/test/unit/util/bundle.test.ts # packages/firestore/test/util/test_platform.ts

schmidt-sebastian

Sorry about the slow review. In general, this looks fine and most of my comments are on style/naming.

schmidt-sebastian · 2020-06-16T17:51:16Z

packages/firestore/src/core/bundle.ts

+ * Represents the [metadata, document] pairs from the bundles in an array.
+ */
+export type BundledDocuments = Array<
+  [bundleProto.BundledDocumentMetadata, api.Document | undefined]


I would prefer if you a nested object here: Array<{metadata: ..., document?: }>. Index-based typing is very hard to parse for readers of the code.

packages/firestore/src/core/bundle.ts

schmidt-sebastian · 2020-06-16T17:56:15Z

packages/firestore/src/local/local_store.ts

@@ -541,65 +548,31 @@ export class LocalStore {
          }
        });

+        const documentBuffer = this.remoteDocuments.newChangeBuffer({


it is much more efficient to keep a global change buffer, as a document that is part of multiple targets will only be written once per RemoteEvent rather than once per target. We should keep this invariant.

Done, thanks for the catch.

schmidt-sebastian · 2020-06-16T17:58:11Z

packages/firestore/src/local/local_store.ts

+   * Returns a promise of a boolean to indicate if the given bundle has already
+   * been loaded and the create time is newer than the current loading bundle.
+   */
+  isNewerBundleLoaded(


hasNewerBundle?

schmidt-sebastian · 2020-06-16T17:59:43Z

packages/firestore/src/local/remote_document_change_buffer.ts

+/**
+ * Represents a document change to be applied to remote document cache.
+ */
+class RemoteDocumentChange {


This could be a simple interface, which would remove any size overhead since interfaces "disappear" when transpiled to JS. The only downside (in terms of code size) is that you will not be able to use this constructor anymore, but instead have to assign object properties directly.

schmidt-sebastian · 2020-06-16T21:30:31Z

packages/firestore/test/unit/local/local_store.test.ts

+    expected: boolean
+  ): LocalStoreTester {
+    this.promiseChain = this.promiseChain.then(() => {
+      return this.localStore.isNewerBundleLoaded(metadata).then(actual => {


Name is also inconsistent here. FWIW, toHaveNewerBundle matches my name suggestion from above.

schmidt-sebastian · 2020-06-16T21:32:05Z

packages/firestore/test/unit/local/local_store.test.ts

+      .toContain(doc('foo/bar1', 1, { val: 'to-delete' }))
+      .after(
+        bundledDocuments([
+          doc('foo/bar', 1, { sum: 1336 }),


Should this be version 2?

We may also want to skip bundle document loading for documents that we already have cached at that version. The local documents seem safer than replacing them with a bundled document that could be from an arbitrary source.

Not there were no foo/bar when loading the bundle. I changed the name to make it less confusing.

Also added a test to check when the existing version is the same.

packages/firestore/test/unit/local/local_store.test.ts

schmidt-sebastian · 2020-06-16T21:34:52Z

packages/firestore/test/unit/local/local_store.test.ts

+
+  it('handles PatchMutation with Transform -> BundledDocuments', () => {
+    // Note: This test reflects the current behavior, but it may be preferable
+    // to replay the mutation once we receive the first value from the backend.


I think what is implemented is the better behavior, since the document will likely end up looking like the local mutation. What do you think?

This is copied from the remote event tests. I am actually not sure why it was there and is it still valid.

I think we can discuss this more with the team in one of the standups.

schmidt-sebastian · 2020-06-16T21:35:37Z

packages/firestore/test/util/helpers.ts

@@ -401,6 +404,55 @@ export function docUpdateRemoteEvent(
  return aggregator.createRemoteEvent(doc.version);
 }

+export class TestBundledDocuments {


Can this be inlined? bundledDocuments would just return BundledDocuments.

I added this because it hard to add code to after(op) method otherwise, suggestions welcome.

wu-hui

Thanks! Given the size of my recent PRs, I think you are already very quick in reviewing them!

wu-hui · 2020-06-17T14:58:43Z

packages/firestore/src/core/bundle.ts

+ * Represents the [metadata, document] pairs from the bundles in an array.
+ */
+export type BundledDocuments = Array<
+  [bundleProto.BundledDocumentMetadata, api.Document | undefined]


packages/firestore/src/core/bundle.ts

wu-hui · 2020-06-17T15:23:12Z

packages/firestore/src/core/component_provider.ts

@@ -209,10 +212,12 @@ export class IndexedDbComponentProvider extends MemoryComponentProvider {
  }

  createLocalStore(cfg: ComponentConfiguration): LocalStore {
+    const serializer = cfg.platform.newSerializer(cfg.databaseInfo.databaseId);


wu-hui · 2020-06-17T15:23:25Z

packages/firestore/src/core/component_provider.ts

    return new MultiTabLocalStore(
      this.persistence,
      new IndexFreeQueryEngine(),
-      cfg.initialUser
+      cfg.initialUser,
+      new BundleConverter(serializer)


wu-hui · 2020-06-17T15:26:10Z

packages/firestore/src/local/indexeddb_remote_document_cache.ts

            'Cannot add a document with a read time of zero'
          );
          const doc = this.documentCache.serializer.toDbRemoteDocument(
-            maybeDocument,
-            this.readTime
+            documentChange.maybeDoc!,


wu-hui · 2020-06-17T16:13:36Z

packages/firestore/test/unit/local/local_store.test.ts

+
+    this.promiseChain = this.promiseChain
+      .then(() => {
+        return this.localStore.applyBundledDocuments(documents);


You were right!

wu-hui · 2020-06-17T16:14:19Z

packages/firestore/test/unit/local/local_store.test.ts

+    expected: boolean
+  ): LocalStoreTester {
+    this.promiseChain = this.promiseChain.then(() => {
+      return this.localStore.isNewerBundleLoaded(metadata).then(actual => {


wu-hui · 2020-06-17T16:21:46Z

packages/firestore/test/unit/local/local_store.test.ts

+      .toContain(doc('foo/bar1', 1, { val: 'to-delete' }))
+      .after(
+        bundledDocuments([
+          doc('foo/bar', 1, { sum: 1336 }),


Not there were no foo/bar when loading the bundle. I changed the name to make it less confusing.

Also added a test to check when the existing version is the same.

packages/firestore/test/unit/local/local_store.test.ts

wu-hui · 2020-06-17T16:41:34Z

packages/firestore/test/unit/local/local_store.test.ts

+
+  it('handles PatchMutation with Transform -> BundledDocuments', () => {
+    // Note: This test reflects the current behavior, but it may be preferable
+    // to replay the mutation once we receive the first value from the backend.


This is copied from the remote event tests. I am actually not sure why it was there and is it still valid.

I think we can discuss this more with the team in one of the standups.

schmidt-sebastian

Some feedback that may trigger some pushback :)

schmidt-sebastian · 2020-06-18T17:45:17Z

packages/firestore/src/core/bundle.ts

+export type BundledDocuments = Array<{
+  metadata: bundleProto.BundledDocumentMetadata;
+  document: api.Document | undefined;
+}>;


Suggested change

export type BundledDocuments = Array<{

metadata: bundleProto.BundledDocumentMetadata;

document: api.Document | undefined;

}>;

export type BundledDocuments = Array<BundledDocument>;

It might be cleaner just to inline this type.

Done, I forgot to do it earlier.

schmidt-sebastian · 2020-06-18T17:59:56Z

packages/firestore/src/local/local_store.ts

+   * LocalDocuments are re-calculated if there are remaining mutations in the
+   * queue.
+   */
+  applyBundleDocuments(documents: BundledDocuments): Promise<MaybeDocumentMap> {


To make today's code review meaningfully different from Tuesday's review, I think maybe we should try to make all bundle code tree-shakeable? I threw together some pseudocode here: https://gist.github.com/schmidt-sebastian/932ffd496e1129309b9d6b4861d2c879. The benefits of this design is that it would allows to make all bundling code optional (once we do the same thing for the persistence implementation).

It might make sense to do this as a follow-up since this PR is basically ready to be submitted.

Happy to make this tree-shakeable, not sure I did everything right though.

I'll update my other PRs to make it tree-shakeable as well.

schmidt-sebastian · 2020-06-18T18:04:03Z

packages/firestore/src/local/local_store.ts

+   * @param remoteVersion The read time of the documents to be applied, it is
+   * a `SnapshotVersion` if all documents have the same read time, or a `DocumentVersionMap` if
+   * they have different read times.
+   */


Do you mind adding a short description for all documents?

schmidt-sebastian · 2020-06-18T18:09:10Z

packages/firestore/src/local/local_store.ts

+    documentBuffer: RemoteDocumentChangeBuffer,
+    documents: MaybeDocumentMap,
+    globalVersion: SnapshotVersion | undefined,
+    documentVersions: DocumentVersionMap | undefined


Looking at this more, would it be possible to use the readTime from the bundle as the globalVersion and drop documentVersions? AFAIK the only real guarantee that we have to make is that a changed document has a changed read time that is higher than the new document version.

If that is not possible, I think we should explore adding the read time to MaybeDocument (it's already part of DbRemoteDocument, so we do have the data). That might be a relatively simple way to improve the code flow here.

I think you are right that readTime is used only to fetch newly changed docs, which means setting a higher than actual readTime for documents is fine: the SDK just need to process more documents than required.

The performance gain here might be beneficial though, given that user could load a big bundle there and a more accurate readTime will likely have a sizeable reduction to the query result.

One other reason to keep document based read time is I don't want us to design future features having to account for the fact that readTime can be higher than actual. I cannot think of a case now, so this is mostly design for the future (that might never happen).

MaybeDocument is constructed mostly from remote (for example: Serializer.fromDocument does not have a read time there) so I am not sure if we can put readTime there.

I think it would be possible to add a readTime to MaybeDocument. It looks like we would need to change a lot of our plumbing, so I am ok with leaving as is. We could add a TODO if this is something that we think would clean up our code in general.

schmidt-sebastian · 2020-06-18T18:14:46Z

packages/firestore/src/local/local_store.ts

+      bundleMetadata.createTime!
+    );
+    return this.persistence
+      .runTransaction('isNewerBundleLoaded', 'readonly', transaction => {


Suggested change

.runTransaction('isNewerBundleLoaded', 'readonly', transaction => {

.runTransaction('hasNewerBundle', 'readonly', transaction => {

schmidt-sebastian · 2020-06-18T18:20:55Z

packages/firestore/src/local/remote_document_change_buffer.ts

+  // The document in this change, null if it is a removal from the cache.
+  readonly maybeDocument: MaybeDocument | null;
+  // The timestamp when this change is read.
+  readonly readTime?: SnapshotVersion;


The use of "null" and "undefined" for "not set" is a bit inconsistent here.

FWIW, we could drop readTime if we add it to MaybeDocument.

Updated to be more consistent.

schmidt-sebastian · 2020-06-18T18:24:49Z

packages/firestore/test/unit/local/local_store.test.ts

+      .then(() => {
+        return this.localStore.applyBundleDocuments(documents);
+      })


Suggested change

.then(() => {

return this.localStore.applyBundleDocuments(documents);

})

.then(() => this.localStore.applyBundleDocuments(documents))

schmidt-sebastian · 2020-06-18T18:25:08Z

packages/firestore/test/unit/local/local_store.test.ts

+    this.promiseChain = this.promiseChain.then(() => {
+      return this.localStore.saveBundle(metadata);
+    });


Suggested change

this.promiseChain = this.promiseChain.then(() => {

return this.localStore.saveBundle(metadata);

});

this.promiseChain = this.promiseChain.then(() => this.localStore.saveBundle(metadata));

schmidt-sebastian · 2020-06-18T18:29:48Z

packages/firestore/test/util/helpers.ts

+  const result: BundledDocuments = [];
+  for (const d of documents) {
+    if (d instanceof NoDocument) {
+      result.push({
+        metadata: {
+          name: JSON_SERIALIZER.toName(d.key),
+          readTime: JSON_SERIALIZER.toVersion(d.version),
+          exists: false
+        },
+        document: undefined
+      });
+    } else if (d instanceof Document) {
+      result.push({
+        metadata: {
+          name: JSON_SERIALIZER.toName(d.key),
+          readTime: JSON_SERIALIZER.toVersion(d.version),
+          exists: true
+        },
+        document: JSON_SERIALIZER.toDocument(d)
+      });
+    }
+  }
+


Maybe:

Suggested change

const result: BundledDocuments = [];

for (const d of documents) {

if (d instanceof NoDocument) {

result.push({

metadata: {

name: JSON_SERIALIZER.toName(d.key),

readTime: JSON_SERIALIZER.toVersion(d.version),

exists: false

},

document: undefined

});

} else if (d instanceof Document) {

result.push({

metadata: {

name: JSON_SERIALIZER.toName(d.key),

readTime: JSON_SERIALIZER.toVersion(d.version),

exists: true

},

document: JSON_SERIALIZER.toDocument(d)

});

}

}

const result = documents.map(d => {

return {

metadata: {

name: JSON_SERIALIZER.toName(d.key),

readTime: JSON_SERIALIZER.toVersion(d.version),

exists: d instanceof Document

},

document: d instanceof Document ? JSON_SERIALIZER.toDocument(d) : undefined

});

});

Done, thanks!

schmidt-sebastian · 2020-06-22T23:31:32Z

packages/firestore/src/local/local_store.ts

@@ -89,6 +95,40 @@ export interface QueryResult {
  readonly remoteKeys: DocumentKeySet;
 }

+/**
+ * Base of local store implementations. This is mainly used to define fields


s/local store/

schmidt-sebastian · 2020-06-22T23:35:11Z

packages/firestore/src/local/local_store.ts

+ * that are used by tree-shakeable free functions that implement optional
+ * local store features.
+ */
+class LocalStoreBase {


So the idea behind the pattern that is used in Datastore/DatastoreImpl (and just as of today, also in Target/TargetImpl) is to have a main class (in this case LocalStore) that doesn't change its public API.

So you would keep LocalStore as is (and continue to export it), and then you would create LocalStoreImpl that makes fields public that you only need to be public in this module. Then you would add a newLocalStore function that creates a LocalStoreImpl, but is typed to return a LocalStore. You then end up with a module that has a bunch of public members that are not leaked externally (in our code base).

Sorry I misunderstood earlier.

As discussed offline, we will need to make LocalStore and SyncEngine interfaces to achieve this because of existing MultiTab* implementations, which is best done from master.

schmidt-sebastian · 2020-06-22T23:40:06Z

packages/firestore/src/local/local_store.ts

+   * Note: this function will use `documentVersions` if it is defined;
+   * when it is not defined, resorts to `globalVersion`.
+   */
+  prepareApplyingDocuments(


s/prepareApplyingDocuments/prepareToApplyDocuments/ (or back to applyDocuments)

Or rather: populateDocumentChangeBuffer?

populateDocumentChangeBuffer sounds good.

schmidt-sebastian · 2020-06-22T23:41:21Z

packages/firestore/src/local/local_store.ts

@@ -642,6 +628,84 @@ export class LocalStore {
      });
  }

+  /**
+   * Prepares document change buffer resulted from documents to be applied,


Nit: This line needs a rewrite.

schmidt-sebastian · 2020-06-22T23:43:46Z

packages/firestore/src/local/local_store.ts

+    txn: PersistenceTransaction,
+    documentBuffer: RemoteDocumentChangeBuffer,
+    documents: MaybeDocumentMap,
+    globalVersion: SnapshotVersion | undefined,


Can we make this non-optional and use SnapshotVersion.min() for the bundle loader?

schmidt-sebastian · 2020-06-22T23:44:35Z

packages/firestore/src/local/local_store.ts

+        let docReadVersion: SnapshotVersion | null = null;
+        if (documentVersions) {
+          docReadVersion = documentVersions.get(key);
+        } else if (globalVersion) {
+          docReadVersion = globalVersion;
+        }
+        debugAssert(!!docReadVersion, 'Document read version must exist');


Suggested change

let docReadVersion: SnapshotVersion | null = null;

if (documentVersions) {

docReadVersion = documentVersions.get(key);

} else if (globalVersion) {

docReadVersion = globalVersion;

}

debugAssert(!!docReadVersion, 'Document read version must exist');

const readTime = documentVersions?.get(key) ?? globalVersion

Done, thanks!

schmidt-sebastian · 2020-06-22T23:46:13Z

packages/firestore/src/local/local_store.ts

@@ -1049,11 +1114,13 @@ export class MultiTabLocalStore extends LocalStore {
  }

  setNetworkEnabled(networkEnabled: boolean): void {
-    this.persistence.setNetworkEnabled(networkEnabled);
+    (this.persistence as IndexedDbPersistence).setNetworkEnabled(


If you add a persistence: IndexedDbPersistence field and assign it manually (similar to mutationQueue), this cast will go away.

schmidt-sebastian · 2020-06-22T23:46:59Z

packages/firestore/src/local/local_store.ts

+  localStore: LocalStore,
+  documents: BundledDocuments
+): Promise<MaybeDocumentMap> {
+  const bundleConverter = new BundleConverter(localStore.serializer);


If you follow the Datastore pattern, you would start this off with:

const localStoreImpl = debugCast(localStore, LocalStoreImpl);

Otherwise, this functions are tree-shakeable as is. Thanks!

Will do once we have addressed inheritance issue in master.

# Conflicts: # packages/firestore/src/core/component_provider.ts # packages/firestore/src/local/indexeddb_remote_document_cache.ts # packages/firestore/src/local/local_store.ts # packages/firestore/src/local/remote_document_change_buffer.ts # packages/firestore/test/unit/local/local_store.test.ts

schmidt-sebastian · 2020-06-24T21:17:29Z

packages/firestore/src/core/bundle.ts

+
+/**
+ * Represents a bundled document, including the metadata and the document
+ * itself, if exists.


missing "it"

schmidt-sebastian · 2020-06-24T21:18:03Z

packages/firestore/src/core/bundle.ts

+  }
+
+  /**
+   * Converts a [metadata, document] pair to a MaybeDocument.


Suggested change

* Converts a [metadata, document] pair to a MaybeDocument.

* Converts a BundledDocument to a MaybeDocument.

schmidt-sebastian · 2020-06-24T21:22:40Z

packages/firestore/src/local/local_store.ts

@@ -347,6 +357,7 @@ class LocalStoreImpl implements LocalStore {
      this.mutationQueue,
      this.persistence.getIndexManager()
    );
+    this.bundleCache = persistence.getBundleCache();


I think we have done enough iterations on this PR, but ideally this would not be a field on LocalStore (as it breaks tree-shaking). Could BundleCache take LocalStore as an argument? Please explore in a follow up.

I see. Will do a followup PR.

schmidt-sebastian · 2020-06-24T21:33:02Z

packages/firestore/src/local/local_store.ts

+      let changedDocs = maybeDocumentMap();
+      documents.forEach((key, doc) => {
+        const existingDoc = existingDocs.get(key);
+        const docReadTime = documentVersions?.get(key) ?? globalVersion;


Nit: I probably suggested this, but this is functionally equivalent to documentVersions?.get(key) || globalVersion, which generates a lot less code. ?? is only needed if you only want to check for undefined or null and explicitly want to not fallback on "" (empty string), 0 or false.

if (doc?.version || 'foo') => if ((doc === null || doc === void 0 ? void 0 : doc.version) || 'foo') if (doc?.version ?? 'foo') => if ((_a = doc === null || doc === void 0 ? void 0 : doc.version) !== null && _a !== void 0 ? _a : 'foo')

As an example for where ?? works but || doesn't, consider:

class Settings { enableAmazingFeature?: boolean; } // This is now always true if (settings.enableAmazingFeature || true) {} // This is true iff enableAmazingFeature=undefined or enableAmazingFeature=true if (settings.enableAmazingFeature ?? true) {}

I see, applied. Thanks!

wu-hui added 19 commits May 21, 2020 13:49

Renaming interfaces without leading I

9602712

Initial commit of bundle reading - for web only.

c5e783e

Tests only run when it is not Node.

5e7fb89

Fix redundant imports

1ee1615

Fix missing textencoder

18f0be1

Remove generator.

aa455bf

Support bundle reader for Node

78248cd

Fix rebase errors.

83160a1

Remote 'only'

24e10cb

Merge branch 'wuandy/Bundles' into wuandy/BundleReaderNode

9d6edc5

# Conflicts: # packages/firestore/src/util/bundle_reader.ts # packages/firestore/test/unit/util/bundle.test.ts

Merge branch 'wuandy/Bundles' into wuandy/BundleReaderNode

4cbe608

# Conflicts: # packages/firestore/src/util/bundle_reader.ts # packages/firestore/test/unit/util/bundle.test.ts

Added more comments, and more tests for Node.

4313e51

Implement BundleCache.

296cfc4

Add applyBundleDocuments to local store.

fff3d36

Add rest of bundle service to localstore

fb762de

Fix change buffer bug

Simplify change buffer get read time logic.

1ec4182

Fix lint errors

cd3ab7a

Add comments.

d991c75

Change localstore to check for newer bundle directly.

af097c5

wu-hui added the api: firestore label Jun 10, 2020

wu-hui changed the base branch from wuandy/Bundles to wuandy/BundlePersistence June 10, 2020 14:41

wu-hui added 2 commits June 13, 2020 22:48

A little more comments.

7ea8335

wu-hui changed the base branch from wuandy/BundlePersistence to wuandy/Bundles June 14, 2020 03:21

wu-hui requested a review from schmidt-sebastian June 15, 2020 15:31

wu-hui assigned schmidt-sebastian Jun 15, 2020

schmidt-sebastian reviewed Jun 16, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Jun 16, 2020

wu-hui commented Jun 17, 2020

View reviewed changes

wu-hui assigned schmidt-sebastian and unassigned wu-hui Jun 17, 2020

Address comments.

5a38f0a

schmidt-sebastian reviewed Jun 18, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Jun 18, 2020

wu-hui added 2 commits June 19, 2020 10:50

Address comments 2

9340d6a

Make it tree-shakeable.

6fc7f4a

wu-hui assigned schmidt-sebastian and unassigned wu-hui Jun 19, 2020

schmidt-sebastian reviewed Jun 22, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Jun 22, 2020

More comments addressing.

bdbd70a

wu-hui assigned schmidt-sebastian and unassigned wu-hui Jun 23, 2020

schmidt-sebastian approved these changes Jun 24, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui Jun 24, 2020

Another around of comments

6fc92ee

wu-hui merged commit e8b9ec2 into wuandy/Bundles Jun 25, 2020

firebase locked and limited conversation to collaborators Jul 26, 2020

	.runTransaction('isNewerBundleLoaded', 'readonly', transaction => {
	.runTransaction('hasNewerBundle', 'readonly', transaction => {

	* Converts a [metadata, document] pair to a MaybeDocument.
	* Converts a BundledDocument to a MaybeDocument.

Implement bundle features in local store. #3200

Implement bundle features in local store. #3200

Uh oh!

Conversation

wu-hui commented Jun 10, 2020

Uh oh!

schmidt-sebastian 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!

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!

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!

wu-hui left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian 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