feat: add new Firestore.runAsyncTransaction #103

kvakos · 2020-02-11T17:49:53Z

Fixes #43 (it's a good idea to open an issue first for context and/or discussion)

googlebot · 2020-02-11T17:49:57Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

kvakos · 2020-02-11T18:10:06Z

@googlebot I signed it!

kvakos · 2020-02-12T08:40:24Z

@googlebot I signed it!

codecov · 2020-02-12T08:45:46Z

Codecov Report

Merging #103 into master will increase coverage by 0.05%.
The diff coverage is 95.23%.

@@             Coverage Diff              @@
##             master     #103      +/-   ##
============================================
+ Coverage     71.56%   71.62%   +0.05%     
- Complexity      968      969       +1     
============================================
  Files            62       62              
  Lines          5202     5222      +20     
  Branches        579      579              
============================================
+ Hits           3723     3740      +17     
- Misses         1299     1302       +3     
  Partials        180      180

Impacted Files	Coverage Δ	Complexity Δ
...n/java/com/google/cloud/firestore/Transaction.java	`97.77% <ø> (ø)`	`11 <0> (ø)`	⬇️
...java/com/google/cloud/firestore/FirestoreImpl.java	`82.35% <95.23%> (+0.28%)`	`24 <1> (+1)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eba397e...51a314d. Read the comment docs.

kvakos · 2020-02-12T09:22:02Z

@googlebot I signed it!

kvakos · 2020-02-12T11:45:08Z

@googlebot I signed it!

googlebot · 2020-02-12T11:45:12Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

kvakos · 2020-02-12T15:16:19Z

I cannot pass Binary Compatibility test. any suggestions?

BenWhitehead · 2020-02-12T20:43:12Z

Binary compatibility has broken due to the new interface methods, which would cause any existing implementers of the interface to no longer satisfy the interfaces requirements.

[ERROR] 7012: com.google.cloud.firestore.Firestore: Method 'public com.google.api.core.ApiFuture runTransactionAsync(com.google.cloud.firestore.Transaction$AsyncFunction)' has been added to an interface
[ERROR] 7012: com.google.cloud.firestore.Firestore: Method 'public com.google.api.core.ApiFuture runTransactionAsync(com.google.cloud.firestore.Transaction$AsyncFunction, com.google.cloud.firestore.TransactionOptions)' has been added to an interface

@chingor13 What is our general policy around adding to veneer interfaces? I know in generated protos we've been a bit more permissive.

@schmidt-sebastian Can you please review this contribution? I believe you're more familiar with the transaction handling logic in the code than I am.

BenWhitehead · 2020-02-12T20:44:38Z

(@schmidt-sebastian I can't assign this PR to you right now, have to track down some permissions)

schmidt-sebastian

Thanks for this. I think I can convince myself that is overall an improvement and that we can take this PR (please take a look at my comments for an eventual downside).

I think that as part of this, we need to make the Transactions class thread safe, since it is now more likely that user code is executed in parallel.. Does that sound reasonable?

schmidt-sebastian · 2020-02-18T18:15:14Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/Transaction.java

@@ -51,6 +51,16 @@
    T updateCallback(Transaction transaction) throws Exception;
  }

+  /**
+   * User callback that takes a Firestore Transaction


Please update to mention the async nature of this callback.

check it, please

schmidt-sebastian · 2020-02-18T18:17:02Z

google-cloud-firestore/src/test/java/com/google/cloud/firestore/TransactionTest.java

@@ -260,6 +373,55 @@ public String updateCallback(Transaction transaction) {
    assertEquals(commit(ByteString.copyFromUtf8("foo5")), requests.get(9));
  }

+  @Test
+  public void limitsRetriesWithFailureAsync() throws Exception {


This test doesn't seem to add test coverage over the existing limitsRetriesWithFailure() test. If you agree, we could probably remove it. Same applies to limitsRetriesWithSuccessAsync().

tests removed

schmidt-sebastian · 2020-02-18T18:18:49Z

google-cloud-firestore/src/test/java/com/google/cloud/firestore/TransactionTest.java

@@ -384,6 +630,46 @@ public void getMultipleDocuments() throws Exception {
    assertEquals(commit(TRANSACTION_ID), requests.get(2));
  }

+  @Test
+  public void getMultipleDocumentsAsync() throws Exception {


The above comment also applies here. The code path you are testing is already covered by getDocumentAsync(). This also applies to other test functions added below.

test removed

schmidt-sebastian · 2020-02-18T18:21:59Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/Firestore.java

+   * @return An ApiFuture that will be resolved with the result from updateFunction.
+   */
+  @Nonnull
+  <T> ApiFuture<T> runTransactionAsync(@Nonnull final Transaction.AsyncFunction<T> updateFunction);


We use the "async" suffix in other parts of the API to indicate that the function returns an ApiFuture. "Async" is usually coupled with a blocking API that lacks the suffix. I think what you are doing here is slightly different - runTransaction() is also async, but in a slightly different way.

What do you think of renaming these new methods to runAsyncTransaction?

method renamed

schmidt-sebastian · 2020-02-18T18:36:41Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/Firestore.java

+   * transaction. If any document read within the transaction has changed, the updateFunction will
+   * be retried. If it fails to commit after 5 attempts, the transaction will fail.
+   *
+   * @param updateFunction The function to execute within the transaction context.


One potential downside of your proposal is that it makes it now much easier to invoke transactions that take a long time to execute (as the user may now be inclined to kick off other tasks as well). Can you add something to the comment to explain these downsides? E.g. something like: "Running a transaction places locks all all consumed documents. To unblock other clients, the Firestore backend automatically releases all locks after 60 seconds of inactivity and fails all transactions that last longer than 270 seconds (see https://firebase.google.com/docs/firestore/quotas#writes_and_transactions)"

comment added to the javadoc

schmidt-sebastian · 2020-02-18T18:45:17Z

@chingor13 What is our general policy around adding to veneer interfaces? I know in generated protos we've been a bit more permissive.

I would not consider implementing a custom FirebaseFirestore a supported use case. While it is true that it might break some of our users, this limitation will make it very hard for us to add additional functionality.

schmidt-sebastian · 2020-03-10T21:29:42Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/Firestore.java

+  <T> ApiFuture<T> runAsyncTransaction(
+      @Nonnull final Transaction.AsyncFunction<T> updateFunction,
+      @Nonnull TransactionOptions transactionOptions);
+


Please move both these methods below the overload for runTransaction.

schmidt-sebastian · 2020-03-10T21:34:51Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java

@@ -384,7 +401,21 @@ public void onSuccess(List<WriteResult> writeResults) {
                  @Override
                  public void run() {
                    try {
-                      callbackResult.set(transactionCallback.updateCallback(transaction));
+                      ApiFuture<T> updateCallback = transactionCallback.updateCallback(transaction);
+                      ApiFutures.addCallback(


If I am reading the documentation of addCallback correctly it only runs the ApiFutureCallback on the provided executor. We actually want to run transactionCallback on the user executor. Do you mind fixing that and/or convincing me that I am mistaken?

You are right.

schmidt-sebastian · 2020-03-10T21:36:07Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/Transaction.java

@@ -51,6 +51,16 @@
    T updateCallback(Transaction transaction) throws Exception;
  }

+  /**
+   * User callback that takes a Firestore Async Transaction


"Async user callback that takes a Firestore transaction."

Can you also add a period to line 45?

period was added to both

schmidt-sebastian · 2020-03-10T21:36:22Z

google-cloud-firestore/src/test/java/com/google/cloud/firestore/TransactionTest.java

-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
+import static org.junit.Assert.*;


Please no glob imports.

global import replaced

schmidt-sebastian · 2020-03-10T21:37:35Z

google-cloud-firestore/src/test/java/com/google/cloud/firestore/TransactionTest.java

@@ -213,6 +298,34 @@ public String updateCallback(Transaction transaction) {
    assertEquals(1, requests.size());
  }

+  @Test
+  public void noRollbackOnBeginFailureAsync() throws Exception {


You didn't change this code path, so I think we can remove this test.

I duplicate all tests in TransactionTest.java that somehow play with runTransaction and implemented them with runAsyncTransaction. Old sync/blocking methods as it is now implemented also are testing async implementation. I can delete all of *async tests. Can I delete all *async tests?

Almost all *async tests were deleted except those with other mocked setup

schmidt-sebastian · 2020-03-10T21:38:44Z

google-cloud-firestore/src/test/java/com/google/cloud/firestore/TransactionTest.java

@@ -463,6 +653,40 @@ public QuerySnapshot updateCallback(Transaction transaction)
    assertEquals(commit(TRANSACTION_ID), requests.get(2));
  }

+  @Test
+  public void getQueryAsync() throws Exception {


Same comment as above.

schmidt-sebastian · 2020-03-10T21:38:53Z

google-cloud-firestore/src/test/java/com/google/cloud/firestore/TransactionTest.java

@@ -497,6 +721,42 @@ public String updateCallback(Transaction transaction) {
    assertEquals(commit(TRANSACTION_ID, writes.toArray(new Write[] {})), requests.get(1));
  }

+  @Test
+  public void updateDocumentAsync() throws Exception {


Same comment as above.

schmidt-sebastian · 2020-03-10T21:38:59Z

google-cloud-firestore/src/test/java/com/google/cloud/firestore/TransactionTest.java

@@ -532,6 +792,43 @@ public String updateCallback(Transaction transaction) {
    assertEquals(commit(TRANSACTION_ID, writes.toArray(new Write[] {})), requests.get(1));
  }

+  @Test
+  public void setDocumentAsync() throws Exception {


Same comment as above.

schmidt-sebastian · 2020-03-10T21:39:03Z

google-cloud-firestore/src/test/java/com/google/cloud/firestore/TransactionTest.java

@@ -567,6 +864,43 @@ public String updateCallback(Transaction transaction) {
    assertEquals(commit(TRANSACTION_ID, writes.toArray(new Write[] {})), requests.get(1));
  }

+  @Test
+  public void createDocumentAsync() throws Exception {


Same comment as above.

schmidt-sebastian · 2020-03-10T21:39:07Z

google-cloud-firestore/src/test/java/com/google/cloud/firestore/TransactionTest.java

@@ -604,4 +938,44 @@ public String updateCallback(Transaction transaction) {
    assertEquals(begin(), requests.get(0));
    assertEquals(commit(TRANSACTION_ID, writes.toArray(new Write[] {})), requests.get(1));
  }
+
+  @Test
+  public void deleteDocumentAsync() throws Exception {


Same comment as above.

kvakos · 2020-03-11T15:51:57Z

Can I resolve done conversation?

schmidt-sebastian

Thanks! Sorry for the long round trip time, but I hope this unblocks you.

schmidt-sebastian · 2020-03-11T16:20:45Z

google-cloud-firestore/src/test/java/com/google/cloud/firestore/TransactionTest.java

+                SettableApiFuture<String> apiFuture = SettableApiFuture.create();
+                apiFuture.set("foo");
+                return apiFuture;


Nit: Replace with https://github.com/googleapis/api-common-java/blob/master/src/main/java/com/google/api/core/ApiFutures.java#L107 (here and below)

replaced both places with ApiFutures#immediateFuture()

schmidt-sebastian · 2020-03-11T17:25:31Z

google-cloud-firestore/src/test/java/com/google/cloud/firestore/TransactionTest.java

+              public ApiFuture<String> updateCallback(Transaction transaction) {
+                SettableApiFuture<String> apiFuture = SettableApiFuture.create();
+                apiFuture.setException(new Exception("Expected exception"));
+                return apiFuture;


You can use an "immediateFailedFuture" here if you want to do another edit :)

schmidt-sebastian

@BenWhitehead In my opinion, this is good to merge. I don't know what the final decision on binary compatibility is, but I am signing off on this.

kvakos · 2020-03-11T22:53:34Z

To @schmidt-sebastian: I appreciate very much your help. Thanks a lot.

BenWhitehead · 2020-03-12T17:37:32Z

@kvakos Can you run mvn com.coveo:fmt-maven-plugin:format to fixup the source files for the Code Format check?

kvakos · 2020-03-13T08:28:32Z

Code format applied

BenWhitehead · 2020-03-13T17:44:43Z

Thanks @kvakos I'll follow up and get info about how the new interface methods will impact versioning.

BenWhitehead · 2020-03-19T20:43:58Z

@kvakos We've figured out what needs to be done as far as versioning is concerned and we should be able to merge this soon (most likely as is, at worst after a rebase).

BenWhitehead · 2020-03-20T19:30:43Z

I'll fix the binary compatibility build shortly in another PR

* updated CHANGELOG.md [ci skip] * updated README.md [ci skip] * updated versions.txt [ci skip] * updated proto-google-cloud-firestore-admin-v1/pom.xml [ci skip] * updated proto-google-cloud-firestore-v1/pom.xml [ci skip] * updated proto-google-cloud-firestore-v1beta1/pom.xml [ci skip] * updated grpc-google-cloud-firestore-admin-v1/pom.xml [ci skip] * updated grpc-google-cloud-firestore-v1/pom.xml [ci skip] * updated grpc-google-cloud-firestore-v1beta1/pom.xml [ci skip] * updated google-cloud-firestore/pom.xml [ci skip] * updated samples/install-without-bom/pom.xml [ci skip] * updated samples/pom.xml [ci skip] * updated samples/snapshot/pom.xml [ci skip] * updated samples/snippets/pom.xml [ci skip] * updated google-cloud-firestore-bom/pom.xml [ci skip] * updated pom.xml [ci skip] * chore: add notice to CHANGELOG.md regarding feature #103 Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Co-authored-by: BenWhitehead <BenWhitehead@users.noreply.github.com>

googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Feb 11, 2020

athakor added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 12, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 12, 2020

googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Feb 12, 2020

athakor added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 12, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 12, 2020

BenWhitehead self-requested a review February 12, 2020 20:43

schmidt-sebastian suggested changes Feb 18, 2020

View reviewed changes

schmidt-sebastian suggested changes Mar 10, 2020

View reviewed changes

schmidt-sebastian approved these changes Mar 11, 2020

View reviewed changes

schmidt-sebastian reviewed Mar 11, 2020

View reviewed changes

schmidt-sebastian approved these changes Mar 11, 2020

View reviewed changes

BenWhitehead added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 12, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 12, 2020

feat: Asynchronous transactions

51a314d

BenWhitehead added the kokoro:run Add this label to force Kokoro to re-run the tests. label Mar 13, 2020

yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Mar 13, 2020

BenWhitehead changed the title ~~Asynchronous transaction~~ feat: add new Firestore.runAsyncTransaction Mar 19, 2020

BenWhitehead added the semver: minor A new feature was added. No breaking changes. label Mar 19, 2020

BenWhitehead approved these changes Mar 20, 2020

View reviewed changes

BenWhitehead merged commit b28b660 into googleapis:master Mar 20, 2020

BenWhitehead mentioned this pull request Mar 20, 2020

chore: add clirr-ignored-differences.xml for new runAsyncTransaction methods added to Firestore #143

Merged

kvakos deleted the asyncTransaction branch March 21, 2020 15:49

BenWhitehead added a commit that referenced this pull request Apr 8, 2020

chore: add notice to CHANGELOG.md regarding feature #103

3c922a4

feat: add new Firestore.runAsyncTransaction #103

feat: add new Firestore.runAsyncTransaction #103

Conversation

kvakos commented Feb 11, 2020

googlebot commented Feb 11, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

kvakos commented Feb 11, 2020

kvakos commented Feb 12, 2020

codecov bot commented Feb 12, 2020 • edited Loading

Codecov Report

kvakos commented Feb 12, 2020

kvakos commented Feb 12, 2020

googlebot commented Feb 12, 2020

kvakos commented Feb 12, 2020

BenWhitehead commented Feb 12, 2020

BenWhitehead commented Feb 12, 2020

schmidt-sebastian 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

schmidt-sebastian commented Feb 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian Mar 10, 2020 • edited Loading

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

kvakos Mar 11, 2020 • edited Loading

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

kvakos commented Mar 11, 2020

schmidt-sebastian 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

schmidt-sebastian left a comment

Choose a reason for hiding this comment

kvakos commented Mar 11, 2020

BenWhitehead commented Mar 12, 2020

kvakos commented Mar 13, 2020

BenWhitehead commented Mar 13, 2020

BenWhitehead commented Mar 19, 2020

BenWhitehead commented Mar 20, 2020

codecov bot commented Feb 12, 2020 •

edited

Loading

schmidt-sebastian Mar 10, 2020 •

edited

Loading

kvakos Mar 11, 2020 •

edited

Loading