Release Overlay #3112

wu-hui · 2021-11-08T18:49:06Z

Performance test result: #3161

google-oss-bot · 2021-11-08T18:58:04Z

Coverage Report

Affected SDKs

`firebase-firestore`

SDK overall coverage changed from 45.32% (9466242) to 45.17% (462500e4) by -0.16%.

Filename	Base (`9466242`)	Head (462500e4)	Diff
BasePath.java	98.08%	86.54%	-11.54%
DeleteMutation.java	90.00%	95.00%	+5.00%
FieldMask.java	55.56%	57.89%	+2.34%
LocalDocumentsView.java	97.70%	69.14%	-28.56%
PatchMutation.java	98.39%	100.00%	+1.61%
SQLiteOverlayMigrationManager.java	61.90%	90.48%	+28.57%
SQLitePersistence.java	84.57%	86.70%	+2.13%
SQLiteSchema.java	96.65%	96.63%	-0.03%
SetMutation.java	94.29%	97.14%	+2.86%

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 (462500e4) is created by Prow via merging commits: 9466242 dff4bbd.

google-oss-bot · 2021-11-08T19:00:19Z

Binary Size Report

Affected SDKs

firebase-firestore

Type Base (9466242) Head (462500e4) Diff

aar 1.23 MB 1.23 MB +97 B (+0.0%)

apk (release) 3.33 MB 3.33 MB +32 B (+0.0%)

Test Logs

Notes

Head commit (462500e4) is created by Prow via merging commits: 9466242 dff4bbd.

schmidt-sebastian

Woohoo!

Please verify that the schema upgrades works as intended using an actual app. Thanks!

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

wu-hui · 2021-11-09T16:02:00Z

@schmidt-sebastian Please take another look!

Without the new commit, upgrade will actually crash, because data_migrations table is not created. Thanks for reminding me to check with an actual App.

schmidt-sebastian · 2021-11-09T17:16:02Z

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

-                : Collections.emptyList());
+    List<MutationResult> mutationResults =
+        transformResult == null
+            ? singletonList(new MutationResult(version, emptyList()))


We usually refer to these method using the class name: Arrays.singletonList.

schmidt-sebastian · 2021-11-09T17:16:41Z

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

+            ? singletonList(new MutationResult(version, emptyList()))
+            : Arrays.stream(transformResult)
+                .map(o -> new MutationResult(version, Collections.singletonList(TestUtil.wrap(o))))
+                .collect(Collectors.toList());


IMHO this entire statement is not very readable.

schmidt-sebastian · 2021-11-09T17:17:53Z

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

