Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Expose offline database merge API #12860

Merged
merged 5 commits into from
Sep 26, 2018
Merged

Conversation

LukasPaczos
Copy link
Member

Closes #12692.

@LukasPaczos LukasPaczos added Android Mapbox Maps SDK for Android in progress labels Sep 11, 2018
@LukasPaczos LukasPaczos added this to the android-v6.6.0 milestone Sep 11, 2018
@LukasPaczos LukasPaczos force-pushed the 12692-android-offline-db-merge branch 2 times, most recently from ef5bc00 to e7983f4 Compare September 11, 2018 15:28
* Only resources and tiles that belong to a region will be copied over. Identical
* regions will be flattened into a single new region in the main database.
* <p>
* A callback with a `MapboxOfflineTileCountExceededException` error will be invoked if
Copy link
Contributor

Choose a reason for hiding this comment

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

How/Where is this handled? I don't see any reference to this exception type anywhere else.

Copy link
Member Author

@LukasPaczos LukasPaczos Sep 18, 2018

Choose a reason for hiding this comment

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

Good catch, we pushing the error message through MergeOfflineRegionsCallback#onError. The doc needs to be updated.

@LukasPaczos LukasPaczos force-pushed the 12692-android-offline-db-merge branch 5 times, most recently from 218e809 to 39e549a Compare September 18, 2018 15:12
@LukasPaczos
Copy link
Member Author

This one is ready for a review but still relies on #12876 and waits for a fix for I/O errors thrown by locally generated databases.

@kkaefer
Copy link
Contributor

kkaefer commented Sep 18, 2018

Debugged the crash and found that it was crashing with SQLITE_IOERR, and extended error code SQLITE_IOERR_GETTEMPPATH. In our database merging code, we're using temporary tables, which may generate temporary files. Since Android doesn't have a default temporary directory, we have to explicitly set one.

@kkaefer
Copy link
Contributor

kkaefer commented Sep 18, 2018

  • When the test is started without other tests being run prior to it, the database doesn't exist yet and we get "Merge database has incorrect user_version". (Note that the main database doesn't exist yet, the database that we are merging from does exist)
  • Once the test has run, the main database already contains the region. Subsequent runs will not exercise the merging code (and not trigger the temp file crash). We should restore the original database before every run

@LukasPaczos
Copy link
Member Author

When the test is started without other tests being run prior to it, the database doesn't exist yet and we get "Merge database has incorrect user_version". (Note that the main database doesn't exist yet, the database that we are merging from does exist)

I haven't noticed these issues locally or during firebase runs and capturing from an internal convo that it might be dependent on other variables like run profiles.

Once the test has run, the main database already contains the region. Subsequent runs will not exercise the merging code (and not trigger the temp file crash). We should restore the original database before every run

Each firebase run is performed on a clean install, so we should exercise the merging code with every push. The instrumentation test is also deleting the region, the only things that persist are tiles. When it comes to the example activity, the region can be removed via the DeleteRegionActivity example activity that manages a list of locally stored regions.

@kkaefer
Copy link
Contributor

kkaefer commented Sep 24, 2018

Each firebase run is performed on a clean install

yeah, but it means that local testing will be much harder.

@LukasPaczos
Copy link
Member Author

it means that local testing will be much harder

True, I can try removing the database when exiting the test activity.

@LukasPaczos
Copy link
Member Author

Removing database would require recreating the singleton FileSource instance or exposing OfflineDatabase::removeExisting, otherwise, the DB will be recreated only when resources are requested and fail once. I feel like it's better to clear app's local storage and restart the process instead.

Copy link
Member

@tobrun tobrun left a comment

Choose a reason for hiding this comment

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

🚀

@LukasPaczos LukasPaczos merged commit 22c2c88 into master Sep 26, 2018
@LukasPaczos LukasPaczos deleted the 12692-android-offline-db-merge branch September 26, 2018 11:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants