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.

Sign up for GitHub

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

Jump to bottom

Tomandersen/transaction options #3664

Merged
merged 29 commits into from
Apr 27, 2022
Merged

Tomandersen/transaction options #3664

merged 29 commits into from
Apr 27, 2022

Conversation

tom-andersen
Copy link
Contributor

@tom-andersen tom-andersen commented Apr 19, 2022 •
edited by wu-hui
Loading

Add TransactionOptions to allow control of maximum attempts to commit before transaction fails.

Fix: #2877

eldhosembabu and others added 23 commits February 23, 2022 16:21
@eldhosembabu
- Changes to protoc version to make it working for m1 macs.
246a067
@eldhosembabu
- Changes to protoc version to make it working for m1 macs.
f7873ee
@eldhosembabu
- Changes to dependency versions to make it working for m1 macs.
5a31b19
@eldhosembabu
Merge remote-tracking branch 'origin/master' into m1-mac-fixes
1208d9d
@eldhosembabu
Merge remote-tracking branch 'origin/master' into m1-mac-fixes
97518d5
@eldhosembabu
- Reverting the robolectric version change
b8a85ef
@eldhosembabu
Revert "- Changes to dependency versions to make it working for m1 ma…
80da16e 
…cs."

This reverts commit 5a31b19
@eldhosembabu
- Updating robolectric versions to 4.7+ to get support for m1 macs.
cca600e 
See: robolectric/robolectric#6311
@eldhosembabu
Reverting some changes to check
090773a
@eldhosembabu
Merge remote-tracking branch 'origin/master' into m1-mac-fixes
73dd784
@eldhosembabu
Merge remote-tracking branch 'origin/master' into m1-mac-fixes
09e9664
@eldhosembabu
Merge remote-tracking branch 'origin/master' into m1-mac-fixes
61c4819
@eldhosembabu
Merge remote-tracking branch 'origin/master' into m1-mac-fixes
fcb9e05
@eldhosembabu
Merge remote-tracking branch 'origin/master' into m1-mac-fixes
c3132b1
@eldhosembabu
Merge remote-tracking branch 'origin/master' into m1-mac-fixes
3191148
@eldhosembabu
Merge remote-tracking branch 'origin/master' into m1-mac-fixes
c64e321
@eldhosembabu
Merge remote-tracking branch 'origin/master' into m1-mac-fixes
1fc153b
@eldhosembabu
Merge remote-tracking branch 'origin/master' into m1-mac-fixes
e70532f
@eldhosembabu
Merge remote-tracking branch 'origin/master' into m1-mac-fixes
b155fe7
@tom-andersen
Merge remote-tracking branch 'origin/master' into m1-mac-fixes
5119f6b
@tom-andersen
Merge remote-tracking branch 'origin/master' into m1-mac-fixes
5c5dcdb
@tom-andersen
Merge remote-tracking branch 'origin/master' into m1-mac-fixes
af391ae
@tom-andersen
Add TransactionOptions to allow control over number of attempts to co…
a93bfce 
…mmit, before transaction fails.
@tom-andersen tom-andersen requested a review from wu-hui April 19, 2022 23:14
@google-oss-bot
Copy link
Contributor

The public api surface has changed for the subproject firebase-firestore:
error: Added method com.google.firebase.firestore.FirebaseFirestore.runTransaction(com.google.firebase.firestore.TransactionOptions,com.google.firebase.firestore.Transaction.Function) [AddedMethod]
error: Added class com.google.firebase.firestore.TransactionOptions [AddedClass]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Apr 19, 2022 •
edited
Loading

Coverage Report 1

Affected Products

  • firebase-firestore

    Overall coverage changed from 46.25% (b600e83) to 46.04% (40b73c5) by -0.21%.

    FilenameBase (b600e83)Merge (40b73c5)Diff
    AsyncQueue.java78.61%78.11%-0.50%
    Datastore.java32.14%29.76%-2.38%
    FirebaseFirestore.java37.08%36.87%-0.21%
    FirestoreChannel.java18.10%16.38%-1.72%
    FirestoreClient.java38.52%30.88%-7.64%
    GrpcCallProvider.java69.41%62.35%-7.06%
    IndexBackfiller.java92.19%76.56%-15.63%
    LruGarbageCollector.java93.46%84.11%-9.35%
    SetMutation.java94.44%97.22%+2.78%

Test Logs