@@ -963,9 +964,8 @@ public void testReadsAllDocumentsForInitialCollectionQueries() {
    localStore.executeQuery(query, /* usePreviousResults= */ true);

    assertRemoteDocumentsRead(/* byKey= */ 0, /* byQuery= */ 2);
-    if (!Persistence.OVERLAY_SUPPORT_ENABLED) {


Not a blocker, but we should ideally remove all statements like this (including the flag) or leave them all here. If we leave the flag, we should be able to turn it off during development and have the tests pass.

schmidt-sebastian · 2021-11-09T17:18:59Z

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

  }

  void runSchemaUpgrades(int fromVersion) {
    int toVersion = VERSION;
-    if (Persistence.OVERLAY_SUPPORT_ENABLED) {


We should remove the flag in its entirety to just set it to true but leave all conditional checks. This makes sure that we can turn it to false again.

This one here has to be deleted, because if the version is increased again, doing this will override it.

# Conflicts: # firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java

schmidt-sebastian · 2021-11-23T22:10:19Z

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

@@ -203,7 +203,9 @@ private void recalculateAndSaveOverlays(Map<DocumentKey, MutableDocument> docs)
    // along the way.
    for (MutationBatch batch : batches) {
      for (DocumentKey key : batch.getKeys()) {
-        FieldMask mask = batch.applyToLocalView(docs.get(key), masks.get(key));
+        FieldMask mask =
+            masks.containsKey(key) ? masks.get(key) : FieldMask.fromSet(new HashSet<>());


Can we create an EMPTY_MASK sentinel? The missing mask is the default case and we create two objects each time.

schmidt-sebastian · 2021-11-23T22:11:07Z

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

@@ -152,23 +151,25 @@ private void udpateViews(int targetId, boolean fromCache) {
    notifyLocalViewChanges(viewChanges(targetId, fromCache, asList(), asList()));
  }

-  private void acknowledgeMutation(long documentVersion, @Nullable Object transformResult) {
+  private void acknowledgeMutationWithTransform(long documentVersion, Object... transformResult) {


acknowledgeMutationWithTransformResults ? (or leave as before)

schmidt-sebastian · 2021-11-23T22:11:46Z

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

-                : Collections.emptyList());
+    List<MutationResult> mutationResults =
+        Collections.singletonList(new MutationResult(version, emptyList()));
+    if (transformResult != null) {


This array should never be null. You can also use Java8 streams to simplify this logic.

It is null when acknowledgeMutation is called.

You should call it with no arguments instead of null.

wu-hui · 2021-11-24T15:34:59Z

/test check-changed

wu-hui · 2021-11-25T15:58:45Z

/retest

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

wu-hui · 2021-11-30T21:29:37Z

/retest

# Conflicts: # firebase-firestore/CHANGELOG.md # firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java

Release Overlay

39771c3

wu-hui requested a review from schmidt-sebastian November 8, 2021 18:49

google-cla bot added the cla: yes Override cla label Nov 8, 2021

wu-hui assigned schmidt-sebastian Nov 8, 2021

google-oss-bot added the size/L label Nov 8, 2021

schmidt-sebastian approved these changes Nov 8, 2021

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Nov 8, 2021

mikehardy reviewed Nov 9, 2021

View reviewed changes

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

Schema upgrade done right.

016221d

wu-hui assigned schmidt-sebastian and unassigned wu-hui Nov 9, 2021

wu-hui unassigned schmidt-sebastian Nov 9, 2021

wu-hui added 2 commits November 9, 2021 11:27

Insert or Ignore.

93ba4a7

Pass empty mask, not null when recalculation.

f9d4208

wu-hui assigned schmidt-sebastian Nov 9, 2021

schmidt-sebastian reviewed Nov 9, 2021

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Nov 9, 2021

Feedback

b7d6597

wu-hui assigned schmidt-sebastian and unassigned wu-hui Nov 23, 2021

Merge branch 'master' into wuandy/OverlayRelease

4c53217

# Conflicts: # firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java

schmidt-sebastian reviewed Nov 23, 2021

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Nov 23, 2021

More feedback.

c145203

wu-hui assigned schmidt-sebastian and unassigned wu-hui Nov 24, 2021

wu-hui added 2 commits November 30, 2021 10:35

Merge branch 'master' into wuandy/OverlayRelease

9f8c61e

Feedback.

3aa0f7f

schmidt-sebastian reviewed Nov 30, 2021

View reviewed changes

firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java Outdated Show resolved Hide resolved

wu-hui force-pushed the wuandy/OverlayRelease branch from 0823cfc to 3aa0f7f Compare November 30, 2021 19:56

Functional style

1649407

schmidt-sebastian approved these changes Nov 30, 2021

View reviewed changes

wu-hui added 3 commits December 6, 2021 14:40

Merge branch 'master' into wuandy/OverlayRelease

4e76e7a

# Conflicts: # firebase-firestore/CHANGELOG.md # firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java

merge to resolve conflict

a8e393c

Fix test error

dff4bbd

wu-hui merged commit 7c9c133 into master Dec 6, 2021

wu-hui deleted the wuandy/OverlayRelease branch December 6, 2021 21:04

firebase locked and limited conversation to collaborators Jan 6, 2022

Release Overlay #3112

Release Overlay #3112

Uh oh!

Conversation

wu-hui commented Nov 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

google-oss-bot commented Nov 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report

Affected SDKs

firebase-firestore

Test Logs

Notes

Uh oh!

google-oss-bot commented Nov 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Binary Size Report

Affected SDKs

firebase-firestore

Test Logs

Notes

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wu-hui commented Nov 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

wu-hui commented Nov 24, 2021

Uh oh!

wu-hui commented Nov 25, 2021

Uh oh!

Uh oh!

wu-hui commented Nov 30, 2021

Uh oh!

Uh oh!

wu-hui commented Nov 8, 2021 •

edited

Loading

google-oss-bot commented Nov 8, 2021 •

edited

Loading

`firebase-firestore`

google-oss-bot commented Nov 8, 2021 •

edited

Loading

`firebase-firestore`

wu-hui commented Nov 9, 2021 •

edited

Loading