Backfill Mutations #3323

schmidt-sebastian · 2022-01-18T22:42:10Z

This is another version of #3202

It's a bit simpler:

It moves all IndexOffset logic to IndexBackfiller
It re-uses more code in the LocalDocumentsView
It removes the latest read time from the offset, as we can just keep the offset at the current read time

I also added a new LocalStoreTestCase that verifies our query processing.

google-oss-bot · 2022-01-18T22:54:55Z

Coverage Report ¹

Affected Products

`firebase-firestore`

Overall coverage changed from 45.31% (28c950c) to 45.39% (c0f7f9b) by +0.08%.

Filename	Base (`28c950c`)	Merge (c0f7f9b)	Diff
AutoValue_FieldIndex_IndexState.java	45.45%	63.64%	+18.18%
AutoValue_FieldIndex_Segment.java	33.33%	54.17%	+20.83%
IndexBackfiller.java	76.00%	77.92%	+1.92%
LocalDocumentsResult.java	?	100.00%	?
MemoryRemoteDocumentCache.java	98.31%	98.21%	-0.09%
PatchMutation.java	100.00%	98.39%	-1.61%
SetMutation.java	97.14%	94.29%	-2.86%
SQLiteDocumentOverlayCache.java	95.08%	96.72%	+1.64%
SQLiteRemoteDocumentCache.java	96.75%	96.64%	-0.11%
Util.java	65.00%	67.13%	+2.13%

Test Logs

Notes

Commit (c0f7f9b) is created by Prow via merging PR base commit (28c950c) and head commit (73d4808).
Run gradle <product>:checkCoverage to produce HTML coverage reports locally. After gradle commands finished, report files can be found under <product-build-dir>/reports/jacoco/.

https://storage.googleapis.com/firebase-sdk-metric-reports/b24m3DM3ci.html

google-oss-bot · 2022-01-18T22:55:12Z

Size Report ¹

Affected Products

firebase-firestore
Type Base (28c950c) Merge (c0f7f9b) Diff
aar 1.24 MB 1.24 MB +487 B (+0.0%)
apk (release) 3.33 MB 3.33 MB +284 B (+0.0%)

Test Logs

Notes

Commit (c0f7f9b) is created by Prow via merging PR base commit (28c950c) and head commit (73d4808).

https://storage.googleapis.com/firebase-sdk-metric-reports/iMrTEmUrEF.html

schmidt-sebastian · 2022-01-21T20:24:56Z

/test check-changed

thebrianchen

big brain 🤯

thebrianchen · 2022-01-21T17:28:12Z

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