Notes

  • Commit (40b73c5) is created by Prow via merging PR base commit (b600e83) and head commit (1d62d46).
  • Run gradle <product>:checkCoverage to produce HTML coverage reports locally. After gradle commands finished, report files can be found under <product-build-dir>/reports/jacoco/.

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/qLDYYy7VUN.html

@tom-andersen
Merge remote-tracking branch 'origin/tomandersen/transactionOptions' …
c6d0361 
…into tomandersen/transactionOptions

# Conflicts:
#	firebase-firestore/src/main/java/com/google/firebase/firestore/TransactionOptions.java
@google-oss-bot
Copy link
Contributor

google-oss-bot commented Apr 19, 2022 •
edited
Loading

Size Report 1

Affected Products

  • firebase-firestore

    TypeBase (b600e83)Merge (40b73c5)Diff
    aar1.25 MB1.26 MB+2.19 kB (+0.2%)
    apk (release)3.38 MB3.38 MB+852 B (+0.0%)

Test Logs

Notes

  • Commit (40b73c5) is created by Prow via merging PR base commit (b600e83) and head commit (1d62d46).

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/dTHDZirEe8.html

@google-oss-bot
Copy link
Contributor

The public api surface has changed for the subproject firebase-firestore:
error: Added method com.google.firebase.firestore.FirebaseFirestore.runTransaction(com.google.firebase.firestore.TransactionOptions,com.google.firebase.firestore.Transaction.Function) [AddedMethod]
error: Added class com.google.firebase.firestore.TransactionOptions [AddedClass]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

@tom-andersen tom-andersen marked this pull request as ready for review April 19, 2022 23:46
@wu-hui wu-hui requested a review from markarndt April 20, 2022 00:45
wu-hui
wu-hui requested changes Apr 20, 2022
View reviewed changes
Copy link
Contributor

@wu-hui wu-hui left a comment

Choose a reason for hiding this comment

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

Looking good, some nits on comments.

firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/TransactionOptions.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/TransactionOptions.java Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/TransactionOptions.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/TransactionOptions.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/TransactionOptions.java Outdated Show resolved Hide resolved
@wu-hui wu-hui assigned tom-andersen and unassigned wu-hui Apr 20, 2022
@wu-hui
Copy link
Contributor

wu-hui commented Apr 20, 2022

You should also add an entry to CHANGELOG.md.

See here: https://osscs.corp.google.com/firebase-sdk/firebase-android-sdk/+/master:firebase-firestore/CHANGELOG.md;l=5

tom-andersen
tom-andersen commented Apr 21, 2022
View reviewed changes
Copy link
Contributor Author

@tom-andersen tom-andersen left a comment

Choose a reason for hiding this comment

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

Addressed comments

firebase-firestore/src/main/java/com/google/firebase/firestore/TransactionOptions.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/TransactionOptions.java Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/TransactionOptions.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/TransactionOptions.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/TransactionOptions.java Outdated Show resolved Hide resolved
tom-andersen
tom-andersen commented Apr 21, 2022
View reviewed changes
Copy link
Contributor Author

@tom-andersen tom-andersen left a comment

Choose a reason for hiding this comment

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

Fixed changes

@tom-andersen tom-andersen requested a review from wu-hui April 21, 2022 22:14
wu-hui
wu-hui approved these changes Apr 22, 2022
View reviewed changes
Copy link
Contributor

@wu-hui wu-hui left a comment

Choose a reason for hiding this comment

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

LGTM, but please merge it after you get tech writer approval.

@wu-hui wu-hui removed their assignment Apr 22, 2022
@tom-andersen tom-andersen merged commit 672c6dc into master Apr 27, 2022
@tom-andersen tom-andersen deleted the tomandersen/transactionOptions branch April 27, 2022 18:00
dconeybe added a commit to firebase/firebase-ios-sdk that referenced this pull request May 25, 2022
@dconeybe
Firestore/CHANGELOG.md: Copied message from firebase/firebase-android…
10e2001 
…-sdk#3664
This was referenced May 25, 2022
Merged
Closed
Merged
@firebase firebase locked and limited conversation to collaborators May 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wu-hui wu-hui wu-hui approved these changes

@markarndt markarndt Awaiting requested review from markarndt

Assignees

@tom-andersen tom-andersen

@markarndt markarndt

Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Very long (10 - 30 seconds) transaction failure timeout while offline
5 participants
@tom-andersen @google-oss-bot @wu-hui @markarndt @eldhosembabu