Store per-user state for indexes #3153

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Merged

schmidt-sebastian merged 7 commits into master from mrschmidt/collectiongroupupdatetime

Nov 23, 2021

Contributor

schmidt-sebastian commented Nov 19, 2021

This PR moves the "how updated is the index?" state from the index table to a per-user table and adds a sequence number. This allows the backfiller to process all index entries independently for each user.

google-cla bot added the cla: yes label

google-oss-bot added the size/XL label


          Store per-user state for indexes

851de4f

schmidt-sebastian force-pushed the mrschmidt/collectiongroupupdatetime branch from d69fbd5 to 851de4f Compare

November 19, 2021 18:07

schmidt-sebastian requested a review from thebrianchen

November 19, 2021 18:08

schmidt-sebastian assigned thebrianchen

schmidt-sebastian added the api: firestore label

Contributor

google-oss-bot commented Nov 19, 2021 •

edited

Loading

Coverage Report

Affected SDKs

`firebase-firestore`

SDK overall coverage changed from 44.99% (a8f8dd9) to 45.09% (fef3bbbe) by +0.10%.

Click to show coverage changes in 12 files.

Filename	Base (`a8f8dd9`)	Head (fef3bbbe)	Diff
AutoValue_FieldIndex.java	55.56%	58.33%	+2.78%
AutoValue_FieldIndex_IndexState.java	?	45.45%	?
DeleteMutation.java	90.00%	95.00%	+5.00%
FirebaseFirestore.java	37.71%	37.50%	-0.21%
Index.java	25.92%	26.41%	+0.49%
IndexBackfiller.java	75.31%	77.78%	+2.47%
LocalSerializer.java	90.73%	92.72%	+1.99%
MemoryIndexManager.java	66.67%	64.29%	-2.38%
PatchMutation.java	98.39%	100.00%	+1.61%
SQLiteIndexManager.java	95.71%	99.33%	+3.62%
SQLiteRemoteDocumentCache.java	95.65%	95.83%	+0.18%
SQLiteSchema.java	96.31%	96.27%	-0.05%

Test Logs

Notes

HTML coverage reports can be produced locally with ./gradlew <product>:checkCoverage.
Report files are located at <product-build-dir>/reports/jacoco/.

Head commit (fef3bbbe) is created by Prow via merging commits: a8f8dd9 8152115.

Contributor

google-oss-bot commented Nov 19, 2021 •

edited

Loading

Binary Size Report

Affected SDKs

firebase-firestore

Type Base (a8f8dd9) Head (fef3bbbe) Diff

aar 1.22 MB 1.23 MB +2.71 kB (+0.2%)

apk (release) 3.32 MB 3.32 MB +352 B (+0.0%)

Test Logs

Notes

Head commit (fef3bbbe) is created by Prow via merging commits: a8f8dd9 8152115.


          Simplify by Brian

a5eb6e4

thebrianchen suggested changes

View reviewed changes

thebrianchen left a comment

First pass. Haven't looked at the tests yet, but had some initial feedback for the implementation + some questions.

firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java Outdated

@@ @@ -340,7 +339,8 @@ public FirebaseApp getApp() { @@
                           }
                         }
-                        parsedIndices.add(FieldIndex.create(-1, collectionGroup, segments, SnapshotVersion.NONE));
+                        parsedIndices.add(
+                            FieldIndex.create(-1, collectionGroup, segments, FieldIndex.IndexState.DEFAULT));

thebrianchen Nov 19, 2021

nit: can we use a static global var for -1?

Contributor Author

schmidt-sebastian Nov 22, 2021

Done.

firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexBackfiller.java

