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

Mila/multi db #1321

Merged
merged 23 commits into from
May 19, 2023
Merged

Mila/multi db #1321

merged 23 commits into from
May 19, 2023

Conversation

milaGGL
Copy link
Contributor

@milaGGL milaGGL commented May 12, 2023 •
edited

Add multiDB support.

Ported from:

TODO:

  • Add 2 new Firestore::GetInstance public interface to accept dynamic database name
  • Update the firestore cache to use std::pair<App*, std::string> as key instead of App*, to identify DBs in the same app while creating and deleting firestore instances.
  • Add TestFirestoreWithDatabaseId and IsUsingFirestoreEmulator test helper method
  • Add test cases for multiDB support
    - New GetInstance method call verification
    - Instances' equality check
    - Can create & terminate multiple Firestore instances
    - Can create and fetch documents from different Firestore instances offline and online

@milaGGL milaGGL closed this May 12, 2023
@tom-andersen tom-andersen reopened this May 15, 2023
@tom-andersen tom-andersen self-requested a review May 15, 2023 14:44
@milaGGL milaGGL marked this pull request as ready for review May 16, 2023 15:52
@milaGGL milaGGL requested a review from tom-andersen May 16, 2023 20:07
tom-andersen
tom-andersen reviewed May 16, 2023
View reviewed changes
Copy link
Contributor

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

LGTM

firestore/src/common/firestore.cc Show resolved Hide resolved
firestore/src/include/firebase/firestore.h Show resolved Hide resolved
@milaGGL milaGGL requested a review from wu-hui May 18, 2023 14:14
wu-hui
wu-hui reviewed May 18, 2023
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.

Close to LGTM!

I do want to see less usage of const char* though, we should try to avoid raw pointers, especially when database id is not expensive to copy around.

firestore/integration_test_internal/src/firestore_test.cc Outdated Show resolved Hide resolved
firestore/integration_test_internal/src/util/locate_emulator.cc Outdated Show resolved Hide resolved
firestore/src/include/firebase/firestore.h
* the given database ID.
*/
static Firestore* GetInstance(::firebase::App* app,
const char* db_name,
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont have the API proposal at hand, but what is the reason to use const char* here instead of a const std::string& ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the original API proposal , it was const std::string&, and later requested to be changed to const char*. I am not sure why.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, at least we should find out why. I suppose there is a reason they asked for this.

firestore/src/common/firestore.cc Outdated Show resolved Hide resolved
firestore/integration_test_internal/src/firestore_test.cc Show resolved Hide resolved
firestore/src/main/firestore_main.h Show resolved Hide resolved
wu-hui
wu-hui approved these changes May 19, 2023
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, good work!

@milaGGL milaGGL merged commit 35f554b into main May 19, 2023
26 checks passed
@github-actions github-actions bot added the tests: in-progress This PR's integration tests are in progress. label May 19, 2023
@github-actions
Copy link

github-actions bot commented May 19, 2023 •
edited

❌  Integration test FAILED

Requested by @milaGGL on commit 35f554b
Last updated: Fri May 19 14:29 PDT 2023
View integration test log & download artifacts

Failures Configs
missing_log [TEST] [ERROR] [Windows] [x64] [openssl]
app_check [TEST] [FLAKINESS] [Android] [1/3 os: macos] [1/2 android_device: android_target]
(1 failed tests)  FirebaseAppCheckTest.TestDebugProviderValidToken
firestore [TEST] [FAILURE] [iOS] [macos] [1/2 ios_device: simulator_target]
(1 failed tests)  SmokeTest.TestCanWriteASingleDocument
[TEST] [FLAKINESS] [Android] [1/3 os: windows] [1/2 android_device: android_target]
(1 failed tests)  CRASH/TIMEOUT
gma [TEST] [FLAKINESS] [Android] [1/3 os: ubuntu] [1/2 android_device: android_target]
(1 failed tests)  FirebaseGmaTest.TestRewardedAdLoad

Add flaky tests to go/fpl-cpp-flake-tracker

@github-actions github-actions bot added the tests: failed This PR's integration tests failed. label May 19, 2023
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label May 19, 2023
@milaGGL milaGGL mentioned this pull request May 29, 2023
Merged
3 tasks
@firebase firebase locked and limited conversation to collaborators Jun 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tom-andersen tom-andersen tom-andersen approved these changes

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

Assignees

@tom-andersen tom-andersen

@milaGGL milaGGL

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@milaGGL @tom-andersen @wu-hui