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

Firestore TransactionOptions added, to specify maxAttempts #318

Merged
merged 6 commits into from
Jul 7, 2022
Merged

Firestore TransactionOptions added, to specify maxAttempts #318

merged 6 commits into from
Jul 7, 2022

Conversation

dconeybe
Copy link
Contributor

This is a port of firebase/firebase-cpp-sdk#966, firebase/firebase-ios-sdk#9838 and firebase/firebase-android-sdk#3664, which adds the new TransactionOptions class to Firestore, which can be used to specify the maximum number of attempts to run a transaction before giving up. Previously, this maximum was hardcoded to 5.

@dconeybe dconeybe self-assigned this May 31, 2022
@google-cla
Copy link

google-cla bot commented May 31, 2022

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

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@google-cla google-cla bot added cla: no and removed cla: yes labels Jun 3, 2022
@dconeybe dconeybe added cla: yes and removed cla: no labels Jun 3, 2022
wu-hui
wu-hui approved these changes Jun 10, 2022
View reviewed changes
firestore/src/TransactionOptions.cs Outdated Show resolved Hide resolved
@google-cla google-cla bot added cla: no and removed cla: yes labels Jun 13, 2022
@google-cla google-cla bot added cla: no and removed cla: yes labels Jun 29, 2022
@dconeybe dconeybe changed the base branch from main to release-9.2.0-20220628-232109 June 29, 2022 14:35
@dconeybe dconeybe added cla: yes tests-requested: quick Trigger a quick set of integration tests. and removed cla: no labels Jun 29, 2022
@google-cla google-cla bot added cla: no and removed cla: yes labels Jun 29, 2022
@github-actions github-actions bot added tests: in-progress This PR's integration tests are in progress. and removed tests-requested: quick Trigger a quick set of integration tests. labels Jun 29, 2022
@github-actions
Copy link

github-actions bot commented Jun 29, 2022 •
edited
Loading

❌  Integration test FAILED

Requested by @firebase-workflow-trigger[bot] on commit 75497f2
Last updated: Wed Jul 6 17:08 PDT 2022
View integration test log & download artifacts

Failures Configs
analytics [TEST] [ERROR] [2019] [macos] [1/5 Platform(s): iOS] [1/3 Test Device(s): simulator_target]
firestore [BUILD] [ERROR] [2019] [macos] [1/3 Platform(s): Android]
[TEST] [ERROR] [2019] [macos] [1/5 Platform(s): iOS] [1/3 Test Device(s): ios_target]
[TEST] [FAILURE] [2019] [macos] [1/5 Platform(s): iOS] [1/3 Test Device(s): ios_target]
functions [TEST] [ERROR] [2019] [macos] [1/5 Platform(s): iOS] [1/3 Test Device(s): simulator_target]
installations [TEST] [ERROR] [2019] [macos] [1/5 Platform(s): iOS] [1/3 Test Device(s): simulator_target]
messaging [TEST] [ERROR] [2019] [macos] [2/5 Platform(s): iOS Android] [All 2 FTL Devices]
[TEST] [FAILURE] [2019] [macos] [1/5 Platform(s): iOS] [1/3 Test Device(s): simulator_target]
storage [TEST] [FAILURE] [2019] [macos] [4/5 Platform(s): ubuntu iOS macos windows] [1/3 Test Device(s): simulator_target]

@dconeybe dconeybe force-pushed the dconeybe/TransactionOptions branch from 3e5cce4 to cc9c484 Compare June 29, 2022 15:03
@google-cla google-cla bot added the cla: yes label Jun 29, 2022
@google-cla google-cla bot removed the cla: no label Jun 29, 2022
@dconeybe dconeybe force-pushed the dconeybe/TransactionOptions branch from cc9c484 to 43d2bcb Compare June 29, 2022 15:31
@dconeybe dconeybe changed the base branch from release-9.2.0-20220628-232109 to main June 29, 2022 15:32
@dconeybe dconeybe removed the tests: in-progress This PR's integration tests are in progress. label Jun 29, 2022
@github-actions github-actions bot added tests: in-progress This PR's integration tests are in progress. tests: failed This PR's integration tests failed. labels Jun 29, 2022
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jun 29, 2022
@dconeybe
Copy link
Contributor Author

