Bundle's named query resume. #3395

wu-hui · 2020-07-12T01:17:07Z

No description provided.

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

Fix change buffer bug

Two TODO: 1. load from secondary tab. 2. don't fail async queue.

schmidt-sebastian · 2020-07-28T22:52:57Z

packages/firestore/src/api/database.ts

@@ -1925,7 +1936,8 @@ export class Query<T = firestore.DocumentData> implements firestore.Query<T> {
  constructor(
    public _query: InternalQuery,
    readonly firestore: Firestore,
-    protected readonly _converter: firestore.FirestoreDataConverter<T> | null
+    protected readonly _converter: firestore.FirestoreDataConverter<T> | null,
+    private _readFrom?: SnapshotVersion


Please rename everywhere.

schmidt-sebastian · 2020-07-28T22:57:15Z

packages/firestore/src/core/sync_engine.ts

@@ -175,7 +175,7 @@ export interface SyncEngine extends RemoteSyncer {
   * server. All the subsequent view snapshots or errors are sent to the
   * subscribed handlers. Returns the initial snapshot.
   */
-  listen(query: Query): Promise<ViewSnapshot>;
+  listen(query: Query, readFrom?: SnapshotVersion): Promise<ViewSnapshot>;


I think I misunderstood your code when I looked at it yesterday (was a bit rushed). It looks like the idea is that the readTime comes from EventManager.listem(). That doesn't seem correct.

The code that is interested in a query shouldn't need to know that the query was bundled at one point and it shouldn't even know about the version that it is at. Think about this as similar to resume tokens - LocalStore knows about them, but the API surface does not. Once persisted, the readTime should only need to be passed from LocalStore to RemoteStore.

A named query should be a helper to quickly construct the Query that was bundled. It should not be a requirement for bundling. If a user happens to reconstruct the same query from scratch, they should get the same semantics (e.g. query resumption).

As a first step, it looks like this means that we should remove readTime from the BundleCache and move it to the TargetCache.

schmidt-sebastian · 2020-07-28T23:02:48Z

packages/firestore/exp/src/api/database.ts

+    return null;
+  }
+
+  // TODO(wuandy): make this work with exp build.


You simply imported the wrong Query:

diff --git a/packages/firestore/exp/src/api/database.ts b/packages/firestore/exp/src/api/database.ts index 52f80f80..e613e8a1 100644 --- a/packages/firestore/exp/src/api/database.ts +++ b/packages/firestore/exp/src/api/database.ts @@ -44,13 +44,14 @@ import { cast } from '../../../lite/src/api/util'; import { Code, FirestoreError } from '../../../src/util/error'; import { Deferred } from '../../../src/util/promise'; import { LruParams } from '../../../src/local/lru_garbage_collector'; -import { CACHE_SIZE_UNLIMITED, Query } from '../../../src/api/database'; +import { CACHE_SIZE_UNLIMITED } from '../../../src/api/database'; import { DatabaseId, DatabaseInfo } from '../../../src/core/database_info'; import { indexedDbStoragePrefix, indexedDbClearPersistence } from '../../../src/local/indexeddb_persistence'; import { LoadBundleTask } from '../../../src/api/bundle'; +import {Query} from "../../../lite"; /** * The root reference to the Firestore database and the entry point for the @@ -322,7 +323,5 @@ export async function namedQuery( return null; } - // TODO(wuandy): make this work with exp build. - return null; - // return new Query(namedQuery.query, firestoreImpl, null, namedQuery.readTime); + return new Query(firestoreImpl,namedQuery.query, null, namedQuery.readTime); }

schmidt-sebastian · 2020-07-28T23:05:58Z

packages/firestore/src/api/database.ts

@@ -1925,7 +1936,8 @@ export class Query<T = firestore.DocumentData> implements firestore.Query<T> {
  constructor(
    public _query: InternalQuery,
    readonly firestore: Firestore,
-    protected readonly _converter: firestore.FirestoreDataConverter<T> | null
+    protected readonly _converter: firestore.FirestoreDataConverter<T> | null,
+    private _readTime?: SnapshotVersion


_readTime should only be part of TargetData and not be used in the API surface.

schmidt-sebastian · 2020-07-28T23:06:45Z

packages/firestore/src/api/database.ts

+      {
+        readFrom: this._readTime,
+        includeMetadataChanges: options.includeMetadataChanges
+      },


This shouldn't be needed.

schmidt-sebastian · 2020-07-28T23:17:31Z

packages/firestore/src/remote/serializer.ts

@@ -975,6 +975,11 @@ export function toTarget(

  if (targetData.resumeToken.approximateByteSize() > 0) {
    result.resumeToken = toBytes(serializer, targetData.resumeToken);
+  } else if (targetData.snapshotVersion.compareTo(SnapshotVersion.min()) > 0) {


Can we:

Always update the read time for each TargetData?

Always persist it?

If i understand correctly, this is the same comment that we should persist named queries in target cache as well?

Not quite. I would assume that targetData.snapshotVersion.compareTo(SnapshotVersion.min()) > 0 is always true (unless the target just got allocated, in which case the existing snapshot version should also be null). Can you verify that, and if so drop the if-condition here?

if (resumeToken) { ... } result.readTime = toTimestamp( serializer, targetData.snapshotVersion.toTimestamp() );

Hmm, problem is in the proto resumeToken and readTime are in oneof type.

This brings your question though: should we always use readtime to resume. Because this change makes resumeToken always overwritten. I think functionally this works but we should clean up.

So I think the problem is that we can't tell which one is newer and we likely want the newer of the two. In stand up we discussed that the resumeToken is preferred, since it contains more information to help the backend resume at the right query position. I would think we could do something like this:

If readTime > snapshotVersion, use readTime

Otherwise use resumeToken

Does that work?

As further discussed, the fix is to drop resumeToken when we update a target.

Still, there are many tests rely on this behaviour now, that i want to remove the check later.

schmidt-sebastian · 2020-07-28T23:19:43Z

packages/firestore/test/integration/api_internal/bundle.test.ts

+        .orderBy('bar', 'desc')
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        .limit(1) as any)._query
+    );


It would be so nice if these tests lived in "api", which would mean that they get tested via firestore-exp and the minified builds. Can we use pre-encoded bundle and load it from JSON:?

I can give it a try, but in a later PR?

schmidt-sebastian · 2020-07-28T23:25:34Z

packages/firestore/test/unit/specs/spec_builder.ts

+    query: Query,
+    readFrom: TestSnapshotVersion
+  ): this {
+    // Note we do not call this.nextStep() here because we reuse userListens().


I am not sure this needs to be spec tested, and if anything it seems like it should be two steps (one that loads the query, one that listens to it). To me, restoring a Query by name is separate from query execution. It doesn't need to interact with the rest of the system (SyncEngine, RemoteStore).

The test could be simple unit tests for the Query class:

const query1 = query.where('foo', > 1); storeInBundle('fooQuery', 'querye.where('foo', > 1)}'); const query2 = namedQuery('fooQuery'); expect(query1).to.equal(query2);

Done. I moved it to localstore test though.

schmidt-sebastian · 2020-07-28T23:26:45Z

packages/firestore/test/unit/util/bundle_data.ts

  addNamedQuery(
    name: string,
    readTime: api.Timestamp,
-    bundledQuery: BundledQuery
+    query: Query,
+    limitType?: LimitType


Query already has the LimitType.

This is confusing I agree: the limit type here is from bundles types because bundles never really save an actual limit-to-last query. It saves a limit query with same orderBy and cursors, with a type indicator that it's a limit to last query.

Using the limit type from Query would mean we need to construct a limit to last query that translates to a limit query that is equivalent query saved in bundles.

Would it at all be possible to do the Query translation (flip the order) here? Not sure if feasible, but then we could drop the LimitType and reduce the confusion.

schmidt-sebastian · 2020-07-28T23:27:12Z

packages/firestore/test/unit/specs/spec_test_runner.ts

+export interface SpecUserListen {
+  targetId: TargetId;
+  query: string | SpecQuery;
+  fromName?: string;


This should ideally be removed (or moved to a different step).

schmidt-sebastian · 2020-08-04T16:35:31Z

packages/firestore/exp/src/api/database.ts

+): Promise<firestore.Query | null> {
+  const firestoreImpl = cast(firestore, Firestore);
+  const client = await firestoreImpl._getFirestoreClient();
+  const namedQuery = await client.getNamedQuery(name);


This needs merging again. The call to getNamedQuery should call a free-standing version that takes LocalStore as an argument. All methods in FirestoreClient have been updated.

Let me know if you need help with this. You can also defer to a follow up PR.

Yeah, I'd like to defer to my a follow up PR.

schmidt-sebastian · 2020-08-04T16:36:45Z

packages/firestore/src/local/local_store.ts

@@ -236,10 +237,21 @@ export interface LocalStore {
   * they don't get GC'd. A target must be allocated in the local store before
   * the store can be used to manage its view.
   *
+   * @param {SnapshotVersion} readTime The snapshot version the allocated target


Since we are writing TypeScript, no need for {SnapshotVersion} (and also {PersistenceTransaction}).

schmidt-sebastian · 2020-08-04T16:40:18Z

packages/firestore/src/local/local_store.ts

-                  .next(() => targetData);
-              });
-            }
+  _allocateOperation(


Hm. Can you come up with "normal" name for this? I can't think of anything either, otherwise I would suggest something. You can drop the underscore since this is not a public class and you might be able to make this method private.

No longer needed.

schmidt-sebastian · 2020-08-04T16:42:05Z

packages/firestore/src/local/local_store.ts

+      );
+    }
+    return allocateOperation.then(targetData => {
+      // If Multi-Tab is enabled, the existing target data may be newer than


Is there a reason this cannot be done in _allocateOperation? If you move this code, then saveNamedQuery doesn't have to call allocateTarget anymore and you can drop the transaction and the readTime argument.

Nope, i was not sure if persistence transaction could be nested. Took your advice and it looks much better now!

schmidt-sebastian · 2020-08-04T16:42:58Z

packages/firestore/src/local/local_store.ts

+        fromVersion(query.readTime!),
+        transaction
+      );
+      return localStoreImpl.bundleCache.saveNamedQuery(transaction, query);


Couple things:

I think these two calls don't need to happen in the same transaction, as long as the allocateTarget happens before saveNamedQuery. That would then undo most of the changes in this file.

Instead of changing allocateTarget to take a read time, you could also call allocateTarget followed by updateTargetData, removing all changes.

You are leaking the target right now since you never call release. That might be by design, but we should point this out somehow (and ideally make it more explicit in the code).

Applied first two.

Regarding 3, they will get collected aren't they? Also added comments to explain.

They will get collected in IndexedDB, but the local target mapping doesn't get cleared. this.targetDataByTarget and this.targetIdByTarget seems to stay around. I wonder if this makes it possible for LRU GC to delete from disk even though it is still considered active in LocalStore? Can you verify what the implications would be?

OK, so it looks like being in targetDataByTarget means it is active, unless user happen to unlisten the same query somehow.

This is not a bad thing I think, added comment to explain.

schmidt-sebastian · 2020-08-04T17:06:26Z

packages/firestore/test/unit/specs/spec_builder.ts

@@ -73,7 +73,7 @@ export interface LimboMap {

 export interface ActiveTargetSpec {
  queries: SpecQuery[];
-  resumeToken: string;
+  resumeToken: string | TestSnapshotVersion;


This should probably be split into two distinct fields, since TestSnapshotVersion is not a resumeToken.

schmidt-sebastian · 2020-08-04T17:07:20Z

packages/firestore/test/unit/specs/spec_builder.ts

@@ -253,7 +253,7 @@ export class SpecBuilder {
    return this;
  }

-  userListens(query: Query, resumeToken?: string): this {
+  userListens(query: Query, resumeFrom?: string | TestSnapshotVersion): this {


FWIW, the test above would read super cleanly if you made this

Suggested change

userListens(query: Query, resumeFrom?: string | TestSnapshotVersion): this {

userListens(query: Query, resumeFrom?: { resumeToken?: string; readTime?: TestSnapshotVersion}}): this {

schmidt-sebastian · 2020-08-04T17:07:59Z

packages/firestore/test/unit/specs/spec_builder.ts

@@ -1029,7 +1029,7 @@ export class SpecBuilder {
  private addQueryToActiveTargets(
    targetId: number,
    query: Query,
-    resumeToken?: string
+    resumeToken?: string | TestSnapshotVersion


Make this resumeFrom as well, since TestSnapshotVersion is not a resumeToken.

schmidt-sebastian · 2020-08-04T17:08:59Z

packages/firestore/test/unit/specs/spec_test_runner.ts

@@ -1360,8 +1367,10 @@ export interface SpecStep {
  expectedSnapshotsInSyncEvents?: number;
 }

-/** [<target-id>, <query-path>] */
-export type SpecUserListen = [TargetId, string | SpecQuery];
+export interface SpecUserListen {


HOLY SMOKES. THIS IS THE BEST THING I HAVE READ ALL YEAR.

Thanks for cleaning this up.

schmidt-sebastian · 2020-08-04T17:10:22Z

packages/firestore/test/unit/util/bundle_data.ts

  addNamedQuery(
    name: string,
    readTime: api.Timestamp,
-    bundledQuery: BundledQuery
+    query: Query,
+    limitType?: LimitType


Would it at all be possible to do the Query translation (flip the order) here? Not sure if feasible, but then we could drop the LimitType and reduce the confusion.

schmidt-sebastian · 2020-08-05T17:22:32Z

packages/firestore/src/remote/serializer.ts

@@ -975,6 +975,11 @@ export function toTarget(

  if (targetData.resumeToken.approximateByteSize() > 0) {
    result.resumeToken = toBytes(serializer, targetData.resumeToken);
+  } else if (targetData.snapshotVersion.compareTo(SnapshotVersion.min()) > 0) {


So I think the problem is that we can't tell which one is newer and we likely want the newer of the two. In stand up we discussed that the resumeToken is preferred, since it contains more information to help the backend resume at the right query position. I would think we could do something like this:

If readTime > snapshotVersion, use readTime

Otherwise use resumeToken

Does that work?

schmidt-sebastian · 2020-08-05T17:30:07Z

packages/firestore/test/unit/specs/bundle_spec.test.ts

+
+    return spec()
+      .loadBundle(bundleString1)
+      .userListens(query1, /* resumeFrom */ '', 400)


400 still raises questions here. If you use named optional arguments this will be super clean, I promise:

userListens(query1 ) userListens(query1, { resumeToken : 'abc' } ) userListens(query1, { readTime : 400 } )

Done, it is better!

schmidt-sebastian · 2020-08-05T18:23:44Z

packages/firestore/src/local/local_store.ts

+        fromVersion(query.readTime!),
+        transaction
+      );
+      return localStoreImpl.bundleCache.saveNamedQuery(transaction, query);


They will get collected in IndexedDB, but the local target mapping doesn't get cleared. this.targetDataByTarget and this.targetIdByTarget seems to stay around. I wonder if this makes it possible for LRU GC to delete from disk even though it is still considered active in LocalStore? Can you verify what the implications would be?

schmidt-sebastian

Pretty much LGTM, minus one style nit in regards to the spec tests and the discussion from standup.

Also better spec tests.

wu-hui added 30 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

temp checkin

e735e23

Implement without async/await

17ba434

Major code complete.

f808d8d

Integration tests added.

db1d864

Added spec tests.

979ffd9

Two TODO: 1. load from secondary tab. 2. don't fail async queue.

Add comments and move types to d.ts

f7ff495

Support loading string for real.

556a007

Makes sure SDK still works after loading bad bundles.

adf1504

Better spect test.

b364ab0

Load bundles from secondary clients should raise snapshots

2588f20

Support secondary to secondary.

93bc964

wu-hui added 3 commits July 28, 2020 13:44

Use legacy.LoadBundleTask

8baf8fc

Merge branch 'wuandy/ExpBundlesApi' into wuandy/BundleQueryResume

7abcf19

Address first round feedbacks

f228dfb

wu-hui changed the base branch from wuandy/ExpBundlesApi to wuandy/Bundles July 28, 2020 21:28

Merge branch 'wuandy/Bundles' into wuandy/BundleQueryResume

5a6e8ea

schmidt-sebastian reviewed Jul 28, 2020

View reviewed changes

wu-hui added 3 commits July 31, 2020 12:39

Save readtime to target cache and remove readtime from Query

70d350a

Save named queries to target cache as well.

8ea9e7b

Fix more errors.

487982c

wu-hui assigned schmidt-sebastian and unassigned schmidt-sebastian Jul 31, 2020

schmidt-sebastian reviewed Aug 4, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Aug 4, 2020

Address feedback

9f1e135

wu-hui assigned schmidt-sebastian and unassigned wu-hui Aug 5, 2020

Remove readTime from allocateTarget

5eb05d9

schmidt-sebastian reviewed Aug 5, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Aug 5, 2020

drop resume token when updating read time.

e057fc1

Also better spec tests.

wu-hui assigned schmidt-sebastian and unassigned wu-hui Aug 5, 2020

schmidt-sebastian approved these changes Aug 5, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Aug 5, 2020

wu-hui merged commit 5ebf8c3 into wuandy/Bundles Aug 6, 2020

firebase locked and limited conversation to collaborators Sep 6, 2020

	userListens(query: Query, resumeFrom?: string \| TestSnapshotVersion): this {
	userListens(query: Query, resumeFrom?: { resumeToken?: string; readTime?: TestSnapshotVersion}}): this {

Bundle's named query resume. #3395

Bundle's named query resume. #3395

Uh oh!

Conversation

wu-hui commented Jul 12, 2020

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!

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!

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!

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!