Initial base classes for index backfill #2950

thebrianchen · 2021-09-02T17:46:19Z

No description provided.

google-oss-bot · 2021-09-02T17:54:37Z

Coverage Report

Affected SDKs

`firebase-firestore`

SDK overall coverage changed from 41.43% (e41b233) to 41.48% (10c3770c) by +0.05%.

Filename	Base (`e41b233`)	Head (10c3770c)	Diff
AsyncQueue.java	76.88%	77.00%	+0.12%
ComponentProvider.java	100.00%	97.30%	-2.70%
FirestoreClient.java	29.60%	29.01%	-0.59%
IndexBackfiller.java	?	50.00%	?
IndexEntry.java	?	100.00%	?
LocalStore.java	97.15%	96.81%	-0.34%
SQLitePersistence.java	83.06%	83.24%	+0.18%
Scheduler.java	?	0.00%	?

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 (10c3770c) is created by Prow via merging commits: e41b233 db08413.

google-oss-bot · 2021-09-02T17:57:39Z

Binary Size Report

Affected SDKs

firebase-firestore

Type Base (e41b233) Head (10c3770c) Diff

aar 1.18 MB 1.19 MB +5.33 kB (+0.4%)

apk (release) 3.24 MB 3.24 MB +1.79 kB (+0.1%)

Test Logs

Notes

Head commit (10c3770c) is created by Prow via merging commits: e41b233 db08413.

schmidt-sebastian

This looks good, but it could be simplified by making it a bit less generic. We should also figure out how to run this optionally when the indexing flag is enabled.

schmidt-sebastian · 2021-09-02T22:14:51Z

firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java

@@ -143,6 +145,10 @@ public FirestoreClient(
          if (gcScheduler != null) {
            gcScheduler.stop();
          }
+
+          if (indexScheduler != null) {


Should this scheduler always run? We may also need to backfill indices that were configured after client startup.

In my mind, the scheduler should always run, but whether the scheduler schedules anything is another question. We can change this as necessary as we build more functionality out.

schmidt-sebastian · 2021-09-02T22:17:06Z

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

+  /** How long we wait to try running LRU GC after SDK initialization. */
+  private static final long INITIAL_GC_DELAY_MS = TimeUnit.MINUTES.toMillis(1);
+  /** Minimum amount of time between GC checks, after the first one. */
+  private static final long REGULAR_GC_DELAY_MS = TimeUnit.MINUTES.toMillis(5);


FWIW I think we should be much more aggressive.

Updated to 30 seconds and every minute. We can tune this number once we can run benchmarks.

schmidt-sebastian · 2021-09-02T22:18:13Z

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

+    }
+  }
+
+  public class Scheduler implements StartStopScheduler {


I am trying to figure out what functionality a StartStopSchedule offers over a Scheduler, but I am not convinced I will ever find out :)

Renamed the base class to Scheduler. To be honest, I'm not sure why the LruGarbageCollector's Scheduler used the abstract base class to begin with since it's not shared with any other class.

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

schmidt-sebastian · 2021-09-02T22:19:54Z

...ase-firestore/src/main/java/com/google/firebase/firestore/local/IndexBackfillerDelegate.java

