awaitPendingWrites initial revision #689

ghost · 2019-08-06T14:55:43Z

Added 'waitForPendingWrites' to firestore.

schmidt-sebastian

I think this should work as is - I just have a bunch of comments on styling and comment. The concept is sound though! Thank you.

schmidt-sebastian · 2019-08-06T18:46:48Z

firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java

+    assertTrue(!pendingWrite.isComplete());
+    assertTrue(!awaitsPendingWrites2.isComplete());
+
+    waitFor(firestore.enableNetwork());


Technically, you don't need to wait for enableNetwork() or the pending write.

schmidt-sebastian · 2019-08-06T18:50:26Z

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

+   */
+  public Task<Void> awaitPendingWrites() {
+    this.verifyNotShutdown();
+    if (!remoteStore.canUseNetwork()) {


This should run on the async queue:

It accesses internal state.

enableNetwork() and disableNetwork() run on the async queue, so for this warning to show up these operations would need to resolve before the user calls awaitPendingWrites.

Thanks for the catch.

schmidt-sebastian · 2019-08-06T18:51:19Z

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

+    if (!remoteStore.canUseNetwork()) {
+      Logger.warn(
+          LOG_TAG,
+          "Network is disabled, the Task created to wait for all writes getting"


I think we can simplify this:

"The network is disabled. The task returned by awaitPendingWrites() will not complete until the network is enabled."

schmidt-sebastian · 2019-08-06T18:53:53Z

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

+  public Task<Void> awaitPendingWrites() {
+    this.verifyNotShutdown();
+    if (!remoteStore.canUseNetwork()) {
+      Logger.warn(


I am a little torn on whether this is a "warning" - it is certainly not a problem in the SDK itself, and technically not even for users of the SDK. It prevents unexpected behavior - so I would advocate to lower the log level here.

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

schmidt-sebastian · 2019-08-06T20:01:59Z

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

@@ -187,6 +187,11 @@ public MutationBatch getNextMutationBatchAfterBatchId(int batchId) {
    return queue.size() > index ? queue.get(index) : null;
  }

+  @Override
+  public int getLargestUnacknowledgedBatchId() {
+    return queue.size() == 0 ? 0 : nextBatchId - 1;


Google style guide generally recommends using isEmpty(), since it always runs in O(1).

schmidt-sebastian · 2019-08-06T20:03:35Z

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

@@ -249,6 +249,16 @@ public MutationBatch getNextMutationBatchAfterBatchId(int batchId) {
        .firstValue(row -> decodeInlineMutationBatch(row.getInt(0), row.getBlob(1)));
  }

+  @Override
+  public int getLargestUnacknowledgedBatchId() {
+    if (isEmpty()) {


Nit: In case the mutation queue is not empty, you are running a very similar query twice.

schmidt-sebastian · 2019-08-06T20:04:20Z

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

+    if (isEmpty()) {
+      return 0;
+    }
+    return db.query("SELECT MAX(batch_id) FROM mutations " + "WHERE uid = ?")


Nit: s/" + "//

schmidt-sebastian · 2019-08-06T20:05:30Z

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

@@ -1143,4 +1143,21 @@ public void testHandlesPatchMutationWithTransformThenRemoteEvent() {
    assertChanged(doc("foo/bar", 1, map("sum", 1), Document.DocumentState.LOCAL_MUTATIONS));
    assertContains(doc("foo/bar", 1, map("sum", 1), Document.DocumentState.LOCAL_MUTATIONS));
  }
+
+  @Test
+  public void testGetHighestUnacknowledgedBatchIdReturnsExpectedResult() {


Nit: Maybe remove ReturnsExpectedResult, since it doesn't add too much to the descriptiveness.

schmidt-sebastian · 2019-08-06T20:08:43Z

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

+   *
+   * <p>Both acceptance and rejection count as server acknowledgement.
+   *
+   * @return A {@link Task} which resolves when all pending writes are acknowledged by the server.


We likely want to update the comment a bit:

We should use concepts that are more widely known ("server acknowledgement for all pending writes" -> "when all writes have been sent to the backend").

We should mention how this behaves during a user change.

We should mention that this works even after client restart.

If you want to, we can tweak this comment in the chatroom.

Yeah. I made some change, but not enough. Especially i don't understand your comment about client restart, and how this still works with a restart. Let's chat tmr.

One of the reasons that this is a heavily requested feature is that it offers users the ability to monitor the state of writes that were issued during an earlier app session.If a user issues a write and then closes the app before the write is committed, it is not very straightforward to figure out whether the write made it to the backend once the app is opened again. drain/awaitPendingWrites solves this problem nicely.

Just for reference: This can be done today by either issuing a dummy write and waiting for its completion (one would have to know that we commit writes sequentially) or by issuing gets for all documents that were written and checking the hasPendingWrites flags.

schmidt-sebastian · 2019-08-07T16:06:33Z

firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java

+    Task<Void> pendingWrite = documentReference.set(data);
+    Task<Void> awaitsPendingWrites2 = firestore.waitForPendingWrites();
+
+    // `awaitsPendingWrites1` is complete immediately because there is no pending writes at


s/is complete/completes/
s/there is/there are/

schmidt-sebastian · 2019-08-07T16:15:38Z

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

+    if (!remoteStore.canUseNetwork()) {
+      Logger.debug(
+          TAG,
+          "The network is disabled. The task returned by 'awaitPendingWrites()' will not complete until the network is enabled.");


Nit: Please manually wrap to 100 characters.

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

schmidt-sebastian · 2019-08-07T16:16:53Z

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

+    }
+
+    if (pendingWritesCallbacks.containsKey(largestPendingBatchId)) {
+      pendingWritesCallbacks.get(largestPendingBatchId).add(userTask);


It's fine to stick with the multi-map.

schmidt-sebastian · 2019-08-07T16:18:54Z

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

@@ -282,6 +282,14 @@ public LocalWriteResult writeLocally(List<Mutation> mutations) {
        });
  }

+  /**
+   * Returns the largest (latest) batch id in mutation queue that is pending server response.
+   * Returns 0 if the queue is empty.


You are checking for this constant in registerPendingWritesTask().

If you use -1 (or rather the existing concept of MutationBatch.UNKNOWN) it is apparent that this is a special sentinel value. -1 indicates right away that it is not a valid batch ID.

0, on the other hand, could very well be a valid batch ID. It is not obvious that we decided (or rather that Proto3 decided for us) that the first valid batch ID should be 1.

schmidt-sebastian · 2019-08-07T17:07:34Z

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

@@ -565,6 +626,7 @@ public void handleCredentialChange(User user) {
      // Notify local store and emit any resulting events from swapping out the mutation queue.
      ImmutableSortedMap<DocumentKey, MaybeDocument> changes = localStore.handleUserChange(user);
      emitNewSnapsAndNotifyLocalStore(changes, /*remoteEvent=*/ null);
+      failOutstandingPendingWritesAwaitingTasks();


Ideally, we would want to fail these Tasks before we raise the new snapshots for the newly signed in user. That way, we handle the callbacks for the old user before invoking any callbacks that affect the new user.

schmidt-sebastian · 2019-08-07T17:12:31Z

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

+   *
+   * <p>Both acceptance and rejection count as server acknowledgement.
+   *
+   * @return A {@link Task} which resolves when all pending writes are acknowledged by the server.


One of the reasons that this is a heavily requested feature is that it offers users the ability to monitor the state of writes that were issued during an earlier app session.If a user issues a write and then closes the app before the write is committed, it is not very straightforward to figure out whether the write made it to the backend once the app is opened again. drain/awaitPendingWrites solves this problem nicely.

Just for reference: This can be done today by either issuing a dummy write and waiting for its completion (one would have to know that we commit writes sequentially) or by issuing gets for all documents that were written and checking the hasPendingWrites flags.

schmidt-sebastian

One comment about the order of callbacks and one final nudge to use MutationBatch.UNKNOWN to indicate that there is nothing to wait for.

wilhuff

This isn't a full review just the public comments.

wilhuff · 2019-08-07T20:09:54Z

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

@@ -362,6 +362,16 @@ public WriteBatch batch() {
    return shutdownInternal();
  }

+  /**
+   * Wait until all pending writes existed at the time of calling are sent to the backend.


A bunch of comments on this:

Phrase comments with an implicit "this method" at the beginning

missing "that" between "writes existed"

This doesn't just wait until writes are sent, but also waits for them to complete

[This method ...] Waits until all pending writes that existed at the time of calling are have been successfully written to the server. The task will will not resolve while the client is offline.

wilhuff · 2019-08-07T20:11:26Z

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

+  /**
+   * Wait until all pending writes existed at the time of calling are sent to the backend.
+   *
+   * @return A {@link Task} which resolves when all pending writes are sent to the backend. If there


Don't @link items that are already in the signature of the method. Use @link for anything related that's not part of the signature.

Use {@code Task} to highlight that it's a type without the extra visual noise that comes from an actual link.

schmidt-sebastian · 2019-08-07T20:31:47Z

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

+   *     there is a Firebase user change, the returned {@code Task} will resolve to an exception. If
+   *     the client is offline, the returned {@code Task} will not resolve.
+   */
+  Task<Void> waitForPendingWrites() {


I still think we should move this comment to a doc (it could be the API proposal doc) and do some collaborative editing:

pending writes that existed at the time of calling could be clearer.

written to the server is an unusual way of phrasing the right thing.

I think we should move the comment on the user change to a separate paragraph and move it out of the @return section

Tasks on Android don't resolve, they complete.

@mikelehen usually has a ton of good feedback on these comments, and his feedback tends to be much more valuable than what I can provide.

Thanks!

The API proposal doc would be a good place to collaborate on this.

Note that the "written to the server" language is used in the iOS documentation already which is why I suggested it: https://github.com/firebase/firebase-ios-sdk/blob/master/Firestore/Source/Public/FIRDocumentReference.h#L117. I'm definitely open to finding a better way to succinctly describe this.

Using Sebastian's version now, looks great.

ghost · 2019-08-07T21:10:48Z

Question: if there are no pending writes, will the task resolve while offline?

It will resolve in that case. That sounds right to me.

schmidt-sebastian

Looks great! Thank you.

schmidt-sebastian · 2019-08-08T03:43:54Z

...restore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java

+  }
+
+  private MockCredentialsProvider() {
+    super();


Nit: This super() call is not needed, is it?

schmidt-sebastian · 2019-08-08T03:44:22Z

...restore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java

+    listener.onValue(user);
+  }
+
+  private Listener<User> listener;


Nit: Move to top of class.

schmidt-sebastian · 2019-08-08T03:45:11Z

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

+   * <p>The returned Task completes immediately if there are no outstanding writes. Otherwise, the
+   * Task waits for all previously issued writes (including those written in a previous app
+   * session), but it does not wait for writes that were added after the method is called. If you
+   * wish to wait for additional writes, you have to call `waitForPendingWrites()` again.


Use {@code} for all mentions of waitForPendingWrites.

schmidt-sebastian · 2019-08-08T03:46:33Z

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

+    if (!remoteStore.canUseNetwork()) {
+      Logger.debug(
+          TAG,
+          "The network is disabled. The task returned by 'awaitPendingWrites()' will not"


Super nit: We usually leave the space at the end of the line. This helps to align comments with more than two lines.

schmidt-sebastian · 2019-08-08T03:46:56Z

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

+    }
+  }
+
+  /** Resolves tasks waiting for this batch id to get acknowledged by server, if there is any. */


schmidt-sebastian · 2019-08-08T03:47:44Z

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

+    assertEquals(2, localStore.getHighestUnacknowledgedBatchId());
+
+    rejectMutation();
+    assertEquals(-1, localStore.getHighestUnacknowledgedBatchId());


s/-1/MutationBatch.UNKNOWN/

awaitPendingWrites initial revision

74603b8

ghost added the api: firestore label Aug 6, 2019

googlebot added the cla: yes Override cla label Aug 6, 2019

google-oss-bot added the size/L label Aug 6, 2019

ghost requested a review from schmidt-sebastian August 6, 2019 15:20

schmidt-sebastian self-assigned this Aug 6, 2019

Add test to check task failure upon user change.

9d8841e

schmidt-sebastian reviewed Aug 6, 2019

View reviewed changes

schmidt-sebastian assigned ghost and unassigned schmidt-sebastian Aug 6, 2019

address comments #1

a683e2f

schmidt-sebastian reviewed Aug 7, 2019

View reviewed changes

schmidt-sebastian suggested changes Aug 7, 2019

View reviewed changes

addressing comments #2

86e3abf

ghost assigned schmidt-sebastian and unassigned ghost Aug 7, 2019

wilhuff reviewed Aug 7, 2019

View reviewed changes

addressing comments #3

67fec18

schmidt-sebastian reviewed Aug 7, 2019

View reviewed changes

addressing comments #4: better public comment.

ec2d7d2

schmidt-sebastian approved these changes Aug 8, 2019

View reviewed changes

schmidt-sebastian assigned ghost and unassigned schmidt-sebastian Aug 8, 2019

fixing more nits

a4e4355

ghost merged commit f096bbe into master Aug 8, 2019

rlazo deleted the wuandy/AwaitPendingWrites branch September 27, 2019 14:55

firebase locked and limited conversation to collaborators Oct 8, 2019

This pull request was closed.

awaitPendingWrites initial revision #689

awaitPendingWrites initial revision #689

Uh oh!

Conversation

ghost commented Aug 6, 2019 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

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!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

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

ghost commented Aug 6, 2019 •

edited by ghost

Loading

ghost commented Aug 7, 2019 •

edited by ghost

Loading