@@ @@ -131,71 +135,87 @@ public Results backfill() { @@
                   return persistence.runTransaction(
                       "Backfill Indexes",
                       () -> {
-                        // TODO(indexing): Handle field indexes that are removed by the user.

thebrianchen Nov 19, 2021

Did you mean to remove this TODO? It seems like we haven't handled deleting field indexes yet.

Contributor Author

schmidt-sebastian Nov 22, 2021

It was handled in another PR: #3118

firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexBackfiller.java Show resolved Hide resolved

firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexBackfiller.java Outdated

                   return oldestDocuments.size();
                 }
+                /** Returns the new read time for the index. */

thebrianchen Nov 19, 2021

nit: add comment that documents input needs to be sorted by read time.

Contributor Author

schmidt-sebastian Nov 22, 2021

Done

firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexManager.java Show resolved Hide resolved

firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java Outdated

                       () -> {
-                        // TODO(indexing): Do we need to store a different update time per user? We need to ensure
-                        // that we index mutations entries for all users.
+                        // Global configuration for all existing indices

thebrianchen Nov 19, 2021

s/indices/field indexes

Contributor Author

schmidt-sebastian Nov 22, 2021

Done

firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexManager.java Show resolved Hide resolved

firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexManager.java Show resolved Hide resolved

firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexManager.java Outdated

+                              + "FROM index_state WHERE uid = ?")
+                      .forEach(
+                          row -> {
+                            int indexId = row.getInt(1);

thebrianchen Nov 19, 2021

row.getInt(0) maps to indexid

Contributor Author

schmidt-sebastian Nov 22, 2021

Thanks for catching. Fixed and tested.

firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexManager.java Show resolved Hide resolved

schmidt-sebastian added 3 commits

November 22, 2021 09:39

WIP

0d45f4c


          Merge

40b8f01


          Review

ea0e636

schmidt-sebastian assigned thebrianchen and unassigned thebrianchen

thebrianchen approved these changes

View reviewed changes

thebrianchen left a comment

lgtm, thanks for explaining some of the questions i had

firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexManager.java Outdated

+                    db.execute(
+                        "REPLACE INTO index_state (index_id, uid,  sequence_number, "
+                            + "read_time_seconds, read_time_nanos) VALUES(?, ?, ?, ?, ?)",
+                        collectionGroup,

thebrianchen Nov 22, 2021

Should we be passing in updatedIndex.getIndexId() instead of collectionGroup here? If so, please add test.

Contributor Author

schmidt-sebastian Nov 23, 2021

Holy !$#1! So there was a test for this that should have caught this - the testChangeUser(). The only problem is that it called updateCollectionGroup with the first index. When we then call start() we call row.getInt(). The value is actually coll1, but getInt() returns 0, which happens to be the right value. I changed the test to modify the second index instead.

Thanks so much for spotting!

firebase-firestore/src/test/java/com/google/firebase/firestore/local/IndexBackfillerTest.java

-                  assertEquals("coll3", collectionGroups.get(0));
-                  assertEquals("coll1", collectionGroups.get(1));
-                  assertEquals("coll2", collectionGroups.get(2));
+                  collectionGroup = indexManager.getNextCollectionGroupToUpdate();

thebrianchen Nov 22, 2021

nit: remove outdated comment

Contributor Author

schmidt-sebastian Nov 22, 2021

Done

firebase-firestore/src/test/java/com/google/firebase/firestore/local/IndexBackfillerTest.java Outdated

+                  FieldIndex fieldIndex =
+                      fieldIndex(
+                          collectionGroup,
+                          -1,

thebrianchen Nov 22, 2021

nit: use FieldIndex const values here and below

Contributor Author

schmidt-sebastian Nov 22, 2021

Done

...base-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteIndexManagerTest.java

-                  assertEquals(2, collectionGroups.size());
-                  assertEquals("coll1", collectionGroups.get(0));
-                  assertEquals("coll2", collectionGroups.get(1));
+                  indexManager.addFieldIndex(

thebrianchen Nov 22, 2021

nit: update test name to reflect this test

Contributor Author

schmidt-sebastian Nov 22, 2021

Done

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen

schmidt-sebastian added 2 commits

November 23, 2021 09:31


          Review

2313e0f


          Test fix

Contributor Author

schmidt-sebastian commented Nov 23, 2021

/test device-check-changed

Contributor

google-oss-bot commented Nov 23, 2021

@schmidt-sebastian: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
device-check-changed	`8152115`	link	`/test device-check-changed`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

schmidt-sebastian merged commit d8fedfe into master

schmidt-sebastian deleted the mrschmidt/collectiongroupupdatetime branch

November 23, 2021 18:03

thebrianchen mentioned this pull request

Use sequence numbers instead of read time in collection group #3111

Closed

firebase locked and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

api: firestore cla: yes size/XL