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 support for read-only transactions in TransactionOptions #320

Merged
merged 15 commits into from Aug 12, 2020
Merged

feat: add support for read-only transactions in TransactionOptions #320

merged 15 commits into from Aug 12, 2020

Conversation

BenWhitehead
Copy link
Collaborator

  • Add new typesafe builders for read-only TransactionOptions.readOnlyOptionsBuilder and read-write TransactionOptions.readWriteOptionsBuilder transactions in TransactionOptions
    • These new builders ensure only those parameters relevant to the respective type of transaction are available
  • Deprecate existing TransactionOptions.create(...) methods in favor of the new builders
  • Deprecate existing and annotate @internalapi TransactionOptions.getNumberOfAttempts
  • Update Transaction and TransactionRunner to use TransactionOptions directly

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 6, 2020
@BenWhitehead BenWhitehead added this to the v2 milestone Aug 6, 2020
@codecov
Copy link

codecov bot commented Aug 6, 2020

Codecov Report

Merging #320 into master will increase coverage by 0.14%.
The diff coverage is 87.69%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #320      +/-   ##
============================================
+ Coverage     72.62%   72.76%   +0.14%     
- Complexity     1049     1054       +5     
============================================
  Files            64       64              
  Lines          5468     5512      +44     
  Branches        673      679       +6     
============================================
+ Hits           3971     4011      +40     
- Misses         1286     1289       +3     
- Partials        211      212       +1     
Impacted Files Coverage Δ Complexity Δ
...java/com/google/cloud/firestore/FirestoreImpl.java 78.57% <ø> (-0.98%) 26.00 <0.00> (-3.00)
...n/java/com/google/cloud/firestore/Transaction.java 85.41% <33.33%> (-12.15%) 11.00 <0.00> (ø)
...com/google/cloud/firestore/TransactionOptions.java 96.00% <95.65%> (+21.00%) 12.00 <10.00> (+6.00)
.../com/google/cloud/firestore/TransactionRunner.java 100.00% <100.00%> (ø) 5.00 <3.00> (+1.00)
...in/java/com/google/cloud/firestore/BulkWriter.java 71.49% <0.00%> (+0.46%) 29.00% <0.00%> (+1.00%)

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 e7054df...738e82c. Read the comment docs.

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.

Technically, this looks good. This is however very "Java-y" in that in uses so much code to achieve comparatively little. Before I look in depth in all parts here (and I think in general the code is fine), I would like to ask if it is possible to merge all these builders into one TransactionOptionsBuilder (with setExecutor, setNumberOfAttempts, setReadTime and setMode). The only downside I see that that it would now be technically possible to set a read time for ReadWrite transactions, but it would shave off almost all of the code in this PR.

FWIW, we could also do something like:

TransactionOptionsBuilder

  • setExecutor
  • setNumberOfAttempts
  • setReadWrite()
  • setReadOnly(nullable readTime)

@BenWhitehead
Copy link
Collaborator Author

I'll flush out the java docs for the builder methods next week.

If we decide to do retries for Read only I can look at reducing the "java-yness" of the builders.

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.

I think I am convinced that we shouldn't retry read_only transactions (based on the 60s cutoff for readTime). For better or worse, we will already retry BatchGetDocuments request that fail with ABORTED even when inside a transaction.

I will take another pass once we have docs.

@Nonnull
public abstract TransactionOptions build();

static ReadOnlyOptionsBuilder readOnlyBuilder() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Marking as resolved due to the discrepancy between retry/readTime.

* Add new typesafe builders for read-only `TransactionOptions.readOnlyOptionsBuilder` and read-write `TransactionOptions.readWriteOptionsBuilder` transactions in TransactionOptions
  * These new builders ensure only those parameters relevant to the respective type of transaction are available
