[core][android] Introduce OfflineManager.runPackDatabaseAutomatically(boolean) API #15967

pozdnyakov · 2019-11-25T13:22:46Z

Fixes mapbox/mapbox-gl-native-team#102

chloekraw · 2019-11-26T01:56:20Z

@pozdnyakov @tmpsantos thanks for this -- I like the idea a lot!

Right now I'm wondering whether the naming and documentation of these APIs could be further clarified.

I think one way we could add greater clarity is by explicitly referencing the SQLite VACUUM operation by name. The cache management APIs @tmpsantos added a few months ago already reference VACUUM in the inline documentation, e.g.,

mapbox-gl-native/include/mbgl/storage/default_file_source.hpp

Line 216 in 753a640

* potentially slow because it will trigger a VACUUM on SQLite,

chloekraw · 2019-11-26T01:59:29Z

...rm/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/offline/OfflineManager.java

@@ -669,6 +669,23 @@ private boolean isValidOfflineRegionDefinition(OfflineRegionDefinition definitio
  @Keep
  public native void setOfflineMapboxTileCountLimit(long limit);

+  /**
+   * Sets the automatic database file packing.


FYI, when we're ready to merge, changes to platform files should be PRed separately in https://github.com/mapbox/mapbox-gl-native-android

This will be required after #15970 lands

include/mbgl/storage/default_file_source.hpp

chloekraw · 2019-11-26T02:20:09Z

include/mbgl/storage/default_file_source.hpp

@@ -195,6 +190,11 @@ class DefaultFileSource : public FileSource {
     */
    void packDatabase(std::function<void(std::exception_ptr)> callback);

+    /*
+     * Enables or disables auto database file packing after deleteing regions and clearing ambient cache.


Suggestion:

Sets whether packing the database by invoking VACUUM on SQLite occurs automatically after an offline region is deleted (deleteOfflineRegion()) or the ambient cache is cleared (clearAmbientCache()).

By default, packing is enabled. If disabled, disk space will not be freed after resources are removed unless packDatabase() is explicitly called.

include/mbgl/storage/default_file_source.hpp

chloekraw · 2019-11-26T03:12:18Z

include/mbgl/storage/default_file_source.hpp

+    /*
+     * Enables or disables auto database file packing after deleteing regions and clearing ambient cache.
+     */
+    void setAutopack(bool);


I think this API name is confusing. Naming this function void runPackDatabaseAutomatically(bool) would be a little clearer: the full reference to the related API is provided and it's obvious that if true, packDatabase is run.

But why not implement this functionality so that boolean runPackDatabase is accepted as a variable in existing functions? e.g.:

void deleteOfflineRegion(OfflineRegion&&, std::function<void(std::exception_ptr)>, bool runPackDatabase = true);

void clearAmbientCache(std::function<void (std::exception_ptr)>, bool runPackDatabase = true);

This way, the developer can decide with each function call whether to run packDatabase() automatically, and this new "setAutopack" API is not needed.

My understanding is that this wouldn't be a breaking change because if runPackDatabase is not passed in by the developer, it defaults to true which matches current behavior of both APIs.

In the future, my preference would be to name all APIs around VACUUM/database defragmentation with those words explicitly.

Unless packDatabase() does something additional to running VACUUM, what value do we provide the developer by wrapping a widely-known functionality (https://www.sqlitetutorial.net/sqlite-vacuum/) around this ambiguous word "pack"?

Unless packDatabase() does something additional to running VACUUM, what value do we provide the developer by wrapping a widely-known functionality (https://www.sqlitetutorial.net/sqlite-vacuum/) around this ambiguous word "pack"?

VACUUM is tied to sqlite, at some point we can switch to another DB provider. IMO it's better to keep API generic enough.

But why not implement this functionality so that boolean runPackDatabase is accepted as a variable in existing functions?

This does not match well with the Android API: java does not support method parameters set by default.

Also, we do not run VACUUM - we run PRAGMA incremental_vacuum which provides a different behavior, so we'd better not to disclose the implementation internals in the API.

VACUUM is tied to sqlite, at some point we can switch to another DB provider. IMO it's better to keep API generic enough.

Also, we do not run VACUUM - we run PRAGMA incremental_vacuum which provides a different behavior, so we'd better not to disclose the implementation internals in the API.

Gotcha, makes sense to not reference VACUUM in the API name. Still, I think perhaps an alternative to "pack" should be considered for the future, unless there's a great reason why "pack" makes sense that I'm overlooking.

@pozdnyakov what do you think about specifying VACUUM / PRAGMA incremental_vacuum in the inline documentation though?

This does not match well with the Android API: java does not support method parameters set by default.

😔 I guess our hands are tied here.

@pozdnyakov what do you think about changing the name from setAutopack to runPackDatabaseAutomatically then? (Also open to other suggestions.)

Finally -- is this function a global function for OfflineManager, so that if set, it would apply to every call to deleteOfflineRegion or clearAmbientCache that happens afterwards?

@pozdnyakov what do you think about specifying VACUUM / PRAGMA incremental_vacuum in the inline documentation though?

We could mention it as possible implementation detail, however, it's better IMO to just describe what it does - shrink the database file size, and point out the potential performance issues.

@pozdnyakov what do you think about changing the name from setAutopack to runPackDatabaseAutomatically then? (Also open to other suggestions.)

runPackDatabaseAutomatically looks much better, thanks for proposing it!

Finally -- is this function a global function for OfflineManager, so that if set, it would apply to every call to deleteOfflineRegion or clearAmbientCache that happens afterwards?

yeah, it'll apply to all the following calls of deleteOfflineRegion or clearAmbientCache.

platform/android/src/offline/offline_manager.cpp

...orm/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/offline/OfflineRegion.java

This reverts commit 9bc3aa4.

- added a unit test - Updated inline comments in default_file_source.hpp

pozdnyakov · 2019-11-27T12:18:21Z

@chloekraw Thanks for your comments! They are considered in the latest patch set.

tmpsantos · 2019-11-27T21:11:56Z

platform/default/src/mbgl/storage/offline_database.cpp

@@ -705,7 +705,7 @@ std::exception_ptr OfflineDatabase::clearAmbientCache() try {

    resourceQuery.run();

-    vacuum();
+    if (autopack) vacuum();


Optional: We could replace all the vacuum() calls with a protected mayPack() doing the autopack == true check and move the code inside ::vacuum to ::pack.

We've places where vacuum should be called unconditionally, e.g. at database creation in order to enable incremental vacuum

pozdnyakov · 2019-12-02T11:49:08Z

Note for the change log: Introduce OfflineManager.runPackDatabaseAutomatically(boolean) and remove the redundant OfflineRegion.deleteAndSkipPackDatabase() method.

tobrun · 2019-12-02T11:55:41Z

I will run point on integrating these changes in mapbox-gl-native-android

pozdnyakov added Android Mapbox Maps SDK for Android offline Core The cross-platform C++ core, aka mbgl labels Nov 25, 2019

pozdnyakov requested review from tmpsantos and tobrun November 25, 2019 13:22

pozdnyakov self-assigned this Nov 25, 2019

pozdnyakov force-pushed the mikhail_offlinedatabase_setautopack branch from 565ab6a to 753a640 Compare November 25, 2019 13:35

chloekraw reviewed Nov 26, 2019

View reviewed changes

platform/android/src/offline/offline_manager.cpp Outdated Show resolved Hide resolved

chloekraw reviewed Nov 26, 2019

View reviewed changes

...orm/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/offline/OfflineRegion.java Show resolved Hide resolved

pozdnyakov mentioned this pull request Nov 26, 2019

Introduce OfflineManager.clearAmbientCacheAndSkipPackDatabase() #15962

Closed

pozdnyakov added 4 commits November 26, 2019 17:36

Revert "[android] Add OfflineRegion.deleteAndSkipPackDatabase"

6f6a4ec

This reverts commit 9bc3aa4.

[core] Introduce OfflineDatabase::runPackDatabaseAutomatically() API

27e9be7

- added a unit test - Updated inline comments in default_file_source.hpp

[core] Add OfflineDatabase.Pack unit test

4160ce9

[android] Add OfflineManager.runPackDatabaseAutomatically(boolean) API

ada01e0

pozdnyakov force-pushed the mikhail_offlinedatabase_setautopack branch from 753a640 to ada01e0 Compare November 27, 2019 12:16

pozdnyakov marked this pull request as ready for review November 27, 2019 12:17

pozdnyakov changed the title ~~[core][android] Introduce OfflineManager.setAutopack(bool) API~~ [core][android] Introduce OfflineManager.runPackDatabaseAutomatically(boolean) API Nov 27, 2019

tmpsantos approved these changes Nov 27, 2019

View reviewed changes

tobrun approved these changes Nov 28, 2019

View reviewed changes

pozdnyakov merged commit e373d8a into master Dec 2, 2019

pozdnyakov deleted the mikhail_offlinedatabase_setautopack branch December 2, 2019 11:46

pozdnyakov mentioned this pull request Dec 9, 2019

[CP][core][android] Decouple vacuum from other operations on offline database #16029

Merged

chloekraw mentioned this pull request Mar 25, 2020

Expose platform bindings for new OfflineManager APIs mapbox/mapbox-gl-native-ios#239

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core][android] Introduce OfflineManager.runPackDatabaseAutomatically(boolean) API #15967

[core][android] Introduce OfflineManager.runPackDatabaseAutomatically(boolean) API #15967

pozdnyakov commented Nov 25, 2019

chloekraw commented Nov 26, 2019 •

edited

Loading

chloekraw Nov 26, 2019

chloekraw Nov 26, 2019 •

edited

Loading

chloekraw Nov 26, 2019

pozdnyakov Nov 26, 2019

pozdnyakov Nov 26, 2019

chloekraw Nov 27, 2019

pozdnyakov Nov 27, 2019

pozdnyakov commented Nov 27, 2019

tmpsantos Nov 27, 2019

pozdnyakov Nov 27, 2019

pozdnyakov commented Dec 2, 2019

tobrun commented Dec 2, 2019

[core][android] Introduce OfflineManager.runPackDatabaseAutomatically(boolean) API #15967

[core][android] Introduce OfflineManager.runPackDatabaseAutomatically(boolean) API #15967

Conversation

pozdnyakov commented Nov 25, 2019

chloekraw commented Nov 26, 2019 • edited Loading

Choose a reason for hiding this comment

chloekraw Nov 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pozdnyakov commented Nov 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pozdnyakov commented Dec 2, 2019

tobrun commented Dec 2, 2019

chloekraw commented Nov 26, 2019 •

edited

Loading

chloekraw Nov 26, 2019 •

edited

Loading