Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add new Firestore.runAsyncTransaction #103

Merged
merged 1 commit into from Mar 20, 2020
Merged

feat: add new Firestore.runAsyncTransaction #103

merged 1 commit into from Mar 20, 2020

Conversation

kvakos
Copy link
Contributor

@kvakos kvakos commented Feb 11, 2020

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

@googlebot
Copy link

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
Corporate signers

ℹ️ Googlers: Go here for more info.

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

kvakos commented Feb 11, 2020

@googlebot I signed it!

1 similar comment
@kvakos
Copy link
Contributor Author

kvakos commented Feb 12, 2020

@googlebot I signed it!

@athakor athakor added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 12, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 12, 2020
@codecov
Copy link

codecov bot commented Feb 12, 2020

Codecov Report

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

Impacted file tree graph

@@             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
Copy link
Contributor Author

kvakos commented Feb 12, 2020

@googlebot I signed it!

1 similar comment
@kvakos
Copy link
Contributor Author

kvakos commented Feb 12, 2020

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot 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 athakor added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 12, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 12, 2020
@kvakos
Copy link
Contributor Author

kvakos commented Feb 12, 2020

I cannot pass Binary Compatibility test. any suggestions?

@BenWhitehead
Copy link
Collaborator

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 BenWhitehead self-requested a review February 12, 2020 20:43
@BenWhitehead
Copy link
Collaborator

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

Copy link
Contributor

@schmidt-sebastian schmidt-sebastian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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

/**
* User callback that takes a Firestore Transaction
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update to mention the async nature of this callback.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check it, please

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

@Test
public void limitsRetriesWithFailureAsync() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests removed

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

@Test
public void getMultipleDocumentsAsync() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test removed

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

method renamed

* 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment added to the javadoc

@schmidt-sebastian
Copy link
Contributor

@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.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move both these methods below the overload for runTransaction.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved

@@ -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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right.

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

/**
* User callback that takes a Firestore Async Transaction
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Async user callback that takes a Firestore transaction."

Can you also add a period to line 45?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

period was added to both

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import static org.junit.Assert.*;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please no glob imports.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

global import replaced

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

@Test
public void noRollbackOnBeginFailureAsync() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@kvakos kvakos Mar 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@Test
public void getQueryAsync() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

@@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above.

@@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

@@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

@@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

@kvakos
Copy link
Contributor Author

kvakos commented Mar 11, 2020

Can I resolve done conversation?

Copy link
Contributor

@schmidt-sebastian schmidt-sebastian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 150 to 152
SettableApiFuture<String> apiFuture = SettableApiFuture.create();
apiFuture.set("foo");
return apiFuture;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replaced both places with ApiFutures#immediateFuture()

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@schmidt-sebastian schmidt-sebastian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Contributor Author

kvakos commented Mar 11, 2020

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

@BenWhitehead BenWhitehead added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 12, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 12, 2020
@BenWhitehead
Copy link
Collaborator

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

@kvakos
Copy link
Contributor Author

kvakos commented Mar 13, 2020

Code format applied

@BenWhitehead BenWhitehead added the kokoro:run Add this label to force Kokoro to re-run the tests. label Mar 13, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Mar 13, 2020
@BenWhitehead
Copy link
Collaborator

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

@BenWhitehead
Copy link
Collaborator

@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 BenWhitehead changed the title Asynchronous transaction feat: add new Firestore.runAsyncTransaction Mar 19, 2020
@BenWhitehead BenWhitehead added the semver: minor A new feature was added. No breaking changes. label Mar 19, 2020
@BenWhitehead
Copy link
Collaborator

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

@BenWhitehead BenWhitehead merged commit b28b660 into googleapis:master Mar 20, 2020
@kvakos kvakos deleted the asyncTransaction branch March 21, 2020 15:49
BenWhitehead added a commit that referenced this pull request Apr 8, 2020
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement. semver: minor A new feature was added. No breaking changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Firestore.runTransaction should support/recognize asynchronous updateFunction result
6 participants