* Deprecate existing `TransactionOptions.create(...)` methods in favor of the new builders
* Deprecate existing and annotate @internalapi `TransactionOptions.getNumberOfAttempts`
* Update Transaction and TransactionRunner to use TransactionOptions directly
/**
* A typesafe builder class representing those options that are applicable when configuring a
* transaction to be read-only. All methods function as "set" rather than returning a new copy
* with a value set on it.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would drop "All methods function as "set" rather than returning a new copy with a value set on it." since this seems expected.

@schmidt-sebastian
Copy link
Contributor

Basically LGTM, pending our discussion on chat on whether we can simplify the TransactionOptions getters.

@BenWhitehead BenWhitehead added the automerge Merge the pull request once unit tests and other checks pass. label Aug 12, 2020
@gcf-merge-on-green
Copy link
Contributor

Your PR has attempted to merge for 3 hours. Please check that all required checks have passed, you have an automerge label, and that all your reviewers have approved the PR

@BenWhitehead BenWhitehead removed the automerge Merge the pull request once unit tests and other checks pass. label Aug 12, 2020
Comment on lines 94 to 99
} else if (TransactionOptionsType.READ_ONLY.equals(transactionOptions.getType())
&& transactionOptions.getReadTime() != null) {
beginTransaction
.getOptionsBuilder()
.getReadOnlyBuilder()
.setReadTime(transactionOptions.getReadTime());
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we must call beginTransaction.getOptionsBuilder().getReadOnlyBuilder() for read-only transaction even if we don't have a read time. You can probably just drop && transactionOptions.getReadTime() != null (and maybe also && previousTransactionId != null).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch. I did need to handle the else branch here that was previously taken care of by the options class. I've updated the code to handle both cases.

@BenWhitehead BenWhitehead merged commit c25dca3 into googleapis:master Aug 12, 2020
@BenWhitehead BenWhitehead deleted the ro-tx branch August 12, 2020 19:21
gcf-merge-on-green bot pushed a commit that referenced this pull request Aug 14, 2020
🤖 I have created a release \*beep\* \*boop\* 
---
## [2.0.0](https://www.github.com/googleapis/java-firestore/compare/v1.35.2...v2.0.0) (2020-08-14)


### New Features

#### Query Partition API

New API and backend RPC which allows for fetching a set of cursor keys for a 
Collection Group Query. Accessible via the new [`CollectionGroup#getPartitions(long,ApiStreamObserver)`](https://googleapis.dev/java/google-cloud-firestore/2.0.0/com/google/cloud/firestore/CollectionGroup.html#getPartitions-long-com.google.api.gax.rpc.ApiStreamObserver-) method. 

#### Read-Only Transaction Options

[`TransactionOptions`](https://googleapis.dev/java/google-cloud-firestore/2.0.0/com/google/cloud/firestore/TransactionOptions.html)
has been refactored to provide the ability to configure options for read-only 
transactions along with the existing configuration for read-write transactions.

This new ability is provided via the new [`TransactionOptions.createReadOnlyOptionsBuilder()`](https://googleapis.dev/java/google-cloud-firestore/2.0.0/com/google/cloud/firestore/TransactionOptions.html#createReadOnlyOptionsBuilder--) 
type safe builder.

Along with the new type safe builder for read-only options, there is a new type 
safe builder for read-write options as well accessible via [`TransactionOptions.createReadWriteOptionsBuilder()`](https://googleapis.dev/java/google-cloud-firestore/2.0.0/com/google/cloud/firestore/TransactionOptions.html#createReadWriteOptionsBuilder--). Each of the existing `TransactionOptions.create(...)`
methods for configuring read-write options has been deprecated in favor of the new builder.

#### EmulatorCredentials

`com.google.cloud.firestore.FirestoreOptions.Builder.FakeCredentials` has been
made static and renamed to `com.google.cloud.firestore.FirestoreOptions.EmulatorCredentials`
allowing instantiation outside `FirestoreOptions.Builder`.

When connecting to the Cloud Firestore Emulator via `FirestoreOptions` rather than
the environment variable `FIRESTORE_EMULATOR_HOST`, a custom credential implementation
must be specified to allow various admin operations in the emulator. Previously
this required users to create their own implementation due to it not being 
possible to construct a `FakeCredential`. As part of this change, `EmulatorCredentials`
is static and therefore able to be constructed from any location.

### Breaking Changes

#### New Firestore Admin Client API Artifact

The Cloud Firestore Admin Client has been migrated to its own maven artifact `com.google.cloud:google-cloud-firestore-admin`
rather than being bundled in `com.google.cloud:google-cloud-firestore`. All 
packages and classes have retained their existing names.

The new artifact is included in the `com.google.cloud:google-cloud-firestore-bom`, 
`com.google.cloud:google-cloud-bom` and `com.google.cloud:libraries-bom` 
artifacts and is accessible by adding the new dependency to your `pom.xml` file:

```xml
<dependency>
  <groupId>com.google.cloud</groupId>
  <artifactId>google-cloud-firestore-admin</artifactId>
</dependency>
```

#### Removal of v1beta1

Cloud Firestore has been GA for some time now, and the `google-cloud-firestore` 
code base has been using the protos and generated classes for the v1 api since 
that time. As such, we will no longer be publishing artifacts for the deprecated
v1beta1 protos. All functionality from v1beta1 is present in v1, and all users
should update any code to use v1.

#### Removal of support for `java.util.Date` in Snapshots

It is no longer possible to configure the ability for `java.util.Date` to be
returned from `DocumentSnapshot.get(FieldPath)` or `DocumentSnapshot.getData()`
for properties which are stored as Timestamps in Cloud Firestore.

The default behavior has been to return `com.google.cloud.Timestamp` by default
for some time, and is now the only option. Any code that is dependent on the old
behavior must be updated to use Timestamps instead of Date.

### Laundry List of Pull Requests

#### ⚠ BREAKING CHANGES

* add support for the Query Partition API (#202)
  * `Firestore#collectionGroup(...)` has a new return type `CollectionGroup` 
    which requires any code that previously used the method be re-compiled to
    pick up the new signature. `CollectionGroup` extends `Query` and as such 
    does not require your code to be updated, only the compiled class files.
* move FirestoreAdminClient and associated classes to new artifact google-cloud-firestore-admin (#311)
* remove deprecated v1beta1 protos and grpc client (#305)
* remove deprecated FirestoreOptions#setTimestampsInSnapshotsEnabled (#308)
* remove deprecated getCollections() methods (#307)
* various renames due to generator changes

#### Features

* add support for read-only transactions in TransactionOptions ([#320](https://www.github.com/googleapis/java-firestore/issues/320)) ([c25dca3](https://www.github.com/googleapis/java-firestore/commit/c25dca3ed6ca0c156ec60569ebc9f3a481bd4fee))
* add support for the Query Partition API ([#202](https://www.github.com/googleapis/java-firestore/issues/202)) ([3996548](https://www.github.com/googleapis/java-firestore/commit/39965489cbc836af573e500d57007c88241d7eb6))


#### Bug Fixes

* refactor FakeCredentials ([#325](https://www.github.com/googleapis/java-firestore/issues/325)) ([269e62c](https://www.github.com/googleapis/java-firestore/commit/269e62c6b8031d48e7f2e282b09b5ffcfadae547)), closes [#190](https://www.github.com/googleapis/java-firestore/issues/190)


#### Dependencies

* update dependency com.google.cloud:google-cloud-shared-dependencies to v0.8.5 ([#322](https://www.github.com/googleapis/java-firestore/issues/322)) ([1b21350](https://www.github.com/googleapis/java-firestore/commit/1b21350c0bc4a21cee2b281f944cbd061b1f8898))
* update dependency com.google.cloud:google-cloud-shared-dependencies to v0.8.6 ([#324](https://www.github.com/googleapis/java-firestore/issues/324)) ([b945fdb](https://www.github.com/googleapis/java-firestore/commit/b945fdb04da76a1e007d012c809449c5a43bb990))
* update jackson dependencies to v2.11.2 ([#314](https://www.github.com/googleapis/java-firestore/issues/314)) ([15d68cd](https://www.github.com/googleapis/java-firestore/commit/15d68cd93ac1fd206895fd37155a9ba82b9196ca))


#### Miscellaneous Chores

* enable gapicv2 ([#188](https://www.github.com/googleapis/java-firestore/issues/188)) ([92224bc](https://www.github.com/googleapis/java-firestore/commit/92224bcd52aa88cc6eb1da28747de0535d776a0f))
* move FirestoreAdminClient and associated classes to new artifact google-cloud-firestore-admin ([#311](https://www.github.com/googleapis/java-firestore/issues/311)) ([03ef755](https://www.github.com/googleapis/java-firestore/commit/03ef755dd164e6f1ec749f3f985b913b5ae23d14))
* remove deprecated FirestoreOptions#setTimestampsInSnapshotsEnabled ([#308](https://www.github.com/googleapis/java-firestore/issues/308)) ([7255a42](https://www.github.com/googleapis/java-firestore/commit/7255a42bcee3a6938dd5fafaef3465f948f39600))
* remove deprecated getCollections() methods ([#307](https://www.github.com/googleapis/java-firestore/issues/307)) ([bb4ddf1](https://www.github.com/googleapis/java-firestore/commit/bb4ddf1ce3cc3bd2e06a4ad5097bd18060e4467b))
* remove deprecated v1beta1 protos and grpc client ([#305](https://www.github.com/googleapis/java-firestore/issues/305)) ([96adacb](https://www.github.com/googleapis/java-firestore/commit/96adacbf52ace27e54b7a210d7c73b46922fbcbd))
* add BulkWriter ([#323](https://www.github.com/googleapis/java-firestore/issues/323)) ([e7054df](https://www.github.com/googleapis/java-firestore/commit/e7054df79b4139fdfd0cc6aa0620fbfa1a10a6b0))
* make BulkWriter package private ([#330](#330)) ([ef0869a](ef0869a))

---


This PR was generated with [Release Please](https://github.com/googleapis/release-please).
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants