feat: add support for optimistic transactions #1548

schmidt-sebastian · 2021-06-29T21:44:28Z

This PR adds support for optimistic transactions. A verify write is added to the commit for all documents read during an optimistic transaction.

This PR relies on an backend feature that is not yet public. We need to figure out what our strategy here will be.

The first commit just re-arranges tests. It is probably best to only review the second commit.

Fixes #1089

thebrianchen

thanks for taking the time to explain this to me!

thebrianchen · 2021-07-01T21:54:37Z

dev/src/transaction.ts

  private _writeBatch: WriteBatch;
  private _backoff: ExponentialBackoff;
-  private _requestTag: string;
+  private _locks = new Map<string, Precondition>();


optional: rename to readDocs or something similar to indicate that this map contains documents read in the transaction.

I was confused initially, since these aren't really "locks".

Renamed to _readVersions and added comment. I also changed it to no longer store preconditions but just the underlying data as I think this makes more sense with the new name.

thebrianchen · 2021-07-01T21:58:14Z

dev/src/transaction.ts

-    documentReader.transactionId = this._transactionId;
-    return documentReader.get(this._requestTag);
+
+    if (!this._optimisticLocking) {


optional: for future me's sake, please add a short comment (not necessarily here) to describe how optimistic locking is implemented with verify + not using transaction ids.

Otherwise, it seems like it's just an optimistic locking is just an option that you pass into the transaction request.

Good suggestion. Comment added.

thebrianchen · 2021-07-01T22:00:15Z

dev/src/transaction.ts

+      documentReader.transactionId = this._transactionId;
+    }
+
+    return documentReader.get(this._requestTag).then(docs => {


Do we have to handle the error case here? If we fail to fetch a document inside a tx, but the error is caught in the transaction callback, the final commit() won't have the preconditions set.

I think we need to chain the for loop after a silencePromise().

I don't think we need to catch the error here. The error is bubbled up to the user. If the user choses to proceed, they know that we failed to fetch the document and cannot lock on it. We also do not know what to lock on as we have no information about the state of the document.

thebrianchen · 2021-07-01T22:04:05Z

dev/src/transaction.ts

+            })
+          );
+        } else {
+          this._locks.set(


What's the expected behavior if an optimistic transaction reads the same document twice, but gets a different value each time?

My intuition says the tx should fail since the document value changed within the scope of the transaction, and if that's the intended behavior, we should check that the document isn't in the map.

Yes, thanks for thinking about this. I copied the logic from Android.

thebrianchen · 2021-07-01T22:10:46Z

dev/src/transaction.ts

@@ -609,6 +671,8 @@ function isRetryableTransactionError(error: GoogleError): boolean {
        // IDs that have expired. While INVALID_ARGUMENT is generally not
        // retryable, we retry this specific case.
        return !!error.message.match(/transaction has expired/);
+      case StatusCode.FAILED_PRECONDITION:


question: why do we retry FAILED_PRECONDITION for optimistic locking?

The backend uses this error code when a document is updated after we read it during a transaction.

Please add a comment for this.

There should already be a comment (in the last diff).

thebrianchen · 2021-07-01T22:12:18Z

dev/src/write-batch.ts

    requestTag?: string;
    retryCodes?: number[];
    methodName?: FirestoreUnaryMethod;
+    preproccessor?: (request: api.ICommitRequest) => api.ICommitRequest;


Now that I've grasped the PR, the preprocessor implementation is so clean ✨

"Clean" is a much nicer word than "hacky" :)

thebrianchen · 2021-07-01T22:21:25Z

dev/test/transaction.ts

+    );
+  });
+
+  it('does not combines precondition if already set', () => {


ultranit: s/combines/combine :)

thebrianchen · 2021-07-01T22:22:56Z

dev/test/transaction.ts

+      getDocument(/* transaction= */ ''),
+      commit(undefined, [set, verify])
+    );
+  });


optional (based on my above comments in transaction.ts): add test to check precondition if the same doc is read twice, add test to check that preconditions hold even if the document get() fails.

thebrianchen · 2021-07-01T22:24:22Z

dev/src/index.ts

-   * are read during the transaction. These locks block other transactions,
-   * batched writes, and other non-transactional writes from changing that
-   * document. Any writes in a read-write transactions are committed once
+   * By default, read-write transactions obtain a pessimistic locks on all