dconeybe commented Jun 29, 2022 •
edited
Loading

https://github.com/firebase/firebase-unity-sdk/actions/runs/2584731985 triggered to build and run the full suite of integration tests.

Update: Build failed: ld: error: cannot find -lfirebase_auth. I think I need to include auth when testing firestore due to firestore's dependency on auth. I'll try again.

@dconeybe
Copy link
Contributor Author

dconeybe commented Jun 29, 2022 •
edited
Loading

https://github.com/firebase/firebase-unity-sdk/actions/runs/2584931823 triggered to build and run the full suite of integration tests, this time including auth with firestore to hopefully avoid the ld: error: cannot find -lfirebase_auth error.

Update: The build was successful; the integration tests have been started: https://github.com/firebase/firebase-unity-sdk/actions/runs/2585337183

Update: I couldn't get the build-2019-macos-latest-Android action to pass. It was apparently due to an Android NDK compatibility issue.

@dconeybe
Copy link
Contributor Author

dconeybe commented Jun 30, 2022 •
edited
Loading

https://github.com/firebase/firebase-unity-sdk/actions/runs/2592067531 triggered to build and run the full suite of integration tests, this time I've temporarily cherry-picked an Android NDK fix (5a934e1) to hopefully fix the build issues in the build-2019-macos-latest-Android action when running the integration tests.

(note that the last attempt at this run hung on Install Unity in macOS for 3+ hours: https://github.com/firebase/firebase-unity-sdk/actions/runs/2591114395)

Update: The build was successful; the integration tests have been started: https://github.com/firebase/firebase-unity-sdk/actions/runs/2592455470

@dconeybe
Copy link
Contributor Author

dconeybe commented Jul 1, 2022 •
edited
Loading

https://github.com/firebase/firebase-unity-sdk/actions/runs/2594580720 started after merging in the Android fix from the main branch.

Integration Tests: https://github.com/firebase/firebase-unity-sdk/actions/runs/2594914112 (failed on Android)

@dconeybe dconeybe removed the tests: failed This PR's integration tests failed. label Jul 1, 2022
@jonsimantov
Copy link
Contributor

Hmm, the Android build is still failing. @sunmou99 any idea if this is related?

@sunmou99
Copy link
Contributor

sunmou99 commented Jul 1, 2022 •
edited
Loading

Hmm, the Android build is still failing. @sunmou99 any idea if this is related?

The unity installation tool seems unstable. I will add retry logic into the tool itself.

@sunmou99 sunmou99 added the tests-requested: quick Trigger a quick set of integration tests. label Jul 6, 2022
@github-actions github-actions bot added tests: in-progress This PR's integration tests are in progress. and removed tests-requested: quick Trigger a quick set of integration tests. labels Jul 6, 2022
@dconeybe
Copy link
Contributor Author

dconeybe commented Jul 6, 2022

Build workflow: https://github.com/firebase/firebase-unity-sdk/actions/runs/2624659988

Integration tests: https://github.com/firebase/firebase-unity-sdk/actions/runs/2625079199

@github-actions github-actions bot added the tests: failed This PR's integration tests failed. label Jul 6, 2022
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jul 7, 2022
@dconeybe dconeybe merged commit 45781bc into main Jul 7, 2022
@dconeybe dconeybe deleted the dconeybe/TransactionOptions branch July 7, 2022 03:56
@firebase firebase locked and limited conversation to collaborators Aug 7, 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

@tom-andersen tom-andersen Awaiting requested review from tom-andersen

Assignees

@tom-andersen tom-andersen

@wu-hui wu-hui

@dconeybe dconeybe

Labels
api: firestore cla: yes tests: failed This PR's integration tests failed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dconeybe @jonsimantov @sunmou99 @wu-hui @tom-andersen