-/** The result of a write to the local store. */
-public final class LocalWriteResult {
-  private final int batchId;
+/** The result of a applying local mutations. */


s/of a/of

Optionally, include how this class is now being used to handle backfill.

thebrianchen · 2022-01-21T22:08:05Z

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

-        }
+  /** Returns the lowest offset for the provided index group. */
+  private IndexOffset getExistingOffset(Collection<FieldIndex> fieldIndexes) {
+    if (fieldIndexes.isEmpty()) {


change to assertion b/c fieldIndexes should never be empty.

thebrianchen · 2022-01-21T22:16:35Z

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

@@ -281,4 +320,11 @@ void recalculateAndSaveOverlays(Set<DocumentKey> documentKeys) {

    return results;
  }
+
+  /** Returns a base document that can be used to apply `overlay`. */
+  private MutableDocument createBaseDocument(DocumentKey key, @Nullable Overlay overlay) {


consider renaming to getBaseDocument() since we're getting the base doc from remote doc cache, and then creating an invalid one if it's not found

Done (this was also the original name)

thebrianchen · 2022-01-21T22:17:20Z

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


-    IndexOffset newOffset = getNewOffset(documents, existingOffset);
+    IndexOffset newOffset = getNewOffset(existingOffset, nextBatch);


As discussed, please add some tests to verify offset correctness when a new fieldindex is added to a collection group that has already been partially indexed.

Similarly, can we add a test to check that we call computeViews() to properly fetch the base doc for a patch mutation in an overlay without a corresponding base doc?

Added a couple of tests to cover all these scenarios.

thebrianchen · 2022-01-21T22:21:28Z

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

+   * @param collectionGroup The collection group for the documents.
+   * @param offset The offset to index into.
+   * @param count The number of documents to return
+   * @return A LocalDocumentsRResult with the documents that follow the provided offset and the last


thebrianchen · 2022-01-21T22:22:39Z

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

@@ -281,4 +320,11 @@ void recalculateAndSaveOverlays(Set<DocumentKey> documentKeys) {

    return results;
  }
+
+  /** Returns a base document that can be used to apply `overlay`. */


optional reword: Returns the base document that corresponds to the provided overlay.

Skipped as "corresponds" doesn't sound correct when we return an invalid document for a set() even though there is actually a valid document in the cache.

thebrianchen · 2022-01-21T22:24:06Z

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

  }

-  public int getBatchId() {
-    return batchId;
+  public int getLargestBatchId() {


As discussed, the "largest" batch id here lacks the surrounding context that it's the largest batch id processed by the index backfiller for a collection group. Add comment for context or separate it out.

Renamed back to getBatchId().

thebrianchen · 2022-01-21T22:25:46Z

firebase-firestore/src/main/java/com/google/firebase/firestore/model/FieldIndex.java

@@ -141,7 +141,7 @@ public static IndexOffset create(
    /**
     * Creates an offset that matches all documents with a read time higher than {@code readTime}.
     */
-    public static IndexOffset create(SnapshotVersion readTime) {
+    public static IndexOffset create(SnapshotVersion readTime, int largestBatchId) {


optional: rename to avoid unintentional usage where caller expects offset to be the exact provided readtime.

some quick examples: createForNextReadTime/createSucessor?

createSucessor() it is.

thebrianchen · 2022-01-21T22:32:19Z

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

+        computeViews(docs, overlays, Collections.emptySet());
+    return new LocalDocumentsResult(largestBatchId, localDocs);
+  }
+


Trying to follow along for getDocumentsMatchingCollectionQuery().

If we get the overlays in L298, don't we want to run createBaseDocument() to get the original remote base doc for the overlay rather than just use a newInvalidDocument()?

thebrianchen · 2022-01-21T22:36:16Z

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

+  }
+
+  @Test
+  public void testBackfillUpdatesToLatestReadTimeInCollection() {


This test is kind of useless now since we're already indexing a document.

Removed. We have a similar test already (testBackfillWritesLatestReadTimeToFieldIndexOnCompletion)

google-oss-bot · 2022-01-24T20:07:24Z

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

Test name	Commit	Details	Rerun command
device-check-changed	`73d4808`	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.

google-oss-bot added the size/XL label Jan 18, 2022

This was referenced Jan 19, 2022

Add Overlay class #3329

Merged

Add getOverlays(CollectionGroup) #3330

Merged

schmidt-sebastian force-pushed the mrschmidt/simplifymutationidhandling branch from d25f862 to bba4249 Compare January 20, 2022 03:11

Backfill Mutations

85eba03

schmidt-sebastian force-pushed the mrschmidt/simplifymutationidhandling branch from 49939d4 to 85eba03 Compare January 20, 2022 21:05

schmidt-sebastian changed the title ~~Simplified Mutation Batch ID Handling~~ Backfill Mutations Jan 20, 2022

schmidt-sebastian requested a review from thebrianchen January 20, 2022 21:09

schmidt-sebastian assigned thebrianchen Jan 20, 2022

schmidt-sebastian added the api: firestore label Jan 20, 2022

schmidt-sebastian added 3 commits January 20, 2022 14:13

Rename

dc48178

Formatting

eecef54

Fix Lint

4008cd5

schmidt-sebastian added 2 commits January 21, 2022 14:20

Typo

a7a18f6

Typo

f19cd92

thebrianchen approved these changes Jan 21, 2022

View reviewed changes

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Jan 21, 2022

thebrianchen mentioned this pull request Jan 24, 2022

Add mutation batch id to index backfill #3202

Closed

schmidt-sebastian added 2 commits January 24, 2022 11:07

Review

52f36c3

Use Lambda~

73d4808

schmidt-sebastian merged commit 2715647 into master Jan 24, 2022

schmidt-sebastian deleted the mrschmidt/simplifymutationidhandling branch January 24, 2022 19:52

firebase locked and limited conversation to collaborators Feb 24, 2022


		IndexOffset newOffset = getNewOffset(documents, existingOffset);
		IndexOffset newOffset = getNewOffset(existingOffset, nextBatch);

Backfill Mutations #3323

Backfill Mutations #3323

Uh oh!

Conversation

schmidt-sebastian commented Jan 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

google-oss-bot commented Jan 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report 1

Affected Products

firebase-firestore

Test Logs

Notes

Uh oh!

google-oss-bot commented Jan 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Size Report 1

Affected Products

firebase-firestore

Test Logs

Notes

Uh oh!

schmidt-sebastian commented Jan 21, 2022

Uh oh!

thebrianchen 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!

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!

google-oss-bot commented Jan 24, 2022

Uh oh!

Uh oh!

schmidt-sebastian commented Jan 18, 2022 •

edited

Loading

google-oss-bot commented Jan 18, 2022 •

edited

Loading

Coverage Report ¹

`firebase-firestore`

google-oss-bot commented Jan 18, 2022 •

edited

Loading

Size Report ¹

`firebase-firestore`