I'll create a google doc to rewrite this section and add you.

thebrianchen

lgtm, just add the updated comments.

thebrianchen · 2021-07-02T16:28:55Z

dev/src/transaction.ts

@@ -609,6 +671,8 @@ function isRetryableTransactionError(error: GoogleError): boolean {
        // IDs that have expired. While INVALID_ARGUMENT is generally not
        // retryable, we retry this specific case.
        return !!error.message.match(/transaction has expired/);
+      case StatusCode.FAILED_PRECONDITION:


Please add a comment for this.

schmidt-sebastian · 2021-07-02T18:35:14Z

Changed this PR to specify a consistent read time once one has been obtained.

PatrikValkovic · 2024-01-15T10:50:25Z

What is the status of the PR? It is already approved but 2.5 years old without any updates. I would really appreciate this.

chore: re-arrange tests

89e5f6a

schmidt-sebastian requested review from a team as code owners June 29, 2021 21:44

product-auto-label bot added the api: firestore Issues related to the googleapis/nodejs-firestore API. label Jun 29, 2021

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jun 29, 2021

feat: add support for optimistic transactions

cb1a009

schmidt-sebastian force-pushed the mrschmidt/optimisictransactions branch from c8785c2 to cb1a009 Compare June 29, 2021 21:45

schmidt-sebastian changed the title ~~feat: add support for optimisitc transactions~~ feat: add support for optimistic transactions Jun 29, 2021

schmidt-sebastian requested a review from thebrianchen June 29, 2021 21:47

schmidt-sebastian assigned thebrianchen Jun 29, 2021

schmidt-sebastian added 2 commits June 29, 2021 15:47

Merge branch 'master' into mrschmidt/optimisictransactions

4d30c6a

Cleanup

3bac81b

thebrianchen reviewed Jul 1, 2021

View reviewed changes

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Jul 1, 2021

Feedback

6efe771

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Jul 2, 2021

Merge branch 'master' into mrschmidt/optimisictransactions

4a28819

thebrianchen approved these changes Jul 2, 2021

View reviewed changes

Use consistent read times

1837d87

schmidt-sebastian added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 2, 2021

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 2, 2021

bcoe added the owlbot:run Add this label to trigger the Owlbot post processor. label Jul 2, 2021

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jul 2, 2021

thebrianchen approved these changes Jul 2, 2021

View reviewed changes

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Jul 7, 2021

bcoe added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 4, 2022

bcoe added the kokoro:run Add this label to force Kokoro to re-run the tests. label Feb 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for optimistic transactions #1548

feat: add support for optimistic transactions #1548

schmidt-sebastian commented Jun 29, 2021 •

edited

thebrianchen left a comment

thebrianchen Jul 1, 2021

schmidt-sebastian Jul 2, 2021

thebrianchen Jul 1, 2021

schmidt-sebastian Jul 2, 2021

thebrianchen Jul 1, 2021

schmidt-sebastian Jul 2, 2021

thebrianchen Jul 1, 2021

schmidt-sebastian Jul 2, 2021

thebrianchen Jul 1, 2021

schmidt-sebastian Jul 2, 2021

thebrianchen Jul 2, 2021

schmidt-sebastian Jul 2, 2021

thebrianchen Jul 1, 2021

schmidt-sebastian Jul 2, 2021

thebrianchen Jul 1, 2021

schmidt-sebastian Jul 2, 2021

thebrianchen Jul 1, 2021

schmidt-sebastian Jul 2, 2021

thebrianchen Jul 1, 2021

schmidt-sebastian Jul 2, 2021

thebrianchen left a comment

thebrianchen Jul 2, 2021

schmidt-sebastian commented Jul 2, 2021

PatrikValkovic commented Jan 15, 2024 •

edited

feat: add support for optimistic transactions #1548

Are you sure you want to change the base?

feat: add support for optimistic transactions #1548

Conversation

schmidt-sebastian commented Jun 29, 2021 • edited

thebrianchen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thebrianchen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian commented Jul 2, 2021

PatrikValkovic commented Jan 15, 2024 • edited

schmidt-sebastian commented Jun 29, 2021 •

edited

PatrikValkovic commented Jan 15, 2024 •

edited