+ * Persistence layers intending to perform index backfills should implement this interface. This
+ * interface defines the operations that the Index Backfiller needs from the persistence layer.
+ */
+public interface IndexBackfillerDelegate {


Do we need the indirection with the delegate?

Removed the delegate. Initially, I thought we would need to support MemoryPersistence in case the user disables persistence in their app, but I realize we can work around that.

schmidt-sebastian · 2021-09-02T22:21:20Z

...restore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexBackfillerDelegate.java

+
+  @Override
+  @Nullable
+  public IndexEntry getIndexEntry(int indexId) {


I don't think this belongs here. You should only be writing. We also need to figure out how to index partial document updates, but even for those I don't think we should have a public API.

I take this back if you want to use this for testing - but then this should be marked as such.

I'll leave this here for the base prototype, and add more features to it as we build it out.

schmidt-sebastian · 2021-09-02T22:22:51Z

...e-firestore/src/test/java/com/google/firebase/firestore/local/MemoryIndexBackfillerTest.java

+
+@RunWith(RobolectricTestRunner.class)
+@Config(manifest = Config.NONE)
+public class MemoryIndexBackfillerTest extends IndexBackfillerTestCase {


This test will not test anything, will it? We can probably just have a single SQLLite backfill test.

thebrianchen

thanks for the early stage review!

thebrianchen · 2021-09-07T20:37:35Z

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

+  /** How long we wait to try running LRU GC after SDK initialization. */
+  private static final long INITIAL_GC_DELAY_MS = TimeUnit.MINUTES.toMillis(1);
+  /** Minimum amount of time between GC checks, after the first one. */
+  private static final long REGULAR_GC_DELAY_MS = TimeUnit.MINUTES.toMillis(5);


Updated to 30 seconds and every minute. We can tune this number once we can run benchmarks.

thebrianchen · 2021-09-07T20:38:36Z

firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java

@@ -143,6 +145,10 @@ public FirestoreClient(
          if (gcScheduler != null) {
            gcScheduler.stop();
          }
+
+          if (indexScheduler != null) {


In my mind, the scheduler should always run, but whether the scheduler schedules anything is another question. We can change this as necessary as we build more functionality out.

thebrianchen · 2021-09-07T20:39:21Z

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

+    }
+  }
+
+  public class Scheduler implements StartStopScheduler {


Renamed the base class to Scheduler. To be honest, I'm not sure why the LruGarbageCollector's Scheduler used the abstract base class to begin with since it's not shared with any other class.

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

thebrianchen · 2021-09-07T20:40:42Z

...ase-firestore/src/main/java/com/google/firebase/firestore/local/IndexBackfillerDelegate.java

+ * Persistence layers intending to perform index backfills should implement this interface. This
+ * interface defines the operations that the Index Backfiller needs from the persistence layer.
+ */
+public interface IndexBackfillerDelegate {


Removed the delegate. Initially, I thought we would need to support MemoryPersistence in case the user disables persistence in their app, but I realize we can work around that.

thebrianchen · 2021-09-07T20:41:14Z

...restore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexBackfillerDelegate.java

+
+  @Override
+  @Nullable
+  public IndexEntry getIndexEntry(int indexId) {


I'll leave this here for the base prototype, and add more features to it as we build it out.

thebrianchen · 2021-09-07T20:41:19Z

...e-firestore/src/test/java/com/google/firebase/firestore/local/MemoryIndexBackfillerTest.java

+
+@RunWith(RobolectricTestRunner.class)
+@Config(manifest = Config.NONE)
+public class MemoryIndexBackfillerTest extends IndexBackfillerTestCase {


schmidt-sebastian · 2021-09-08T20:30:39Z

firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java

@@ -143,6 +145,10 @@ public FirestoreClient(
          if (gcScheduler != null) {
            gcScheduler.stop();
          }
+
+          if (indexScheduler != null && Persistence.INDEXING_SUPPORT_ENABLED) {


Do we need to check the flag here? The scheduler would not be available if the flag is off.

The name is also a bit generic. I wonder if we should include "backfill" in the scheduler name, which reduces the mental scope from "all of indexing" to "backfill index data".

Removed the flag and renamed to indexBackfillScheduler.

schmidt-sebastian · 2021-09-08T20:32:43Z

firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java

@@ -264,6 +271,10 @@ private void initialize(Context context, User user, FirebaseFirestoreSettings se
    if (gcScheduler != null) {
      gcScheduler.start();
    }
+
+    if (indexScheduler != null && Persistence.INDEXING_SUPPORT_ENABLED) {


I think we should guard the scheduler assignment based on this flag (line 265). Ideally, if indexing is off, none of the logic should even be loaded. It otherwise becomes hard to reason about the system - e.g. the constructor code of the scheduler needs to be aware that indexing might be on or off.

Guarded based on the flag and persistence being enabled.

schmidt-sebastian · 2021-09-08T20:34:32Z

firebase-firestore/src/main/java/com/google/firebase/firestore/index/IndexEntry.java

+
+/**
+ * Represents an index entry saved by the SDK in the local storage. Temporary placeholder, since
+ * we'll probably serialize the indexValue right away rather than store it.


Please add "TODO(indexing)".

Note that we already committed FieldIndex. In #2963, FieldIndex also gets an indexId.

schmidt-sebastian · 2021-09-08T20:35:19Z

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

+    return new BackfillScheduler(asyncQueue, localStore);
+  }
+
+  // TODO: Figure out which index entries to backfill.


TODO(indexing)

schmidt-sebastian · 2021-09-08T20:36:02Z

...e-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteIndexBackfillerTest.java

+  @Before
+  public void setUp() {
+    persistence = PersistenceTestHelpers.createSQLitePersistence();
+    ;


schmidt-sebastian · 2021-09-08T20:37:11Z

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

+    return new Results(/* hasRun= */ true, numIndexesWritten, numIndexesRemoved);
+  }
+
+  public void addIndexEntry(IndexEntry entry) {


These methods would probably also be private if it was not for your tests. They should also be marked @VisibleForTesting if that is true.

All of the test-only methods should also be package-private.

schmidt-sebastian · 2021-09-08T20:38:18Z

...e-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteIndexBackfillerTest.java

+  public void addAndRemoveIndexEntry() {
+    IndexEntry testEntry =
+        new IndexEntry(1, "TEST_BLOB".getBytes(), "sample-uid", "sample-documentId");
+    backfiller.addIndexEntry(testEntry);


addIndex and related methods should be scoped to a SQLite transaction and not be run without it.

Initial base classes for index backfill

2c4dded

thebrianchen self-assigned this Sep 2, 2021

google-cla bot added the cla: yes Override cla label Sep 2, 2021

google-oss-bot added the size/XL label Sep 2, 2021

add todo

90fcdc3

schmidt-sebastian reviewed Sep 2, 2021

View reviewed changes

thebrianchen commented Sep 7, 2021

View reviewed changes

resolve comments, simplify some abstractions

6ca237f

google-oss-bot added size/L and removed size/XL labels Sep 7, 2021

revert change

c660690

thebrianchen force-pushed the bc/index-scheduler branch from f595b7b to c660690 Compare September 7, 2021 20:53

Merge branch 'master' into bc/index-scheduler

b92e273

thebrianchen requested a review from schmidt-sebastian September 7, 2021 23:45

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Sep 8, 2021

schmidt-sebastian reviewed Sep 8, 2021

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Sep 8, 2021

resolve comments round 2

db08413

thebrianchen requested a review from schmidt-sebastian September 9, 2021 19:55

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Sep 9, 2021

schmidt-sebastian approved these changes Sep 9, 2021

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Sep 9, 2021

thebrianchen merged commit 8122d55 into master Sep 9, 2021

thebrianchen deleted the bc/index-scheduler branch September 9, 2021 21:00

firebase locked and limited conversation to collaborators Oct 10, 2021

Initial base classes for index backfill #2950

Initial base classes for index backfill #2950

Uh oh!

Conversation

thebrianchen commented Sep 2, 2021

Uh oh!

google-oss-bot commented Sep 2, 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 Sep 2, 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!

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!

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!

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!

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!

google-oss-bot commented Sep 2, 2021 •

edited

Loading

`firebase-firestore`

google-oss-bot commented Sep 2, 2021 •

edited

Loading

`firebase-firestore`